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

[Lindenmaier, Goetz]

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