Thanks Brent, looks good!
Coleen
On 1/31/17 1:19 PM, Brent Christian wrote:
On 1/30/17 5:52 PM, Coleen Phillimore wrote:
in
http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp.html
287 int mode = 0;
288 if (_jvf->is_interpreted_frame()) { mode |=
MODE_INTERPRETED; }
289 if (_jvf->is_compiled_frame()) { mode |=
MODE_COMPILED; }
The mode can't be interpreted AND compiled so the OR doesn't make
sense here. There may be other options though, so I wouldn't make
these the only cases. It should be something more like:
if (_jvf->is_interpreted_frame()) {
mode = MODE_INTERPRETED;
else if (_jvf->is_compiled_frame()) {
mode = MODE_COMPILED;
}
with no 'else' because it could be entry or runtime stub or new things
that we may eventually add, that you should probably not tell the API.
Thanks - makes sense. Webrev updated in place.
Otherwise this seems fine. I didn't see the update to test "On the
hotspot side, I had to update the 8163014 regtest. "
Ah, that's this:
http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/hotspot/test/runtime/LocalLong/LocalLongHelper.java.sdiff.html
though the main @test file, .../LocalLong/LocalLongTest.java, didn't
need any changes.
Please make sure JPRT -testset hotspot works with this (or check it in
that way).
Yes, that runs cleanly, and I do plan to push through hs.
Thanks for having a look, Coleen.
-Brent
On 1/26/17 8:07 PM, Brent Christian 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/
(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
