RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated

Kim Barrett kim.barrett at oracle.com
Fri Apr 27 16:50:04 UTC 2018


> On Apr 27, 2018, at 4:56 AM, Michal Vala <mvala at redhat.com> wrote:
> 
> 
> 
> On 04/27/2018 12:55 AM, Kim Barrett wrote:
>>> On Apr 25, 2018, at 10:51 AM, Andrew Hughes <gnu.andrew at redhat.com> wrote:
>>> 
>>> On 24 April 2018 at 20:17, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>>> On Apr 23, 2018, at 3:51 AM, Michal Vala <mvala at redhat.com> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> following discussion "RFR: build pragma error with gcc 4.4.7"[1], I'm posting updated patch replacing deprecated readdir_r with readdir. Bug ID is JDK-8179887 [2].
>>>>> 
>>>>> webrev: http://cr.openjdk.java.net/~andrew/8179887/webrev/
>>>>> 
>>>>> I'm asking for the review and eventually sponsorship.
>>>> 
>>> 
>>> The patch looks good to me.
>>> 
>>>> The change to os::readdir in os_linux.inline.hpp looks fine.
>>>> 
>>>> I was going to suggest that rather than changing perfMemory_linux.cpp to use os::readdir in an
>>>> unusual and platform-specific way, it should instead just call ::readdir directly.  However, neither
>>>> of those is right, and that part of the change should not be made; see
>>>> https://bugs.openjdk.java.net/browse/JDK-8134540
>>>> Much nearly duplicated code for PerfMemory support
>>>> 
>>> 
>>> I think, in the circumstances, Michal's solution is the best option at
>>> this point.
>>> 8134540 looks like a more long-term change currently scheduled for 12 (is
>>> anyone currently working on it?), whereas this fix could go into 11 and remove
>>> a couple of unneeded memory allocations.
>>> 
>>>> Looking a bit deeper, there might be some additional follow-on that could be done, eliminating
>>>> the second argument to os::readdir.
>>>> windows: unused
>>>> bsd: freebsd also deprecated readdir_r, I think for similar reasons.
>>>> solaris: doc is clear about thread safety issue being about sharing the DIR*
>>>> aix: I haven’t looked at it, but would guess it’s similar.
>>>> 
>>>> In other words, don’t operate on the DIR* from multiple threads simultaneously, and just use
>>>> ::readdir on non-Windows.  That would all be for another RFE though.
>>>> 
>>>> 
>>> 
>>> This also seems like a more in-depth separate change and not one that
>>> can be performed
>>> by any of us alone, as we don't test all these platforms. As it
>>> stands, Michal's change affects
>>> Linux and Linux alone, and the addition of the use of NULL will make
>>> it clearer that moving
>>> to an os:readdir is feasible on that platform.
>> I disagree, and still think the perfMemory_linux.cpp change should be
>> removed.
>> (1) The change to perfMemory_linux.cpp is entirely unnecessary to
>> address the problem this bug is about.
>> (2) It violates the (implied) protocol for os::readdir.  If
>> Linux-specific code wants to invoke Linux-specific behavior, it should
>> do so by calling a Linux-specific API, not abuse an intended to be
>> portable API.
>> (3) It minorly interferes with some desirable future work.  If there
>> were some significant benefit to doing so, I wouldn't give this much
>> weight, but I don't see a significant benefit.
>> (4) The only benefit is saving some rare short-term memory
>> allocations.  I don't think that's worth the above costs.
>> Note that the Windows version of os::readdir also ignores the second
>> argument, but all callers provide it anyway.
>> I've opened a new CR for general os::readdir cleanup.
>> https://bugs.openjdk.java.net/browse/JDK-8202353
> 
> Ok, if we agree on os_linux.inline.hpp part, I think it should go in. Rest can be solved in JDK-8202353, which should imho include also removal of second argument of os::readdir, once it's unused.
> 
> For now, proposed patch looks like this:
> 
> --- old/src/hotspot/os/linux/os_linux.inline.hpp	2018-04-20 09:16:34.498343185 +0200
> +++ new/src/hotspot/os/linux/os_linux.inline.hpp	2018-04-20 09:16:34.214342670 +0200
> @@ -98,26 +98,8 @@
> 
> inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
> {
> -// readdir_r has been deprecated since glibc 2.24.
> -// See https://sourceware.org/bugzilla/show_bug.cgi?id=19056 for more details.
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> -
> -  dirent* p;
> -  int status;
>   assert(dirp != NULL, "just checking");
> -
> -  // NOTE: Linux readdir_r (on RH 6.2 and 7.2 at least) is NOT like the POSIX
> -  // version. Here is the doc for this function:
> -  // http://www.gnu.org/manual/glibc-2.2.3/html_node/libc_262.html
> -
> -  if((status = ::readdir_r(dirp, dbuf, &p)) != 0) {
> -    errno = status;
> -    return NULL;
> -  } else
> -    return p;
> -
> -#pragma GCC diagnostic pop
> +  return ::readdir(dirp);
> }
> 
> inline int os::closedir(DIR *dirp) {

This looks good.



More information about the build-dev mailing list