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

[Coleen Phillimore]

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