RFR (XXS): 8016474: Crash in sun.reflect.UnsafeObjectFieldAccessorImpl.get

[Igor Veresov]

On Jul 25, 2013, at 1:29 AM, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:

> Igor,
> 
> On 07/25/2013 07:46 AM, Igor Veresov wrote:
>> I haven't looked at it after the perm gen removal but shouldn't all klasses be of type T_METADATA or something?
>> May be the code wasn't updated since then?
> 
> The current implementation puts the Klasses at the beginning of the Java heap, so we should use the exact same method to decompress the compressed klass pointer as we do for compressed oops.
> 
> Harold is in the process of changing that but that change is not yet integrated.

But the bug in question is there because you can specify different alignments for objects and klasses.  Which implies that even now the decoding is different, right? Same bases but different shifts.
  
> 
>> 
>>  The code dereferences src_klass (line 2301), which is of type T_OBJECT. The value of src_klass is read from memory of type T_ADDRESS. The result of the move seems to have the type of the first argument, so it would seem that if it's T_ADDRESS no decompression is going to happen and the deference is going to be problematic. Or does it work differently?
> 
> I agree that this seems like a strange fix.
> 
> I've tried to run the regression test to provoke a crash by forcing a heap base by setting -XX:ObjectAlignmentInBytes=32 -Xmx31g but I can't even get the VM to hit the path that Christian is changing.
> 
> Running with +PrintNMethods I get:
> 
> $ build/linux/linux_amd64_compiler2/debug/hotspot -cp build -Xbatch -XX:ObjectAlignmentInBytes=32 -Xmx31g -XX:+PrintCompressedOopsMode -XX:+PrintCompilation -XX:+PrintNMethods GetUnsafeObjectG1PreBarrier
> 
> heap address: 0x0000000800000000, size: 31744 MB, Compressed Oops with base: 0x00000007f9bff000
> 
>    411    1    b        GetUnsafeObjectG1PreBarrier::readField (10 bytes)
> Compiled method (c2)     438    1 GetUnsafeObjectG1PreBarrier::readField (10 bytes)
> 
> I tried setting -XX:TieredStopAtLevel=1 but it's stil compiled by C2.

You have to turn tiered on for this to work (since it's currently off by default).

> 
> Another problem is that C2 seems to remove the call to unsafe.getObject, so this test doesn't really do anything.
> 

Well, the problem is in C1, so I guess that's ok.

igor


> 
>> 
>> igor
>> 
>> On Jul 24, 2013, at 2:37 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>> 
>>> http://cr.openjdk.java.net/~twisti/8016474
>>> 
>>> 8016474: Crash in sun.reflect.UnsafeObjectFieldAccessorImpl.get
>>> Summary: C1's GetUnsafeObject G1 pre-barrier uses the wrong type to read the klass pointer.
>>> Reviewed-by:
>>> 
>>> There is a bug in C1's GetUnsafeObject G1 pre-barrier code.  If UseCompressedKlassPointers is on we use T_OBJECT to read the klass pointer of the object.  If we also use a different object alignment like 16 or 32 (-XX:ObjectAlignmentInBytes=16) the klass pointer gets decoded with the wrong shift resulting in a wrong pointer and a crash.
> 
> But in the current implementation compressed klass pointers must always be decoded with the same shift as compressed oops.
> Harold's fix for JDK-8003424 is changing that but currently that is the case.
> 
> /Mikael
> 
>>> 
>>> The fix is to always use T_ADDRESS for klass pointer reads.
>>> 
>>> src/share/vm/c1/c1_LIRGenerator.cpp
>>> test/compiler/unsafe/GetUnsafeObjectG1PreBarrier.java
>>> 
>> 
>