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

[Michal Vala]

On 04/27/2018 06:50 PM, Kim Barrett wrote:
>> 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.
> 

Thanks!
Someone to sponsor this please?

-- 
Michal Vala
OpenJDK QE
Red Hat Czech