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

2. https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-2.html#jvms-2.6.1

On Aug 22, 2016, at 9:30 PM, Mandy Chung <mandy.chung@oracle.com> wrote:

We need to follow up this issue to understand what the interpreter and compiler do for this unused slot and whether it’s always zero out.

These slot pairs are a curse, in the same league as endian-ness. Suppose a 64-bit long x lives in L[0] and L[1]. Now suppose that the interpreter (as well it might) has adjacent 32-bit words for those locals. There are four reasonable conventions for apportioning the bits of x into L[0:1]. Call HI(x) the arithmetically high part of x, and LO(x) the other part. Also, call FST(x) the lower-addressed 32-bit component of x, when stored in memory, and SND(x) the other part. Depending on your machine's endian-ness, HI=FST or HI=SND (little-endian, x86). For portable code there are obviously four ways to pack L[0:1]. I've personally seen them all, sometimes as hard-to-find VM bugs. We're just getting started, though. Now let the interpreter generously allocate 64 bits to each local. The above four cases are still possible, but now we have 4 32-bit storage units to play with. That makes (if you do the math) 4x3=12 more theoretically possible ways to store the bits of x into the 128 bits of L[0:1]. I've not seen all 12, but there are several variations that HotSpot has used over time. Confused yet? There's more: All current HotSpot implementations grow the stack downward, which means that the address of L[0] is *higher* than L[1]. This means that the pair of storage units for L[0:1] can be viewed as a memory buffer, but the bits of L[1] come at a lower address. (Once we had a tagged-stack interpreter in which there were extra tag words between the words of L[0] and L[1], for extra fun. We got tired of that.) There's one more annoyance: The memory block located at L[0:1] must be at least 64 bits wide, but it need not be 64-bit aligned, if the size of a local slot is 32 bits. So on machines that cannot perform unaligned 64-bit access, the interpreter needs to load and store 64-bit values as 32-bit halves. But we can put that aside for now; that's a separable cost borne by 32-bit RISCs. How do we simplify this? For one thing, view all reference to HI and LO with extreme suspicion. That goes for misleadingly simple terms like "the low half of x". On Intel everybody knows that's also FST (the first memory word of x), and nods in agreement, and then when you port to SPARC (that was my job) the nods turn into glassy-eyed stares. Next, don't trust L[0] and L[1] to work like array elements. Although the bytecode interpreter refers directly to L[0] and indirectly to L[1], when storing 'x', realize that you don't know exactly how those guys are laid out in memory. The interpreter will make some local decision to avoid the obvious-in-retrospect bug of storing 64-bits to L[0] on a 32-bit machine. The decision might be to form the address of L[1] and treat *that* as the base address of a memory block. The more subtle and principled thing to do would be to form the address of the *end* of L[0] and treat that as the *end* address of a memory block. The two approaches are equivalent on 32-bit machine, but on a 64-bit machine one puts the payload only in L[1] and one only in L[0]. Meanwhile, the JIT, with its free-wheeling approach to storage allocation, will probably try its best to ignore and forget stupid L[1], allocating a right-sized register or stack slot for L[0]. Thus interpreter and JIT can have independent internal conventions for how they assign storage units to L[0:1] and how they use those units to store a 64-bit value. Those independent schemes have to be reconciled along mode change paths: C2I and I2C adapters, deoptimization, and on-stack replacement (= reoptimization). The vframe_hp code does this. A strong global convention would be best, such as always using L[0] and always storing all of x in L[0] if it fits, else SND(x) in L[0] and FST(x) in L[1]. I'm not sure (and I doubt) that we are actually that clean. Any reasonable high-level API for dealing with this stuff will do like the JIT does, and pretend that, whatever the size of L[0] is physically, it contains the whole value assigned to it, without any need to inspect L[1]. That's the best policy for virtualizing stack frames, because it aligns with the plain meaning of bytecodes like "lload0", which don't mention L[1]. The role of L[1] is to provide "slop space" for internal storage in a tiny interpreter; it has no external role. The convention used in HotSpot and the JVM verifier is to assign a special type to L[1], "Top" which means "do not look at me; I contain no bits". A virtualized API which produces a view on such an L[1] needs to return some default value (if pressed), and to indicate that the slot has no payload. HTH — John P.S. If all goes well with Valhalla, we will probably get rid of slot pairs altogether in a future version of the JVM bytecodes. They spoil generics over longs and doubles. The 32-bit implementations of JVM interpreters will have to do extra work, such as have 64-bit slot sizes for methods that work with longs or doubles, but it's worth it.

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

On Jan 26, 2017, at 5:07 PM, Brent Christian <brent.christian@oracle.com> 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/

3734 element->int_field_put(_mode_offset, value); hotspot code uses 2-space indentation stackwalk.hpp line 96 - extra line can be removed. I agree with Remi to make the MODE_* constants in LiveStackFrameInfo final and PrimitiveSlot::size() public. Thanks for the good test and I want to take another look at it and get back to you. Mandy

On Jan 27, 2017, at 8:26 AM, Mandy Chung <mandy.chung@oracle.com> wrote:

...

On Jan 26, 2017, at 5:07 PM, Brent Christian <brent.christian@oracle.com> 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/

:

Thanks for the good test and I want to take another look at it and get back to you.

I check the updated webrev and looks good. The test looks good and it’s updated with my suggested formatting that I sent to you offline. Thanks. Mandy