On Mon, 15 Jan 2024 16:03:03 GMT, Tom Rodriguez <ne...@openjdk.org> wrote:
>> The changes for JDK-8287061 didn't update the SA decoding logic and there
>> are other places where the decoding has gotten out of sync with HotSpot.
>> Some of them can't be tested because they are part of JVMCI but I've added a
>> directed test for the JDK-8287061 code and a more brute force test that
>> tries to decode everything.
>
> Tom Rodriguez has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Update
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java
>
> Co-authored-by: Andrey Turbanov <turban...@gmail.com>
> - Update
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java
>
> Co-authored-by: Andrey Turbanov <turban...@gmail.com>
I can't really review the implementation details because this requires compiler
expertise I don't have. I did review the parts that were understandable without
compiler knowledge and made a few suggestions.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line
1644:
> 1642: }
> 1643: },
> 1644: new Command("testdebuginfodecode", "testdebuginfodecode",
> false) {
This doesn't belong in clhsdb. You can do all of this directly in the test if
you launch it properly.
See for example `TestObjectAlignment.java`.
test/hotspot/jtreg/serviceability/sa/ClhsdbTestAllocationMerge.java line 58:
> 56: Map<String, List<String>> expStrMap = new HashMap<>();
> 57: Map<String, List<String>> unExpStrMap = new HashMap<>();
> 58: test.run(theApp.getPid(), cmds, expStrMap, unExpStrMap);
You can just pass `null` for the two Map args, and no need to import
java.util.HashMap or java.util.Map.
test/hotspot/jtreg/serviceability/sa/ClhsdbTestDebugInfodDecode.java line 52:
> 50: System.out.println("Started LingeredAppWithEnum with pid " +
> theApp.getPid());
> 51:
> 52: List<String> cmds = List.of("testdebuginfodecode");
This will need to change given my suggestion to instead include the contents of
clshdb command within this test.
test/hotspot/jtreg/serviceability/sa/ClhsdbTestDebugInfodDecode.java line 58:
> 56: Map<String, List<String>> expStrMap = new HashMap<>();
> 57: Map<String, List<String>> unExpStrMap = new HashMap<>();
> 58: test.run(theApp.getPid(), cmds, expStrMap, unExpStrMap);
You can just pass null for the two Map args, and no need to import
java.util.HashMap or java.util.Map.
-------------
Changes requested by cjplummer (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17407#pullrequestreview-1825478257
PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1454151509
PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1454163583
PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1454171208
PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1454166498
