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
(fa...@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
