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

[Kumar Srinivasan]

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