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

Wed Apr 1 23:17:47 UTC 2015

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