RFR: 8318682: SA decoding of scalar replaced objects is broken [v4]

[Chris Plummer]

On Mon, 15 Jan 2024 16:03:03 GMT, Tom Rodriguez <never at 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 <turbanoff at gmail.com>
>  - Update src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java
>    
>    Co-authored-by: Andrey Turbanov <turbanoff at 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