RE: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
Hi Bernard, Yes, your fix is good but would be nicer if the comment in line 733 is modified as well since it might be misleading. > 733 /* On AIX, readdir() returns EBADF ... I only have Linux machines to test. But I suggest that your patch being merged soon as the deprecated use breaks the build on many latest Linux distributions, and it's hard for guys to find other POSIX platforms in a short time. -- Thanks, Pengfei -Original Message- From: B. Blaser Sent: Thursday, June 21, 2018 00:01 To: Pengfei Li Cc: kim.barr...@oracle.com; core-libs-...@openjdk.java.net; nd ; build-dev@openjdk.java.net Subject: Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated Hi Pengfei, On 20 June 2018 at 12:08, Pengfei Li wrote: > Hi > > I have tried the patch ( > http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/052991.html ) > on Ubuntu 18.04 machines (x86_64 & aarch64) with glibc=2.26.1 and build is OK. > > There's a small issue within the following code in > src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c > Unlike readdir_r(), readdir() does not return int value. The value of res is > always 0 before #ifdef. > > 727 /* EINTR not listed as a possible error */ > 728 errno = 0; > 729 ptr = readdir64(dirp); > 730 res = errno; > 731 > 732 #ifdef _AIX > 733 /* On AIX, readdir() returns EBADF (i.e. '9') and sets 'result' to > NULL for the */ > 734 /* directory stream end. Otherwise, 'errno' will contain the error > code. */ > 735 if (res != 0) { > 736 res = (ptr == NULL && res == EBADF) ? 0 : errno; > 737 } > 738 #endif > > May I know that if this core-libs change going to be merged recently, or more > platforms needs to be explored? > > -- > Thanks, > Pengfei Thanks for evaluating this patch, 'readdir()' doesn't return an 'int' value but it sets 'errno' instead which is then assigned to 'res'. So, I guess the fix is OK, or did I miss anything? I've also tested it on Linux/x86_64 but ideally, the patch should be tested on all supported POSIX platforms before being integrated. The JBS issue [1] doesn't mention any fix version, so I'm not sure if it'll be targeted to 11 or 12. But if someone is available to test it on other POSIX platforms and review it, this would be nice? Thanks, Bernard [1] https://bugs.openjdk.java.net/browse/JDK-8202794
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
Hi Pengfei, On 20 June 2018 at 12:08, Pengfei Li wrote: > Hi > > I have tried the patch ( > http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/052991.html ) > on Ubuntu 18.04 machines (x86_64 & aarch64) with glibc=2.26.1 and build is OK. > > There's a small issue within the following code in > src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c > Unlike readdir_r(), readdir() does not return int value. The value of res is > always 0 before #ifdef. > > 727 /* EINTR not listed as a possible error */ > 728 errno = 0; > 729 ptr = readdir64(dirp); > 730 res = errno; > 731 > 732 #ifdef _AIX > 733 /* On AIX, readdir() returns EBADF (i.e. '9') and sets 'result' to > NULL for the */ > 734 /* directory stream end. Otherwise, 'errno' will contain the error > code. */ > 735 if (res != 0) { > 736 res = (ptr == NULL && res == EBADF) ? 0 : errno; > 737 } > 738 #endif > > May I know that if this core-libs change going to be merged recently, or more > platforms needs to be explored? > > -- > Thanks, > Pengfei Thanks for evaluating this patch, 'readdir()' doesn't return an 'int' value but it sets 'errno' instead which is then assigned to 'res'. So, I guess the fix is OK, or did I miss anything? I've also tested it on Linux/x86_64 but ideally, the patch should be tested on all supported POSIX platforms before being integrated. The JBS issue [1] doesn't mention any fix version, so I'm not sure if it'll be targeted to 11 or 12. But if someone is available to test it on other POSIX platforms and review it, this would be nice? Thanks, Bernard [1] https://bugs.openjdk.java.net/browse/JDK-8202794
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
> On May 7, 2018, at 11:20 AM, B. Blaser wrote: > > On 7 May 2018 at 14:19, B. Blaser wrote: >> On 6 May 2018 at 18:35, B. Blaser wrote: >>> On 5 May 2018 at 22:26, Kim Barrett wrote: > On May 5, 2018, at 8:03 AM, B. Blaser wrote: > > On 4 May 2018 at 17:42, Alan Bateman wrote: >> On 04/05/2018 16:31, B. Blaser wrote: >>> >>> : >>> >>> I'll create a new JBS issue (unless you want to) since the fix is >>> mostly located in core-libs and only intended to make the build >>> complete. >>> But I'll still need someone to review and push the fix, would you be >>> interested in doing this? >>> >> I think someone needs to explore the behavior on other platforms too as >> the >> #ifndef __linux__ patches are ugly. > > Yes sure but I cannot test it elsewhere myself... and we can easily > remove them once POSIX officially deprecates 'readdir_r' or if someone > validates the fix on other systems. I'm not sure anyone has the ability to test on all supported. That doesn't (and really can't) prevent making changes across platforms to platform-specific code. It just means one needs to get help with testing. Make the change across platforms, test on those platforms one has access to, and ask here for help testing on others. >>> >>> OK, I'll provide a cross-platform fix to be evaluated on other systems too. >> >> Here it is, tier1 is successful on Linux with glibc=2.26. >> Any feedback on other systems would be very helpful. > > Some more cleanup… I’ve created https://bugs.openjdk.java.net/browse/JDK-8202794 to cover this. Suggest switching the subject line.
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 7 May 2018 at 14:19, B. Blaser wrote: > On 6 May 2018 at 18:35, B. Blaser wrote: >> On 5 May 2018 at 22:26, Kim Barrett wrote: On May 5, 2018, at 8:03 AM, B. Blaser wrote: On 4 May 2018 at 17:42, Alan Bateman wrote: > On 04/05/2018 16:31, B. Blaser wrote: >> >> : >> >> I'll create a new JBS issue (unless you want to) since the fix is >> mostly located in core-libs and only intended to make the build >> complete. >> But I'll still need someone to review and push the fix, would you be >> interested in doing this? >> > I think someone needs to explore the behavior on other platforms too as > the > #ifndef __linux__ patches are ugly. Yes sure but I cannot test it elsewhere myself... and we can easily remove them once POSIX officially deprecates 'readdir_r' or if someone validates the fix on other systems. >>> >>> I'm not sure anyone has the ability to test on all supported. That >>> doesn't (and really can't) prevent making changes across platforms to >>> platform-specific code. It just means one needs to get help with >>> testing. Make the change across platforms, test on those platforms >>> one has access to, and ask here for help testing on others. >> >> OK, I'll provide a cross-platform fix to be evaluated on other systems too. > > Here it is, tier1 is successful on Linux with glibc=2.26. > Any feedback on other systems would be very helpful. Some more cleanup... Does this look better? Thanks, Bernard diff -r e81481fea884 src/java.base/unix/native/libjava/TimeZone_md.c --- a/src/java.base/unix/native/libjava/TimeZone_md.cMon May 07 08:56:35 2018 +0200 +++ b/src/java.base/unix/native/libjava/TimeZone_md.cMon May 07 15:14:26 2018 +0200 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1999, 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -122,32 +122,18 @@ DIR *dirp = NULL; struct stat statbuf; struct dirent64 *dp = NULL; -struct dirent64 *entry = NULL; char *pathname = NULL; int fd = -1; char *dbuf = NULL; char *tz = NULL; int res; -long name_max = 0; dirp = opendir(dir); if (dirp == NULL) { return NULL; } -name_max = pathconf(dir, _PC_NAME_MAX); -// If pathconf did not work, fall back to a mimimum buffer size. -if (name_max < 1024) { -name_max = 1024; -} - -entry = (struct dirent64 *)malloc(offsetof(struct dirent64, d_name) + name_max + 1); -if (entry == NULL) { -(void) closedir(dirp); -return NULL; -} - -while (readdir64_r(dirp, entry, &dp) == 0 && dp != NULL) { +while ((dp = readdir64(dirp)) != NULL) { /* * Skip '.' and '..' (and possibly other .* files) */ @@ -214,9 +200,6 @@ pathname = NULL; } -if (entry != NULL) { -free((void *) entry); -} if (dirp != NULL) { (void) closedir(dirp); } diff -r e81481fea884 src/java.base/unix/native/libjava/UnixFileSystem_md.c --- a/src/java.base/unix/native/libjava/UnixFileSystem_md.cMon May 07 08:56:35 2018 +0200 +++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.cMon May 07 15:14:26 2018 +0200 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -312,7 +312,6 @@ { DIR *dir = NULL; struct dirent64 *ptr; -struct dirent64 *result; int len, maxlen; jobjectArray rv, old; jclass str_class; @@ -325,13 +324,6 @@ } END_PLATFORM_STRING(env, path); if (dir == NULL) return NULL; -ptr = malloc(sizeof(struct dirent64) + (PATH_MAX + 1)); -if (ptr == NULL) { -JNU_ThrowOutOfMemoryError(env, "heap allocation failed"); -closedir(dir); -return NULL; -} - /* Allocate an initial String array */ len = 0; maxlen = 16; @@ -339,7 +331,7 @@ if (rv == NULL) goto error; /* Scan the directory */ -while ((readdir64_r(dir, ptr, &result) == 0) && (result != NULL)) { +while ((ptr = readdir64(dir)) != NULL) { jstring name; if (!strcmp(ptr->d_name, ".") || !strcmp(ptr->d_name, "..")) continue; @@ -360,7 +352,6 @@ (*env)->DeleteLocalRef(env, name); } closedir(dir); -free(ptr); /* Copy the final results into an appropriately-sized array */ old = rv; @@ -375,7 +366,6 @@ error: closedir(dir); -free(ptr); return NULL; } diff -r e81481fea884 src/java.base/unix/native/libnio/fs/UnixNativeDispatc
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 6 May 2018 at 18:35, B. Blaser wrote: > On 5 May 2018 at 22:26, Kim Barrett wrote: >>> On May 5, 2018, at 8:03 AM, B. Blaser wrote: >>> >>> On 4 May 2018 at 17:42, Alan Bateman wrote: On 04/05/2018 16:31, B. Blaser wrote: > > : > > I'll create a new JBS issue (unless you want to) since the fix is > mostly located in core-libs and only intended to make the build > complete. > But I'll still need someone to review and push the fix, would you be > interested in doing this? > I think someone needs to explore the behavior on other platforms too as the #ifndef __linux__ patches are ugly. >>> >>> Yes sure but I cannot test it elsewhere myself... and we can easily >>> remove them once POSIX officially deprecates 'readdir_r' or if someone >>> validates the fix on other systems. >> >> I'm not sure anyone has the ability to test on all supported. That >> doesn't (and really can't) prevent making changes across platforms to >> platform-specific code. It just means one needs to get help with >> testing. Make the change across platforms, test on those platforms >> one has access to, and ask here for help testing on others. > > OK, I'll provide a cross-platform fix to be evaluated on other systems too. Here it is, tier1 is successful on Linux with glibc=2.26. Any feedback on other systems would be very helpful. Does this look better? Thanks, Bernard diff -r e81481fea884 src/java.base/unix/native/libjava/TimeZone_md.c --- a/src/java.base/unix/native/libjava/TimeZone_md.cMon May 07 08:56:35 2018 +0200 +++ b/src/java.base/unix/native/libjava/TimeZone_md.cMon May 07 12:38:42 2018 +0200 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1999, 2016, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1999, 2018, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -122,32 +122,18 @@ DIR *dirp = NULL; struct stat statbuf; struct dirent64 *dp = NULL; -struct dirent64 *entry = NULL; char *pathname = NULL; int fd = -1; char *dbuf = NULL; char *tz = NULL; int res; -long name_max = 0; dirp = opendir(dir); if (dirp == NULL) { return NULL; } -name_max = pathconf(dir, _PC_NAME_MAX); -// If pathconf did not work, fall back to a mimimum buffer size. -if (name_max < 1024) { -name_max = 1024; -} - -entry = (struct dirent64 *)malloc(offsetof(struct dirent64, d_name) + name_max + 1); -if (entry == NULL) { -(void) closedir(dirp); -return NULL; -} - -while (readdir64_r(dirp, entry, &dp) == 0 && dp != NULL) { +while ((dp = readdir64(dirp)) != NULL) { /* * Skip '.' and '..' (and possibly other .* files) */ @@ -214,9 +200,6 @@ pathname = NULL; } -if (entry != NULL) { -free((void *) entry); -} if (dirp != NULL) { (void) closedir(dirp); } diff -r e81481fea884 src/java.base/unix/native/libjava/UnixFileSystem_md.c --- a/src/java.base/unix/native/libjava/UnixFileSystem_md.cMon May 07 08:56:35 2018 +0200 +++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.cMon May 07 12:38:42 2018 +0200 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -312,7 +312,6 @@ { DIR *dir = NULL; struct dirent64 *ptr; -struct dirent64 *result; int len, maxlen; jobjectArray rv, old; jclass str_class; @@ -325,13 +324,6 @@ } END_PLATFORM_STRING(env, path); if (dir == NULL) return NULL; -ptr = malloc(sizeof(struct dirent64) + (PATH_MAX + 1)); -if (ptr == NULL) { -JNU_ThrowOutOfMemoryError(env, "heap allocation failed"); -closedir(dir); -return NULL; -} - /* Allocate an initial String array */ len = 0; maxlen = 16; @@ -339,7 +331,7 @@ if (rv == NULL) goto error; /* Scan the directory */ -while ((readdir64_r(dir, ptr, &result) == 0) && (result != NULL)) { +while ((ptr = readdir64(dir)) != NULL) { jstring name; if (!strcmp(ptr->d_name, ".") || !strcmp(ptr->d_name, "..")) continue; @@ -360,7 +352,6 @@ (*env)->DeleteLocalRef(env, name); } closedir(dir); -free(ptr); /* Copy the final results into an appropriately-sized array */ old = rv; @@ -375,7 +366,6 @@ error: closedir(dir); -free(ptr); return NULL; } diff -r e81481fea884 src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c --- a/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c Mon May 07 08:56:35 2018 +0200 +++ b
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 5 May 2018 at 22:26, Kim Barrett wrote: >> On May 5, 2018, at 8:03 AM, B. Blaser wrote: >> >> On 4 May 2018 at 17:42, Alan Bateman wrote: >>> On 04/05/2018 16:31, B. Blaser wrote: : I'll create a new JBS issue (unless you want to) since the fix is mostly located in core-libs and only intended to make the build complete. But I'll still need someone to review and push the fix, would you be interested in doing this? >>> I think someone needs to explore the behavior on other platforms too as the >>> #ifndef __linux__ patches are ugly. >> >> Yes sure but I cannot test it elsewhere myself... and we can easily >> remove them once POSIX officially deprecates 'readdir_r' or if someone >> validates the fix on other systems. > > I'm not sure anyone has the ability to test on all supported. That > doesn't (and really can't) prevent making changes across platforms to > platform-specific code. It just means one needs to get help with > testing. Make the change across platforms, test on those platforms > one has access to, and ask here for help testing on others. OK, I'll provide a cross-platform fix to be evaluated on other systems too. Thanks, Bernard > Waiting for a POSIX change is even more like watching paint dry than > waiting for Java releases used to be.
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
> On May 5, 2018, at 8:03 AM, B. Blaser wrote: > > On 4 May 2018 at 17:42, Alan Bateman wrote: >> On 04/05/2018 16:31, B. Blaser wrote: >>> >>> : >>> >>> I'll create a new JBS issue (unless you want to) since the fix is >>> mostly located in core-libs and only intended to make the build >>> complete. >>> But I'll still need someone to review and push the fix, would you be >>> interested in doing this? >>> >> I think someone needs to explore the behavior on other platforms too as the >> #ifndef __linux__ patches are ugly. > > Yes sure but I cannot test it elsewhere myself... and we can easily > remove them once POSIX officially deprecates 'readdir_r' or if someone > validates the fix on other systems. I'm not sure anyone has the ability to test on all supported. That doesn't (and really can't) prevent making changes across platforms to platform-specific code. It just means one needs to get help with testing. Make the change across platforms, test on those platforms one has access to, and ask here for help testing on others. Waiting for a POSIX change is even more like watching paint dry than waiting for Java releases used to be.
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 4 May 2018 at 17:42, Alan Bateman wrote: > On 04/05/2018 16:31, B. Blaser wrote: >> >> : >> >> I'll create a new JBS issue (unless you want to) since the fix is >> mostly located in core-libs and only intended to make the build >> complete. >> But I'll still need someone to review and push the fix, would you be >> interested in doing this? >> > I think someone needs to explore the behavior on other platforms too as the > #ifndef __linux__ patches are ugly. Yes sure but I cannot test it elsewhere myself... and we can easily remove them once POSIX officially deprecates 'readdir_r' or if someone validates the fix on other systems. > Is there discussion on the original > thread about this? As Kim said, JDK-8202353 summarize the situation. > -Alan I've just made a small improvement (below) using 'errno' to preserve the exact behavior when necessary, what do yo think? Bernard diff -r 1871c5d07caf src/java.base/unix/native/libjava/TimeZone_md.c --- a/src/java.base/unix/native/libjava/TimeZone_md.cFri Apr 27 15:55:29 2018 -0700 +++ b/src/java.base/unix/native/libjava/TimeZone_md.cSat May 05 12:54:56 2018 +0200 @@ -122,7 +122,9 @@ DIR *dirp = NULL; struct stat statbuf; struct dirent64 *dp = NULL; +#ifndef __linux__ struct dirent64 *entry = NULL; +#endif char *pathname = NULL; int fd = -1; char *dbuf = NULL; @@ -140,7 +142,7 @@ if (name_max < 1024) { name_max = 1024; } - +#ifndef __linux__ entry = (struct dirent64 *)malloc(offsetof(struct dirent64, d_name) + name_max + 1); if (entry == NULL) { (void) closedir(dirp); @@ -148,6 +150,9 @@ } while (readdir64_r(dirp, entry, &dp) == 0 && dp != NULL) { +#else +while ((dp = readdir64(dirp)) != NULL) { +#endif /* * Skip '.' and '..' (and possibly other .* files) */ @@ -213,10 +218,11 @@ free((void *) pathname); pathname = NULL; } - +#ifndef __linux__ if (entry != NULL) { free((void *) entry); } +#endif if (dirp != NULL) { (void) closedir(dirp); } diff -r 1871c5d07caf src/java.base/unix/native/libjava/UnixFileSystem_md.c --- a/src/java.base/unix/native/libjava/UnixFileSystem_md.cFri Apr 27 15:55:29 2018 -0700 +++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.cSat May 05 12:54:56 2018 +0200 @@ -312,7 +312,9 @@ { DIR *dir = NULL; struct dirent64 *ptr; +#ifndef __linux__ struct dirent64 *result; +#endif int len, maxlen; jobjectArray rv, old; jclass str_class; @@ -324,14 +326,14 @@ dir = opendir(path); } END_PLATFORM_STRING(env, path); if (dir == NULL) return NULL; - +#ifndef __linux__ ptr = malloc(sizeof(struct dirent64) + (PATH_MAX + 1)); if (ptr == NULL) { JNU_ThrowOutOfMemoryError(env, "heap allocation failed"); closedir(dir); return NULL; } - +#endif /* Allocate an initial String array */ len = 0; maxlen = 16; @@ -339,7 +341,11 @@ if (rv == NULL) goto error; /* Scan the directory */ +#ifndef __linux__ while ((readdir64_r(dir, ptr, &result) == 0) && (result != NULL)) { +#else +while ((ptr = readdir64(dir)) != NULL) { +#endif jstring name; if (!strcmp(ptr->d_name, ".") || !strcmp(ptr->d_name, "..")) continue; @@ -360,7 +366,9 @@ (*env)->DeleteLocalRef(env, name); } closedir(dir); +#ifndef __linux__ free(ptr); +#endif /* Copy the final results into an appropriately-sized array */ old = rv; @@ -375,7 +383,9 @@ error: closedir(dir); +#ifndef __linux__ free(ptr); +#endif return NULL; } diff -r 1871c5d07caf src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c --- a/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c Fri Apr 27 15:55:29 2018 -0700 +++ b/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c Sat May 05 12:54:56 2018 +0200 @@ -726,12 +726,19 @@ char name_extra[PATH_MAX + 1 - sizeof result->d_name]; } entry; struct dirent64* ptr = &entry.buf; +#ifndef __linux__ int res; +#endif DIR* dirp = jlong_to_ptr(value); /* EINTR not listed as a possible error */ /* TDB: reentrant version probably not required here */ +#ifndef __linux__ res = readdir64_r(dirp, ptr, &result); +#else +errno = 0; +ptr = result = readdir64(dirp); +#endif #ifdef _AIX /* On AIX, readdir_r() returns EBADF (i.e. '9') and sets 'result' to NULL for the */ @@ -741,8 +748,13 @@ } #endif +#ifndef __linux__ if (res != 0) { throwUnixException(env, res); +#else +if (errno != 0) { +throwUnixException(env, errno); +#endif return NULL; } else { if (result == NULL) { diff -r 1871c5d07caf src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c --- a/src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c Fri Apr 27 15:55:29 2018 -0700
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
> On May 4, 2018, at 11:42 AM, Alan Bateman wrote: > > On 04/05/2018 16:31, B. Blaser wrote: >> : >> >> I'll create a new JBS issue (unless you want to) since the fix is >> mostly located in core-libs and only intended to make the build >> complete. >> But I'll still need someone to review and push the fix, would you be >> interested in doing this? >> > I think someone needs to explore the behavior on other platforms too as the > #ifndef __linux__ patches are ugly. Is there discussion on the original > thread about this? > > -Alan Yes, see https://bugs.openjdk.java.net/browse/JDK-8202353
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 04/05/2018 16:31, B. Blaser wrote: : I'll create a new JBS issue (unless you want to) since the fix is mostly located in core-libs and only intended to make the build complete. But I'll still need someone to review and push the fix, would you be interested in doing this? I think someone needs to explore the behavior on other platforms too as the #ifndef __linux__ patches are ugly. Is there discussion on the original thread about this? -Alan
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 3 May 2018 at 21:43, Kim Barrett wrote: >> On May 3, 2018, at 1:52 PM, B. Blaser wrote: >> >> Hi, >> >> On 2 May 2018 at 22:40, Kim Barrett wrote: On May 2, 2018, at 5:10 AM, Michal Vala wrote: On 05/01/2018 07:59 PM, Kim Barrett wrote: >> On Apr 27, 2018, at 4:26 PM, Michal Vala wrote: >> Someone to sponsor this please? > Do you have a sponsor yet? If not, I’ll do it. No, I don't. I'd really appreciate if you sponsor it. >>> >>> Will do. I should be able to take care of it in the next day or so. >>> >> >> I've noted some similar issues at several other places which makes the >> JDK build fail on Linux with glibc = 2.26, see comments here: >> https://bugs.openjdk.java.net/browse/JDK-8202353 >> >> Here is a patch for that, does this look reasonable (tier1 seems OK)? > > I think we should move in the direction of eliminating the use of readdir_r > entirely, > either under JDK-8202353, or leave that as only the HotSpot part and have > another > RFE for the uses in java.base/unix/native. I'll create a new JBS issue (unless you want to) since the fix is mostly located in core-libs and only intended to make the build complete. But I'll still need someone to review and push the fix, would you be interested in doing this? Thanks, Bernard
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
> On May 3, 2018, at 1:52 PM, B. Blaser wrote: > > Hi, > > On 2 May 2018 at 22:40, Kim Barrett wrote: >>> On May 2, 2018, at 5:10 AM, Michal Vala wrote: >>> >>> >>> >>> On 05/01/2018 07:59 PM, Kim Barrett wrote: > On Apr 27, 2018, at 4:26 PM, Michal Vala wrote: > Someone to sponsor this please? Do you have a sponsor yet? If not, I’ll do it. >>> >>> No, I don't. I'd really appreciate if you sponsor it. >> >> Will do. I should be able to take care of it in the next day or so. >> > > I've noted some similar issues at several other places which makes the > JDK build fail on Linux with glibc = 2.26, see comments here: > https://bugs.openjdk.java.net/browse/JDK-8202353 > > Here is a patch for that, does this look reasonable (tier1 seems OK)? I think we should move in the direction of eliminating the use of readdir_r entirely, either under JDK-8202353, or leave that as only the HotSpot part and have another RFE for the uses in java.base/unix/native.
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
Hi, On 2 May 2018 at 22:40, Kim Barrett wrote: >> On May 2, 2018, at 5:10 AM, Michal Vala wrote: >> >> >> >> On 05/01/2018 07:59 PM, Kim Barrett wrote: On Apr 27, 2018, at 4:26 PM, Michal Vala wrote: Someone to sponsor this please? >>> Do you have a sponsor yet? If not, I’ll do it. >> >> No, I don't. I'd really appreciate if you sponsor it. > > Will do. I should be able to take care of it in the next day or so. > I've noted some similar issues at several other places which makes the JDK build fail on Linux with glibc = 2.26, see comments here: https://bugs.openjdk.java.net/browse/JDK-8202353 Here is a patch for that, does this look reasonable (tier1 seems OK)? Thanks, Bernard diff -r 1871c5d07caf src/java.base/unix/native/libjava/TimeZone_md.c --- a/src/java.base/unix/native/libjava/TimeZone_md.cFri Apr 27 15:55:29 2018 -0700 +++ b/src/java.base/unix/native/libjava/TimeZone_md.cThu May 03 17:59:31 2018 +0200 @@ -122,7 +122,9 @@ DIR *dirp = NULL; struct stat statbuf; struct dirent64 *dp = NULL; +#ifndef __linux__ struct dirent64 *entry = NULL; +#endif char *pathname = NULL; int fd = -1; char *dbuf = NULL; @@ -140,7 +142,7 @@ if (name_max < 1024) { name_max = 1024; } - +#ifndef __linux__ entry = (struct dirent64 *)malloc(offsetof(struct dirent64, d_name) + name_max + 1); if (entry == NULL) { (void) closedir(dirp); @@ -148,6 +150,9 @@ } while (readdir64_r(dirp, entry, &dp) == 0 && dp != NULL) { +#else +while ((dp = readdir64(dirp)) != NULL) { +#endif /* * Skip '.' and '..' (and possibly other .* files) */ @@ -213,10 +218,11 @@ free((void *) pathname); pathname = NULL; } - +#ifndef __linux__ if (entry != NULL) { free((void *) entry); } +#endif if (dirp != NULL) { (void) closedir(dirp); } diff -r 1871c5d07caf src/java.base/unix/native/libjava/UnixFileSystem_md.c --- a/src/java.base/unix/native/libjava/UnixFileSystem_md.cFri Apr 27 15:55:29 2018 -0700 +++ b/src/java.base/unix/native/libjava/UnixFileSystem_md.cThu May 03 17:59:31 2018 +0200 @@ -312,7 +312,9 @@ { DIR *dir = NULL; struct dirent64 *ptr; +#ifndef __linux__ struct dirent64 *result; +#endif int len, maxlen; jobjectArray rv, old; jclass str_class; @@ -324,14 +326,14 @@ dir = opendir(path); } END_PLATFORM_STRING(env, path); if (dir == NULL) return NULL; - +#ifndef __linux__ ptr = malloc(sizeof(struct dirent64) + (PATH_MAX + 1)); if (ptr == NULL) { JNU_ThrowOutOfMemoryError(env, "heap allocation failed"); closedir(dir); return NULL; } - +#endif /* Allocate an initial String array */ len = 0; maxlen = 16; @@ -339,7 +341,11 @@ if (rv == NULL) goto error; /* Scan the directory */ +#ifndef __linux__ while ((readdir64_r(dir, ptr, &result) == 0) && (result != NULL)) { +#else +while ((ptr = readdir64(dir)) != NULL) { +#endif jstring name; if (!strcmp(ptr->d_name, ".") || !strcmp(ptr->d_name, "..")) continue; @@ -360,7 +366,9 @@ (*env)->DeleteLocalRef(env, name); } closedir(dir); +#ifndef __linux__ free(ptr); +#endif /* Copy the final results into an appropriately-sized array */ old = rv; @@ -375,7 +383,9 @@ error: closedir(dir); +#ifndef __linux__ free(ptr); +#endif return NULL; } diff -r 1871c5d07caf src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c --- a/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c Fri Apr 27 15:55:29 2018 -0700 +++ b/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c Thu May 03 17:59:31 2018 +0200 @@ -726,12 +726,18 @@ char name_extra[PATH_MAX + 1 - sizeof result->d_name]; } entry; struct dirent64* ptr = &entry.buf; +#ifndef __linux__ int res; +#endif DIR* dirp = jlong_to_ptr(value); /* EINTR not listed as a possible error */ /* TDB: reentrant version probably not required here */ +#ifndef __linux__ res = readdir64_r(dirp, ptr, &result); +#else +ptr = result = readdir64(dirp); +#endif #ifdef _AIX /* On AIX, readdir_r() returns EBADF (i.e. '9') and sets 'result' to NULL for the */ @@ -741,10 +747,12 @@ } #endif +#ifndef __linux__ if (res != 0) { throwUnixException(env, res); return NULL; } else { +#endif if (result == NULL) { return NULL; } else { @@ -755,7 +763,9 @@ } return bytes; } +#ifndef __linux__ } +#endif } JNIEXPORT void JNICALL diff -r 1871c5d07caf src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c --- a/src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c Fri Apr 27 15:55:29 2018 -0700 +++ b/src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c Thu May 03 17:5
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
> On May 2, 2018, at 5:10 AM, Michal Vala wrote: > > > > On 05/01/2018 07:59 PM, Kim Barrett wrote: >>> On Apr 27, 2018, at 4:26 PM, Michal Vala wrote: >>> Someone to sponsor this please? >> Do you have a sponsor yet? If not, I’ll do it. > > No, I don't. I'd really appreciate if you sponsor it. Will do. I should be able to take care of it in the next day or so.
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 05/01/2018 07:59 PM, Kim Barrett wrote: On Apr 27, 2018, at 4:26 PM, Michal Vala wrote: For now, proposed patch looks like this: --- old/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 09:16:34.498343185 +0200 +++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-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? Do you have a sponsor yet? If not, I’ll do it. No, I don't. I'd really appreciate if you sponsor it. -- Michal Vala OpenJDK QE Red Hat Czech
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 26 April 2018 at 23:55, Kim Barrett wrote: snip... > > 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, I see your points and don't feel particularly strongly either way. The original version of this patch I wrote didn't include those changes either. -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Web Site: http://fuseyism.com Twitter: https://twitter.com/gnu_andrew_java PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
> On Apr 27, 2018, at 4:26 PM, Michal Vala wrote: >>> For now, proposed patch looks like this: >>> >>> --- old/src/hotspot/os/linux/os_linux.inline.hpp2018-04-20 >>> 09:16:34.498343185 +0200 >>> +++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-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? Do you have a sponsor yet? If not, I’ll do it.
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 04/27/2018 06:50 PM, Kim Barrett wrote: On Apr 27, 2018, at 4:56 AM, Michal Vala wrote: On 04/27/2018 12:55 AM, Kim Barrett wrote: On Apr 25, 2018, at 10:51 AM, Andrew Hughes wrote: On 24 April 2018 at 20:17, Kim Barrett wrote: On Apr 23, 2018, at 3:51 AM, Michal Vala 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.hpp2018-04-20 09:16:34.498343185 +0200 +++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-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
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
> On Apr 27, 2018, at 4:56 AM, Michal Vala wrote: > > > > On 04/27/2018 12:55 AM, Kim Barrett wrote: >>> On Apr 25, 2018, at 10:51 AM, Andrew Hughes wrote: >>> >>> On 24 April 2018 at 20:17, Kim Barrett wrote: > On Apr 23, 2018, at 3:51 AM, Michal Vala 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.
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
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 wrote: > > > On 04/27/2018 12:55 AM, Kim Barrett wrote: >>> >>> On Apr 25, 2018, at 10:51 AM, Andrew Hughes >>> wrote: >>> >>> On 24 April 2018 at 20:17, Kim Barrett wrote: > > On Apr 23, 2018, at 3:51 AM, Michal Vala 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.hpp2018-04-20 > 09:16:34.498343185 +0200 > +++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-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
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 04/27/2018 12:55 AM, Kim Barrett wrote: On Apr 25, 2018, at 10:51 AM, Andrew Hughes wrote: On 24 April 2018 at 20:17, Kim Barrett wrote: On Apr 23, 2018, at 3:51 AM, Michal Vala 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.hpp2018-04-20 09:16:34.498343185 +0200 +++ new/src/hotspot/os/linux/os_linux.inline.hpp2018-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
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
> On Apr 25, 2018, at 10:51 AM, Andrew Hughes wrote: > > On 24 April 2018 at 20:17, Kim Barrett wrote: >>> On Apr 23, 2018, at 3:51 AM, Michal Vala 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
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 24 April 2018 at 20:17, Kim Barrett wrote: >> On Apr 23, 2018, at 3:51 AM, Michal Vala 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. Thanks, -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Web Site: http://fuseyism.com Twitter: https://twitter.com/gnu_andrew_java PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
On 04/24/2018 09:17 PM, Kim Barrett wrote: On Apr 23, 2018, at 3:51 AM, Michal Vala 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 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 Looking a bit deeper, there might be some additional follow-on that could be done, eliminating the second argument to os::readdir. That's what was I first doing. However, I have no resources to test all OSes. I understand that this solution is not clear. However, until we remove the second argument and eventually merge PerfMemory code, it's useless to passing it on linux. That's why I did that cleanup. It can be done for all OSes under another bug id though. 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. -- Michal Vala OpenJDK QE Red Hat Czech
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
> On Apr 23, 2018, at 3:51 AM, Michal Vala 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 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 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.
Re: RFR: 8179887 - Build failure with glibc >= 2.24: error: 'int readdir_r(DIR*, dirent*, dirent**)' is deprecated
Hi Michal, This was discussed in a couple of different threads. Please also see: http://mail.openjdk.java.net/pipermail/build-dev/2018-April/021530.html Thanks, David On 23/04/2018 5:51 PM, Michal Vala 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. Thanks! [1] - http://mail.openjdk.java.net/pipermail/build-dev/2018-March/021236.html [2] - https://bugs.openjdk.java.net/browse/JDK-8179887