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

[Volker Simonis]

Hi Goetz,

thanks for doing these last changes.
>From my side, the change is now ready for push.

Regards,
Volker


On Thu, Mar 5, 2015 at 11:38 AM, Lindenmaier, Goetz
<goetz.lindenmaier at sap.com> wrote:
> Hi Volker,
>
> thanks for looking at this.
>
>> - fix Copyright date in: ...
> Done.
>
>>  src/share/vm/gc_implementation/g1/ptrQueue.cpp ...
> Removed.
>
>> Wouldn't it be better to do the check ... right before the definition of ''cur_chunk_index'
> Yes, this would cover both cases. In general, I tried not to do changes affecting
> the scope of an assertion, but here it's quite obvious as the value is used the
> same way.
> I moved the test up there.
>
>> src/share/vm/memory/blockOffsetTable.cpp
> Simplified.
>
> New webrev:
> http://cr.openjdk.java.net/~goetz/webrevs/8073315-wUnsign/webrev.06/
>
> Best regards,
>   Goetz.
>
> -----Original Message-----
> From: Volker Simonis [mailto:volker.simonis at gmail.com]
> Sent: Mittwoch, 4. März 2015 17:20
> To: Lindenmaier, Goetz
> Cc: Kim Barrett; hotspot-dev Source Developers
> Subject: Re: RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.
>
> Hi Goetz, Kim,
>
> thanks a lot for working this out so far.
> I've looked at the change myself now and I thinks it is in a good state.
> I just found some minor issues:
>
> - fix Copyright date in:
>   src/share/vm/gc_implementation/g1/concurrentMark.hpp
>   src/share/vm/memory/blockOffsetTable.cpp
>
> - not sure why we need the following in:
>   src/share/vm/gc_implementation/g1/ptrQueue.cpp
>
>  196   _index = _sz;
>  197   assert(_index <= _sz, "Invariant.");
>
> ..until we are really, really paranoid that the assignment "_index =
> _sz" can fail :)
>
>
> - in:
>   src/share/vm/gc_implementation/parNew/parCardTableModRefBS.cpp
>
> we do the check:
>
> 245       assert(start_chunk_index >=
> lowest_non_clean_base_chunk_index && // Check underflow.
>
> only in one branch of the following if/else, but in the other branch
> we use 'cur_chunk_index' without check:
>
> 264     assert(lowest_non_clean[cur_chunk_index] == NULL, "Write once
> : value should be stable hereafter");
> 265     jbyte* first_card_of_cur_chunk = byte_for(chunk_mr.start());
> 266     lowest_non_clean[cur_chunk_index] = first_card_of_cur_chunk;
>
> Wouldn't it be better to do the check for 'start_chunk_index >=
> lowest_non_clean_base_chunk_index' right before the definition of
> 'cur_chunk_index' like so:
>
> 198   uintptr_t start_chunk_index = addr_to_chunk_index(chunk_mr.start());
>         assert(start_chunk_index >= lowest_non_clean_base_chunk_index,
> "Bounds error."); // Check underflow.
> 199   uintptr_t cur_chunk_index   = start_chunk_index -
> lowest_non_clean_base_chunk_index;
>
>
> - the change in:
>   src/share/vm/memory/blockOffsetTable.cpp
>
>  795   size_t next = _next_offset_index;
>  796   return next == 0 ? 0 : next - 1;
>
> could be simplified into just one line:
>
>  796   return _next_offset_index == 0 ? 0 : _next_offset_index - 1;
>
> Thanks,
> Volker
>
>
> On Wed, Mar 4, 2015 at 9:10 AM, Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com> wrote:
>> Thanks a lot for looking into this!
>>
>> Best regards,
>>   Goetz.
>>
>> -----Original Message-----
>> From: Kim Barrett [mailto:kim.barrett at oracle.com]
>> Sent: Dienstag, 3. März 2015 22:30
>> To: Lindenmaier, Goetz
>> Cc: hotspot-dev Source Developers
>> Subject: Re: RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.
>>
>> On Mar 3, 2015, at 9:46 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
>>>
>>> Hi Kim,
>>>
>>> as I understand the code, > 0 should hold for both tests.
>>> But I think what is really meant by the check, protecting against underflow,
>>> should be checked in decrement():
>>>
>>> --- //bas2/sapjvm/dev/6/hotspot/src/share/vm/gc_implementation/g1/heapRegionSet.hpp     2015-03-03 09:00:44.000000000 0100
>>> +++ /sapmnt/home/d045726/src3/sapjvm/dev/6/hotspot/src/share/vm/gc_implementation/g1/heapRegionSet.hpp  2015-03-03 09:00:44.000000000 0100
>>> @@ -67,6 +67,9 @@
>>>   }
>>>
>>>   void decrement(const uint length_to_remove, const size_t capacity_to_remove) {
>>> +#ifdef HEAP_REGION_SET_FORCE_VERIFY
>>> +    guarantee(length_to_remove <= _length && capacity_to_remove <= _capacity, "underflow");
>>> +#endif
>>>     _length -= length_to_remove;
>>>     _capacity -= capacity_to_remove;
>>>   }
>>>
>>> Or by casting _length and _capacity to singed types in the guarantee in verify(), which is
>>> quite ugly.  But I think it would work fine by the values one can expect here.
>>>
>>> I changed verify() to check for '>', but didn't add above guarantee, I think that again
>>> is out of the scope of this change.
>>> http://cr.openjdk.java.net/~goetz/webrevs/8073315-wUnsign/webrev.05/
>>>
>>> I ran a row of tests with the adapted assertion, and will push it into our nightbuild.
>>
>> I think you are right that the check is really for underflow, and that should be in decrement()
>> as you suggest.  And I'm fine with the way you've left it.  Anything further can be a later cleanup
>> if desired.
>>
>> So I'm done reviewing.  Looks good to me.
>>