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

Fri Aug 19 23:40:24 UTC 2016

Hi, Fred

Thanks for having a look.  Good questions...
On 08/17/2016 02:28 PM, Frederic Parain wrote:
>
> 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.

That code just checks if we potentially have two consecutive T_INTs 
representing a long.  The code that can tell the difference comes later:

   jlong nextVal = values->long_at(i)

This grabs the full 64-bits (from slot i+1) for examination.

The key difference in (2) is that the second slot can't have a value 
 >32-bits.  In (1), the second slot can contain a full 64-bit value.  I 
would break it down further into:

1a: long with 1 or more of the most significant 32 bits set
1b: long with none of the most significant 32 bits set

1a is the only case that requires adjustment.  StackWalker should return 
the same values for 1b and 2.

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

Interesting.  I've only seen zeros in my testing of locally-declared 
longs.  (How does other VM code account for unused local slots?).

Presuming an un-zeroed slot could contain anything, I can think of a 
couple problems that could occur (returning a mystery T_INT, clobbering 
a "real" int [1]).

Having only seen 0s in unused slots, I still think my suggested code is 
about as good as we can do for this experimental StackWalker feature.

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

Indeed.  This truncation happens in the call to 
create_primitive_value_instance():

  161     case T_INT:
  162       args.push_int(values->int_at(i));

StackValueCollection::int_at() truncates the value to a jint.  So it's 
not strictly necessary for the 2nd slot to be stripped as part of this fix.

Thanks,
-Brent

1.
If *any* T_INT could be an unused slot (not only those containing 0),
and all we can look for are high-order bits set in the 2nd slot, I feel 
all bets are kind of off.  A couple problems could occur:

* For situation 1b (long w/o high bits set), the unused slot would 
remain untouched, and we would return a slot containing a mystery T_INT 
value.

* In the case of a local int followed by a local long, if the long's 
first, unused slot happened to have high-order bits set, it would be 
interpreted as a 64-bit long value, and the preceding slot could have 
its (legitimate) value overwritten:

slot 0: slot for real local int
slot 1: unused slot containing garbage w/ upper bits set
slot 2: 2nd slot of a long, containing the long value

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