[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: PING: RFR: 8194249: SA: G1HeapRegionTable#getByAddress() returns incorrect HeapRegion
From:       Thomas Schatzl <thomas.schatzl () oracle ! com>
Date:       2018-01-31 8:57:12
Message-ID: 1517389032.2352.2.camel () oracle ! com
[Download RAW message or body]

Hi,

On Wed, 2018-01-31 at 09:49 +0900, Yasumasa Suenaga wrote:
> 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/sha
> re/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-1
> 5.19
> 
> Thus I think this is not a bug.

You are right, I mixed them up.

I will push the change then, with jgeorge and me as reviewers.

Thomas


[prev in list] [next in list] [prev in thread] [next in thread]