RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect
Brent Christian
brent.christian at oracle.com
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
>>
More information about the core-libs-dev
mailing list