David/Kumar - thanks a lot for the reviews! I added a comment for the
unsigned cast and pushed to jdk9/dev.
Cheers,
Mikael
On 2015-04-01 16:17, David Holmes wrote:
On 2/04/2015 3:22 AM, Mikael Vidstedt wrote:
New webrev available here:
http://cr.openjdk.java.net/~mikael/webrevs/8074840/webrev.02/webrev/
Changes include:
* Excluded ergo_i586.c in the makefile on macosx instead of using
ifdef-style exclusion
Good.
* Updated to fix a newly introduced (currently silenced) warning in
parse_manifest.c:readAt(), introduced by the change [1] for
JDK-8073158 [2]
The change to have readAt take an unsigned int is not exactly pretty, so
I'm again taking suggestions on alternative solutions. The underlying
reason for making that change is again because read on Windows uses an
unsigned int for count instead of the more common size_t. I played
around with several different ways of working around this (I even
introduced a JLI_Read at some point) but after chatting with mr. Holmes
I decided to cut back on the changes and instead just change the
prototype of readAt to also take an unsigned int in the spirit of
"common denominator". All the current uses of readAt() can AFAICT
tolerate this, and if they don't the problem is already there on
windows. Happy to hear your thoughts though.
I think what you have is the simplest correct fix. There is still size
confusion in parse_manifest with the jlong use of len/flen. It would
probably be clearer to make len uint as it is constrained to a small
value due to END_MAXLEN and ENDRD, but that would likely introduce
more casts. Maybe a comment (or assert?) that len always fits in uint?
Otherwise all good.
Also, some comments below.
I missed the fact that KB introduces unsignedness (is that a word? ;-) )
Thanks,
David
On 2015-03-29 21:07, David Holmes wrote:
Hi Mikael,
On 21/03/2015 4:02 AM, Mikael Vidstedt wrote:
Please review the following change which fixes a number of native
compilation warnings in files related to libjli.
Bug: https://bugs.openjdk.java.net/browse/JDK-8074840
Webrev:
http://cr.openjdk.java.net/~mikael/webrevs/8074840/webrev.01/webrev/
Mostly looks okay, but I find some of these odd. For example:
src/java.base/share/native/libjli/java.c
if (threadStackSize < STACK_SIZE_MINIMUM)
becomes:
if (threadStackSize < (jlong)STACK_SIZE_MINIMUM) {
but integer promotion would make that cast implicitly. And why should
the comparison cause a warning when the subsequent assignment does
not? What was the actual warning and on what compiler?
Real warning - gcc on linux among others:
jdk/src/java.base/share/native/libjli/java.c:792:33: warning: comparison
between signed and unsigned integer expressions [-Wsign-compare]
if (threadStackSize < STACK_SIZE_MINIMUM) {
^
As you can see the warning is generated because of the unsigned vs
signed-ness in combination with a compare; the assignment does not
generate a corresponding warning. The unsigned type comes from the fact
that STACK_SIZE_MINIMUM which is based on the KB macro which in turn is
an unsigned long (UL) constant. One alternative would of course be to
cast the (macro) definition of STACK_SIZE_MINIMUM instead, or not use KB
to start with, but I'm not sure that's any better than having the cast
here. I really don't have a strong opinion though.
---
I second Kumar's query about the ergo_i586.c changes. Seems to me that
if this is causing a problem on non Solaris/Linux then the problem is
in the makefile (jdk/lib/CoreLibraries.gmk) because this file should
be being excluded on platforms it is not used.
Indeed - fixed!
---
src/java.base/windows/native/libjli/cmdtoargs.c
Surely charLength should be a ptrdiff_t ?
Well aren't we picky? Fixed :)
---
src/java.base/windows/native/libjli/java_md.c
Aside: I find it quite bizarre that the vsnprintf family of methods
take a size_t as the count but return an int, when on success it has
to return the number of characters written!
Fully agree. Maybe _today_ is a good day to change the return type of
all printf functions? ;)
Cheers,
Mikael
[1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/cb94a46ab47b
[2] https://bugs.openjdk.java.net/browse/JDK-8073158
---
Thanks,
David
I built the code across all the OracleJDK platforms and there were no
warnings on any of the platforms (in the files related to this
change).
I'm taking suggestions on specific tests to run.
Some specifics:
Unfortunately there is no intrinsic for cpuid in Solaris Studio, so I
had to keep the inline asm code and the corresponding flag to disable
the related warning (E_ASM_DISABLES_OPTIMIZATION). The alternative
would
be to move it out into a dedicated assembly file, but that seems like
more trouble than it's worth given that the warning is harmless.
I'm not entirely happy with the unsigned cast in parse_manifest.c:196,
but unfortunately the windows prototype for read() takes an
unsigned as
its last argument, where All(tm) other platforms take a size_t. In
this
specific case 'len' will never be be more than END_MAXLEN = 0xFFFF +
ENDHDR = 0xFFFF + 22 = 0x10015, meaning it will comfortably fit in an
unsigned on the platforms we support. But if somebody has suggestions
I'm all ears, doing the #ifdef dance is of course also an option.
Note that the warnings were temporarily disabled as part of
JDK-8074096
[1] until such time they could be fixed the proper way. Also note that
this change supersedes the earlier change [2] Dmitry had out for
review.
The bug [3] corresponding to that webrev will be closed as a duplicate
of this bug (JDK-8074839).
Cheers,
Mikael
[1] https://bugs.openjdk.java.net/browse/JDK-8074096
[2] http://cr.openjdk.java.net/~dsamersoff/JDK-8073175/webrev.01/
[3] https://bugs.openjdk.java.net/browse/JDK-8073175
