This patch looks fine. I also welcome Kim's proposal of reverting back to readdir(). We should do this for JDK libraries too, there are still open issues (e.g. https://bugs.openjdk.java.net/browse/JDK-8166372).
Best Regards, Thomas On Fri, Apr 27, 2018 at 10:56 AM, Michal Vala <mv...@redhat.com> wrote: > > > On 04/27/2018 12:55 AM, Kim Barrett wrote: >>> >>> On Apr 25, 2018, at 10:51 AM, Andrew Hughes <gnu.and...@redhat.com> >>> wrote: >>> >>> On 24 April 2018 at 20:17, Kim Barrett <kim.barr...@oracle.com> wrote: >>>>> >>>>> On Apr 23, 2018, at 3:51 AM, Michal Vala <mv...@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
