CoreLibraries.gmk: glad you caught the ergo_i586.c.
That should also address: JDK-8074547
Looks good and Thanks!
Kumar
On 4/1/2015 10: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
* 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.
Also, some comments below.
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
