PING: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion

[Yasumasa Suenaga]

Hi Thomas,

>>   looks good to me - however there is another (pre-existing) bug: the
>> shift in that code should be a logical shift, not an arithmetic
>> shift.
>>
>> I.e. ">>" instead of ">>>".
>>
>> I will run the patch through testing and report back in a few hours.
>> Should be okay.
>
>   is good. Do you want to fix the issue with the shift operator too
> here, or use another CR?

Thanks!

If the use of ">>>" is the bug, I want to fix it in new bug ticket.
I do not think the use of ">>>" is not a bug.

I g1BiasedArray.hpp, G1BiasedMappedArray::get_by_address() uses ">>"
operator to calculate biased_index:
  http://hg.openjdk.java.net/jdk/hs/file/ee513596f3ee/src/hotspot/share/gc/g1/g1BiasedArray.hpp#l134

idx_t is defined as size_t. So it is calculated as unsigned value.
In JLS 15.19, ">>>" is for unsigned. If we use ">>", it might remain
MSB in some cases.
  https://docs.oracle.com/javase/specs/jls/se9/html/jls-15.html#jls-15.19

Thus I think this is not a bug.


Thanks,

Yasumasa


2018-01-31 0:51 GMT+09:00 Thomas Schatzl <thomas.schatzl at oracle.com>:
> Hi all,
>
> On Tue, 2018-01-30 at 11:06 +0100, Thomas Schatzl wrote:
>> Hi,
>>
>> On Tue, 2018-01-30 at 07:00 +1000, David Holmes wrote:
>> > Added in hotspot-gc-dev. Although this is in the SA it is about the
>> > SA interaction with G1 and so likely needs someone familiar with G1
>> > to review it.
>> >
>> > David
>> >
>> > On 28/01/2018 10:41 PM, Yasumasa Suenaga wrote:
>> > > PING: Could you review it?
>> > >
>> > > > >  http://cr.openjdk.java.net/~ysuenaga/JDK-8194249/webrev.01/
>> > >
>> > > This webrev has been reviewed by Jini.
>> > > I need a Reviewer and sponsor.
>>
>>   looks good to me - however there is another (pre-existing) bug: the
>> shift in that code should be a logical shift, not an arithmetic
>> shift.
>>
>> I.e. ">>" instead of ">>>".
>>
>> I will run the patch through testing and report back in a few hours.
>> Should be okay.
>
>   is good. Do you want to fix the issue with the shift operator too
> here, or use another CR?
>
> Thanks,
>   Thomas
>