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

[Brent Christian]

> On 1/30/17 5:52 PM, Coleen Phillimore wrote:
>>
>> 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.

Thanks - makes sense.  Webrev updated in place.

>> Otherwise this seems fine.   I didn't see the update to test "On the
>> hotspot side, I had to update the 8163014 regtest. "

Ah, that's this:

http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/hotspot/test/runtime/LocalLong/LocalLongHelper.java.sdiff.html

though the main @test file, .../LocalLong/LocalLongTest.java, didn't 
need any changes.

>> Please make sure JPRT -testset hotspot works with this (or check it in
>> that way).

Yes, that runs cleanly, and I do plan to push through hs.

Thanks for having a look, Coleen.

-Brent

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