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
