Re: RFR 9: 8074818: Resolve disabled warnings for libjava
On Tue, May 26, 2015 at 7:52 PM, Roger Riggs roger.ri...@oracle.com wrote: Hi, Sadly, but not entirely unexpectedly there is an anomaly in the include files: It seems that Windows does not define O_SYNC and O_DSYNC. To make up for the absence jdk/src/java.base/share/native/libjava/io_util.h conditionally defines them. There is no problem if the system include files appear first, but in the other order, fcntl.h tries to re-define it. In the recommended order, there is no issue. We should work hard to remove order dependencies in include files. I see that io_util.h includes fcntl.h, but only on BSD. Why not include it wherever it is available, (which may be all supported platforms!) before trying to define O_SYNC and D_SYNC?
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
Hi, Sadly, but not entirely unexpectedly there is an anomaly in the include files: It seems that Windows does not define O_SYNC and O_DSYNC. To make up for the absence jdk/src/java.base/share/native/libjava/io_util.h conditionally defines them. There is no problem if the system include files appear first, but in the other order, fcntl.h tries to re-define it. In the recommended order, there is no issue. Roger On 5/22/15 1:47 PM, Martin Buchholz wrote: It's a good idea to order include statements by system dependencies, jdk dependencies, implementation helpers, BUT order of include statements should never ever matter. If it does, then we have a bug that should be fixed. Every header file should be independently includable, and C files should only Include What They Use. It would be good for us to test some of that, e.g. can you compile each .h file as its own translation unit? +#include fcntl.h +#include limits.h + #include jni.h #include jni_util.h #include jlong.h @@ -32,9 +35,6 @@ #include java_io_FileInputStream.h -#include fcntl.h -#include limits.h -
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
On 2015-05-22 16:02, Roger Riggs wrote: Hi Alan, The change to make the assert about the build numbers in getVersionInfo should be a different issue. Perhaps it makes sense to do that as part of the JEP 223: New Version-String Scheme that is specific to the Oracle JDK. The current JEP-223 sandbox does indeed have a check in configure that build numbers cannot exceed 255. /Magnus Thanks, Roger On 5/22/2015 2:55 AM, Alan Bateman wrote: On 22/05/2015 01:42, Roger Riggs wrote: Oops, got the wrong host: Webrev: http://cr.openjdk.java.net/~rriggs/webrev-fix-all-warnings-8074818/ Issues: 8074818: Resolve disabled warnings for libjava 8080007: Stop ignoring warnings for libjava Thanks, Roger In JDK_GetVersionInfo0 then I wonder if we should change this assert to be a fatal error on product builds. Periodically people set the build to numbers 255 and often only see issues when they use a fastdebug build. Th rest looks okay to me. I don't particularly like the IOE_FORMAT in ProcessImpl.c but I think other areas have done similar to deal with this warning. ConcurrentPReader_md.c is being removed in another patch under review at the moment so might be gone before you push. -Alan.
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
The build change looks fine, thanks for fixing this! /Erik On 2015-05-22 02:42, Roger Riggs wrote: Oops, got the wrong host: Webrev: http://cr.openjdk.java.net/~rriggs/webrev-fix-all-warnings-8074818/ Issues: 8074818: Resolve disabled warnings for libjava 8080007: Stop ignoring warnings for libjava Thanks, Roger On 5/21/15 8:34 PM, Ivan Gerasimov wrote: Hi Roger On 22.05.2015 0:09, Roger Riggs wrote: Please review these native code and build changes to clear compilation warnings. Most are due to mixing unsigned types with signed types or providing the correct type to an invoked function. The code changes look good! Webrev: http://bussund0416.us.oracle.com/~rriggs/webrev/webrev-fix-all-warnings-8074818/ I guess, this link may not be accessible from outside Oracle. Sincerely yours, Ivan Issues: 8074818: Resolve disabled warnings for libjava 8080007: Stop ignoring warnings for libjava JPRT in progress for product and fastdebug builds on solaris, linux, macosx, windows, and embedded. Thanks, Roger
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
On 22/05/2015 01:42, Roger Riggs wrote: Oops, got the wrong host: Webrev: http://cr.openjdk.java.net/~rriggs/webrev-fix-all-warnings-8074818/ Issues: 8074818: Resolve disabled warnings for libjava 8080007: Stop ignoring warnings for libjava Thanks, Roger In JDK_GetVersionInfo0 then I wonder if we should change this assert to be a fatal error on product builds. Periodically people set the build to numbers 255 and often only see issues when they use a fastdebug build. Th rest looks okay to me. I don't particularly like the IOE_FORMAT in ProcessImpl.c but I think other areas have done similar to deal with this warning. ConcurrentPReader_md.c is being removed in another patch under review at the moment so might be gone before you push. -Alan.
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
Hi Alan, The change to make the assert about the build numbers in getVersionInfo should be a different issue. Perhaps it makes sense to do that as part of the JEP 223: New Version-String Scheme that is specific to the Oracle JDK. Thanks, Roger On 5/22/2015 2:55 AM, Alan Bateman wrote: On 22/05/2015 01:42, Roger Riggs wrote: Oops, got the wrong host: Webrev: http://cr.openjdk.java.net/~rriggs/webrev-fix-all-warnings-8074818/ Issues: 8074818: Resolve disabled warnings for libjava 8080007: Stop ignoring warnings for libjava Thanks, Roger In JDK_GetVersionInfo0 then I wonder if we should change this assert to be a fatal error on product builds. Periodically people set the build to numbers 255 and often only see issues when they use a fastdebug build. Th rest looks okay to me. I don't particularly like the IOE_FORMAT in ProcessImpl.c but I think other areas have done similar to deal with this warning. ConcurrentPReader_md.c is being removed in another patch under review at the moment so might be gone before you push. -Alan.
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
I plan to have review comments later today. On Thu, May 21, 2015 at 2:09 PM, Roger Riggs roger.ri...@oracle.com wrote: Please review these native code and build changes to clear compilation warnings. Most are due to mixing unsigned types with signed types or providing the correct type to an invoked function. Webrev: http://bussund0416.us.oracle.com/~rriggs/webrev/webrev-fix-all-warnings-8074818/ Issues: 8074818: Resolve disabled warnings for libjava 8080007: Stop ignoring warnings for libjava JPRT in progress for product and fastdebug builds on solaris, linux, macosx, windows, and embedded. Thanks, Roger
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
It's a good idea to order include statements by system dependencies, jdk dependencies, implementation helpers, BUT order of include statements should never ever matter. If it does, then we have a bug that should be fixed. Every header file should be independently includable, and C files should only Include What They Use. It would be good for us to test some of that, e.g. can you compile each .h file as its own translation unit? +#include fcntl.h +#include limits.h + #include jni.h #include jni_util.h #include jlong.h @@ -32,9 +35,6 @@ #include java_io_FileInputStream.h -#include fcntl.h -#include limits.h -
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
On May 22, 10:54am, marti...@google.com (Martin Buchholz) wrote: -- Subject: Re: RFR 9: 8074818: Resolve disabled warnings for libjava | I agree it's a good idea to increase safety by replacing calls to *printf | with calls to *nprintf, BUT when we do so we should also add debugging | assertions that the message fits into the buffer. | | -sprintf(errmsg, format, errnum, detail); | +snprintf(errmsg, fmtsize, IOE_FORMAT, errnum, detail); | | How about | | int needed = snprintf(...) | assert(needed = fmtsize); This only works if fmtsize is unsigned (which I hope it is) when snprintf returns 0. It will also produce a warning with -Wsign-compare. For safety you could do: assert((size_t)needed = fmtsize) christos
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
Hi Roger On 22.05.2015 0:09, Roger Riggs wrote: Please review these native code and build changes to clear compilation warnings. Most are due to mixing unsigned types with signed types or providing the correct type to an invoked function. The code changes look good! Webrev: http://bussund0416.us.oracle.com/~rriggs/webrev/webrev-fix-all-warnings-8074818/ I guess, this link may not be accessible from outside Oracle. Sincerely yours, Ivan Issues: 8074818: Resolve disabled warnings for libjava 8080007: Stop ignoring warnings for libjava JPRT in progress for product and fastdebug builds on solaris, linux, macosx, windows, and embedded. Thanks, Roger
Re: RFR 9: 8074818: Resolve disabled warnings for libjava
Oops, got the wrong host: Webrev: http://cr.openjdk.java.net/~rriggs/webrev-fix-all-warnings-8074818/ Issues: 8074818: Resolve disabled warnings for libjava 8080007: Stop ignoring warnings for libjava Thanks, Roger On 5/21/15 8:34 PM, Ivan Gerasimov wrote: Hi Roger On 22.05.2015 0:09, Roger Riggs wrote: Please review these native code and build changes to clear compilation warnings. Most are due to mixing unsigned types with signed types or providing the correct type to an invoked function. The code changes look good! Webrev: http://bussund0416.us.oracle.com/~rriggs/webrev/webrev-fix-all-warnings-8074818/ I guess, this link may not be accessible from outside Oracle. Sincerely yours, Ivan Issues: 8074818: Resolve disabled warnings for libjava 8080007: Stop ignoring warnings for libjava JPRT in progress for product and fastdebug builds on solaris, linux, macosx, windows, and embedded. Thanks, Roger
