[9] RFR 8074840: Resolve disabled warnings for libjli and libjli_static

Wed Apr 1 17:22:44 UTC 2015

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
>>