RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect (updated)

[Coleen Phillimore]

Added core-libs-dev.
Coleen

On 1/30/17 5:52 PM, Coleen Phillimore wrote:
>
> Hi Brent,
>
> I think this looks more conservative and better.
>
> in
> http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp.html 
>
>
>  287     int mode = 0;
>  288     if (_jvf->is_interpreted_frame()) { mode |= MODE_INTERPRETED; }
>  289     if (_jvf->is_compiled_frame())    { mode |= MODE_COMPILED;    }
>
> The mode can't be interpreted AND compiled so the OR doesn't make 
> sense here.  There may be other options though, so I wouldn't make 
> these the only cases.   It should be something more like:
>
>   if (_jvf->is_interpreted_frame()) {
>     mode = MODE_INTERPRETED;
>   else if (_jvf->is_compiled_frame()) {
>     mode = MODE_COMPILED;
>   }
>
> with no 'else' because it could be entry or runtime stub or new things 
> that we may eventually add, that you should probably not tell the API.
>
> Otherwise this seems fine.   I didn't see the update to test "On the 
> hotspot side, I had to update the 8163014 regtest. "
>
> Please make sure JPRT -testset hotspot works with this (or check it in 
> that way).
>
> Thanks,
> Coleen
>
> On 1/26/17 8:07 PM, Brent Christian wrote:
>> Hi,
>>
>> Please review my updated approach for fixing 8156073 ("2-slot
>> LiveStackFrame locals (long and double) are incorrect") in the 
>> experimental portion of the StackWalker API, added in JDK 9.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8156073
>> Webrev: http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/
>>
>> (For reference, the earlier RFR thread is here:[1].)
>>
>> We've concluded that we can't get enough primitive type info from the 
>> VM to use the current fine-grain *Primitive classes in 
>> LiveStackFrameInfo, so those classes have been removed for now. I've 
>> simplified down to just a PrimitiveSlot32 for 32-bit VMs, and 
>> PrimitiveSlot64 for 64-bit VMs.
>>
>> We also do not attempt to interpret a primitive's type based on the 
>> slot's contents, and instead return the slot's full contents for 
>> every primitive local.  This more accurately reflects the underlying 
>> layout of locals in the VM (versus the previous proposed 
>> LiveStackFrame.getLocals() behavior of having 64-bit behave like 
>> 32-bit by splitting long/double values into two slots).  On the 
>> 64-bit VM, this returns 64-bits even for primitive local variables 
>> smaller than 64-bits (int, etc).
>>
>> The upshot is that we now return the entire value for longs/doubles 
>> on 64-bit VMs.  (The current JDK 9 only reads the first 32-bits.)
>>
>> I also now indicate in LiveStackFrameInfo.toString() if the frame is 
>> interpreted or compiled.
>>
>> On the testing front:
>> In the interest of simplifying LiveStackFrame testing down into a 
>> single test file under jdk/test/, I chose not to add the additional 
>> test case from Fabio Tudone [2,3] (thanks, Fabio!), but instead 
>> incorporated some of those testing ideas into the existing test.  
>> Testing is a bit relaxed versus the previously proposed test case; in 
>> particular it does not enforce endianness.
>>
>> I also removed the more simplistic CountLocalSlots.java test, given 
>> that the updated LocalsAndOperands.java will now better cover testing 
>> -Xcomp.  On the hotspot side, I had to update the 8163014 regtest.
>>
>> Automated test runs have looked fine so far.
>>
>> Thanks,
>> -Brent
>>
>> 1. 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042979.html
>> 2. 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041666.html
>> 3. https://bugs.openjdk.java.net/browse/JDK-8158879
>>
>>
>