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

Thu Apr 2 20:37:23 UTC 2015

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