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

[Frederic Parain]

Brent,

hotspot/src/share/vm/prims/stackwalk.cpp:
  230     if (!expressions &&
  231         values->at(i)->type() == T_INT &&
  232         values->int_at(i) == 0 && // empty first slot of long?
  233         i+1 < values->size() &&
  234         values->at(i+1)->type() == T_INT) {

How does this test make the difference between:
   1 - a local variable of type long
   2 - two consecutive local variables of type int with the value
       of the first one being zero

(1) requires the fix, but (2) does not.

Another question: is it guaranteed that an unused slot from
a pair-slot is always zero? This is not written in the JVM spec,
and I don't remember that the JVM zeroes unused slots.

A last question: the fix tries to provide a view of the local
variables that matches the JVM spec rather than the implementation,
so if half of the long or double value is put in the first slot,
why the second slot is not stripped to only store the other half?

Regards,

Fred

On 08/17/2016 04:05 PM, Brent Christian wrote:
> Hi,
>
> Please review this StackWalker fix in hotspot for 8156073, "2-slot
> LiveStackFrame locals (long and double) are incorrect"
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8156073
> Webrev: http://cr.openjdk.java.net/~bchristi/8156073/webrev.01/
>
> The experimental LiveStackFrame[1] interface for StackWalker provides
> access to stack frames' local variables, returned in an Object[] from
> LiveStackFrame.getLocals().
>
> Long and double primitives are returned using two array slots (similar
> to JVMS section 2.6.1 [2]).  This works as indicated on 32-bit JDKs, but
> needs some fixing on 64-bit.
>
> AFAICT under 64-bit, StackValueCollection stores the entire
> long/double in the 2nd (64-bit) slot:
>
> jlong StackValueCollection::long_at(int slot) const {
> #ifdef _LP64
>   return at(slot+1)->get_int();
> #else
> ...
>
> The first slot goes unused and contains 0, instead of holding half of
> the long value as stackwalker expects.  So stackwalker needs to take
> care splitting the value between the two slots.
>
> The fix examines StackValueCollection::long_at(i), checks for an
> unexpected 0 value from StackValueCollection::int_at(i), and copies
> the needed 32-bits (depending on native byte ordering) into the first
> slot as needed.  (The 2nd slot gets truncated to 32-bits in
> create_primitive_value_instance()).
>
> Here's the fix in action on linux_x64 with the updated
> LocalsAndOperands.java test.  Slots 4+5 contain a long, 6+7 contain a
> double.
>
> Before the fix:
> ...
>   local 3: himom type java.lang.String
>   local 4: 0 type I          <--
>   local 5: 65535 type I
>   local 6: 0 type I          <--
>   local 7: 1293080650 type I
>   local 8: 0 type I
>
> After the fix:
> ...
>   local 3: himom type java.lang.String
>   local 4: 1023 type I       <--
>   local 5: 65535 type I
>   local 6: 1074340347 type I <--
>   local 7: 1293080650 type I
>   local 8: 0 type I
>
> This change also fixes 8158879, "Add new test for verifying 2-slot
> locals in LiveStackFrame".  Thanks to Fabio Tudone
> (fabio at paralleluniverse.co) for the test case.  I only test unused
> ("dead") local variables with -Xint, because they are removed in
> compiled frames.
>
> An automated build & test run on all platforms looks good.
>
> It would be good to also do more testing of LiveStackFrame.getStack(),
> but right now I want to focus on getting this getLocals() fix in.
> Testing of LiveStackFrame.getStack() will happen in a follow-up issue
> (8163825).
>
> Thanks,
> -Brent
>
> 1.
> http://hg.openjdk.java.net/jdk9/hs/jdk/file/1efce54b06b7/src/java.base/share/classes/java/lang/LiveStackFrame.java
>
> 2.
> https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-2.html#jvms-2.6.1
>