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

[Michal Vala]

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) {



-- 
Michal Vala
OpenJDK QE
Red Hat Czech