Re: 8034856/8034857: More gcc warnings
On 2014-02-13 10:23, Alan Bateman wrote: On 13/02/2014 17:56, Mikael Vidstedt wrote: Alan, I made the change to JarFacade.c myself last week, only to then see the comment a few lines above where you added the new include. It seems to indicate that including ctype.h on Solaris/SPARC is a bad idea. I have no idea if the comment is still relevant, but that may be worth understanding first. Do you have cycles to look into it? As the code is using isspace already then it's not clear (unless there are different versions). Before pushing the changes then I ran the tests on all platforms (including Solaris) and the j.l.i tests include a number of tests exercise these manifest attributes with a non-US characters. The change in question appears to come from https://bugs.openjdk.java.net/browse/JDK-6679866, but I'm not sure the bug gives enough additional information. My speculation (and it's really just a speculation) is that it's not related to isspace per-se, but to something else which gets defined/redefined/undefined by including ctype.h. I guess it would be good to know if we have tests which cover the thing the comment is alluding to (non-ascii in Premain-Class). As an aside, the native code warnings coming from the jdk repository are really annoying so this is the reason for the drive-by fixes when I get a few minutes. I think others are doing the same. Absolutely support this work! As a matter of fact I have a couple of change in a sandbox I should send out for review. Cheers, Mikael
Re: 8034856/8034857: More gcc warnings
Alan, I made the change to JarFacade.c myself last week, only to then see the comment a few lines above where you added the new include. It seems to indicate that including ctype.h on Solaris/SPARC is a bad idea. I have no idea if the comment is still relevant, but that may be worth understanding first. Cheers, Mikael > On Feb 13, 2014, at 5:18, Alan Bateman wrote: > > > The number of native code warnings in the build is annoying so this is > another drive-by fix that eliminates a few of them in the serviceability and > security areas. The webrev with the changes is here: > > http://cr.openjdk.java.net/~alanb/8034856+8034857/webrev/ > > In the pkcs11 code the issue is the function prototypes for the throwXXX > functions aren't included. This is fixed by including pkcs11wrapper.h but > that exposes another issue with the header file includes that needed to be > fixed. > > In JarFacade the issue is that it uses isspace but doesn't include the ctype.h > > For LinuxOperatingSystem.c then there are 12 warnings related to fscanf > usages where the format specifier is %lld and the code wants to read into a > uint64_t. I've changed the format specifier to"%"SCNd64 so that it matches > uint64_t and should be okay on both 32 and 64-bit. > > Thanks, > Alan.
Re: 8034856/8034857: More gcc warnings
On 13/02/2014 17:56, Mikael Vidstedt wrote: Alan, I made the change to JarFacade.c myself last week, only to then see the comment a few lines above where you added the new include. It seems to indicate that including ctype.h on Solaris/SPARC is a bad idea. I have no idea if the comment is still relevant, but that may be worth understanding first. Do you have cycles to look into it? As the code is using isspace already then it's not clear (unless there are different versions). Before pushing the changes then I ran the tests on all platforms (including Solaris) and the j.l.i tests include a number of tests exercise these manifest attributes with a non-US characters. As an aside, the native code warnings coming from the jdk repository are really annoying so this is the reason for the drive-by fixes when I get a few minutes. I think others are doing the same. -Alan.
Re: 8034856/8034857: More gcc warnings
Looks fine to me. --Sean On 02/13/2014 08:18 AM, Alan Bateman wrote: The number of native code warnings in the build is annoying so this is another drive-by fix that eliminates a few of them in the serviceability and security areas. The webrev with the changes is here: http://cr.openjdk.java.net/~alanb/8034856+8034857/webrev/ In the pkcs11 code the issue is the function prototypes for the throwXXX functions aren't included. This is fixed by including pkcs11wrapper.h but that exposes another issue with the header file includes that needed to be fixed. In JarFacade the issue is that it uses isspace but doesn't include the ctype.h For LinuxOperatingSystem.c then there are 12 warnings related to fscanf usages where the format specifier is %lld and the code wants to read into a uint64_t. I've changed the format specifier to"%"SCNd64 so that it matches uint64_t and should be okay on both 32 and 64-bit. Thanks, Alan.
Re: 8034856/8034857: More gcc warnings
Changes look good. /Staffan On 13 feb 2014, at 14:18, Alan Bateman wrote: > > The number of native code warnings in the build is annoying so this is > another drive-by fix that eliminates a few of them in the serviceability and > security areas. The webrev with the changes is here: > > http://cr.openjdk.java.net/~alanb/8034856+8034857/webrev/ > > In the pkcs11 code the issue is the function prototypes for the throwXXX > functions aren't included. This is fixed by including pkcs11wrapper.h but > that exposes another issue with the header file includes that needed to be > fixed. > > In JarFacade the issue is that it uses isspace but doesn't include the ctype.h > > For LinuxOperatingSystem.c then there are 12 warnings related to fscanf > usages where the format specifier is %lld and the code wants to read into a > uint64_t. I've changed the format specifier to"%"SCNd64 so that it matches > uint64_t and should be okay on both 32 and 64-bit. > > Thanks, > Alan.
8034856/8034857: More gcc warnings
The number of native code warnings in the build is annoying so this is another drive-by fix that eliminates a few of them in the serviceability and security areas. The webrev with the changes is here: http://cr.openjdk.java.net/~alanb/8034856+8034857/webrev/ In the pkcs11 code the issue is the function prototypes for the throwXXX functions aren't included. This is fixed by including pkcs11wrapper.h but that exposes another issue with the header file includes that needed to be fixed. In JarFacade the issue is that it uses isspace but doesn't include the ctype.h For LinuxOperatingSystem.c then there are 12 warnings related to fscanf usages where the format specifier is %lld and the code wants to read into a uint64_t. I've changed the format specifier to"%"SCNd64 so that it matches uint64_t and should be okay on both 32 and 64-bit. Thanks, Alan.
