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

[Coleen Phillimore]

Thanks Brent, looks good!
Coleen

On 1/31/17 1:19 PM, Brent Christian wrote:
>> 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
>>>>
>>>>
>>>
>>
>