RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.

[Jesper Wilhelmsson]

Lindenmaier, Goetz skrev den 13/3/15 11:03:
> Hi Kim, Jesper,
>
> thanks for dealing with this.
> I made a new webrev, rebased and including your patch, Kim:
> http://cr.openjdk.java.net/~goetz/webrevs/8073315-wUnsign/webrev.07/
>
> Jesper, I don't care whether you push this webrev or one you assembled
> yourself - whatever is less trouble for you.

I'll use the patch from the webrev. Thanks for updating it!
/Jesper

>
> Best regards,
>    Goetz.
>
> -----Original Message-----
> From: Kim Barrett [mailto:kim.barrett at oracle.com]
> Sent: Thursday, March 12, 2015 11:27 PM
> To: Lindenmaier, Goetz
> Cc: Jesper Wilhelmsson; hotspot-dev Source Developers
> Subject: Re: RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.
>
> On Mar 6, 2015, at 4:36 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
>>
>> Hi Jesper,
>>
>> You offered to sponsor this change.  I think it's ok to be pushed.
>> Kim and Volker are ok with it, too, and I ran it through our nightly tests.
>> These cover quite a range of compilers and platforms.
>>
>> This is the final webrev:
>> http://cr.openjdk.java.net/~goetz/webrevs/8073315-wUnsign/webrev.06/
>>
>> I would appreciate a lot if you could sponsor this!
>
> During JPRT testing we discovered a change like the following is
> needed in order to build on 32bit x86 platforms:
>
> diff -r 47e882f3a6bc src/share/vm/runtime/virtualspace.cpp
> --- a/src/share/vm/runtime/virtualspace.cpp	Fri Mar 06 13:16:32 2015 -0500
> +++ b/src/share/vm/runtime/virtualspace.cpp	Thu Mar 12 18:13:52 2015 -0400
> @@ -519,12 +519,13 @@
>
>         // Calc address range within we try to attach (range of possible start addresses).
>         char *const highest_start = (char *)align_ptr_down(zerobased_max - size, attach_point_alignment);
> -      // SS10 and SS12u1 cannot compile "(char *)UnscaledOopHeapMax - size" on solaris sparc 32-bit:
> -      // "Cannot use int to initialize char*." Introduce aux variable.
> -      char *unscaled_end = (char *)UnscaledOopHeapMax;
> -      unscaled_end -= size;
> -      char *lowest_start = (size < UnscaledOopHeapMax) ?
> -        MAX2(unscaled_end, aligned_heap_base_min_address) : aligned_heap_base_min_address;
> +      // Need to be careful about size being guaranteed to be less
> +      // than UnscaledOopHeapMax due to type constraints.
> +      char *lowest_start = aligned_heap_base_min_address;
> +      uint64_t unscaled_end = UnscaledOopHeapMax - size;
> +      if (unscaled_end < UnscaledOopHeapMax) { // unscaled_end wrapped if size is large
> +        lowest_start = MAX2(lowest_start, (char*)unscaled_end);
> +      }
>         lowest_start  = (char *)align_ptr_up(lowest_start, attach_point_alignment);
>         try_reserve_range(highest_start, lowest_start, attach_point_alignment,
>                           aligned_heap_base_min_address, zerobased_max, size, alignment, large);
>
> The problem is that UnscaledOopHeapMax is a uint64_t with a value of
> 2^32.  On a 32bit platform, with size_t being of the same size as
> uint32_t, all possible values for "size" are < UnscaledOopHeapMax.
> And that triggers the newly enabled warning.
>
> There are many possible ways to deal with this. I could pretty easily
> be convinced to use something else.
>
> Can we get the requisite reviewers for this additional change? And
> have it added to the patch / webrev / whatever that will be handed off
> to Jesper for pushing?
>
> Also, the webrev.06 doesn't apply entirely cleanly to the current tip
> of hs-comp.
>