Re: RFR: 8323546: Cleanup jcmd docs for Compiler.perfmap and VM.cds filename parameter [v6]
On Wed, 17 Jan 2024 02:26:22 GMT, Chris Plummer wrote: >> The jcmd docs for Compiler.perfmap currently say: >> >> - *filename*: (Optional) The name of the map file (STRING, no default >> value) >> >> However, there is a default, so not only should that be made more clear in >> the above, but also some descriptive text as to how the default is generated >> should be added. >> >> VM.cds has a similar issue, but already has the descriptive text, so just >> the "no default value" part needs to be fixed. >> >> Another change needed is to consistently use *filename* (italics) instead of >> `filename` (monospace). Note this is how html formatting is done. For the >> man page formatting, *filename* does no formatting and `filename` is >> displayed in color if supported. Personally I prefer `filename`, but it >> seems that there is already a strong precedence for using italics in the >> *arguments* list. For example: >> >> *arguments*: >> >> - *flag name*: The name of the flag that you want to set (STRING, no >> default value) >> >> - *string value*: (Optional) The value that you want to set (STRING, no >> default value) > > Chris Plummer has updated the pull request incrementally with two additional > commits since the last revision: > > - Get rid of extra empty line. > - Remove quotes from around filename to be consistent with how jcmd STRING > defaults are printed. LGTM. I think having one test case for perfmap should be enough. - Marked as reviewed by iklam (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17359#pullrequestreview-1826632142
Re: RFR: 8323546: Clarify docs for Compiler.perfmap filename parameter, and other misc related jcmd doc cleanups [v5]
On Tue, 16 Jan 2024 23:46:02 GMT, Chris Plummer wrote: >> The jcmd docs for Compiler.perfmap currently say: >> >> - *filename*: (Optional) The name of the map file (STRING, no default >> value) >> >> However, there is a default, so not only should that be made more clear in >> the above, but also some descriptive text as to how the default is generated >> should be added. >> >> VM.cds has a similar issue, but already has the descriptive text, so just >> the "no default value" part needs to be fixed. >> >> Another change needed is to consistently use *filename* (italics) instead of >> `filename` (monospace). Note this is how html formatting is done. For the >> man page formatting, *filename* does no formatting and `filename` is >> displayed in color if supported. Personally I prefer `filename`, but it >> seems that there is already a strong precedence for using italics in the >> *arguments* list. For example: >> >> *arguments*: >> >> - *flag name*: The name of the flag that you want to set (STRING, no >> default value) >> >> - *string value*: (Optional) The value that you want to set (STRING, no >> default value) > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Fix issue with not allowing user to specify a filename that is equivalent > to the default filename descripiton. I've updated the PR with Ioi's suggestion and also added a test cast to PerfMapTest.java to make sure if the default is specified, then it actually gets used as the filename (it failed before I added Ioi's fix). Ioi, let me know if you think a similar test for VM.cds is warranted. i also updated the javadoc to not include quotes around the default filename. This is consistent with how other defaults are printed, and also how jcmd "help" prints the default argument. - PR Comment: https://git.openjdk.org/jdk/pull/17359#issuecomment-1894833574
Re: RFR: 8323546: Clarify docs for Compiler.perfmap filename parameter, and other misc related jcmd doc cleanups [v6]
> The jcmd docs for Compiler.perfmap currently say: > > - *filename*: (Optional) The name of the map file (STRING, no default > value) > > However, there is a default, so not only should that be made more clear in > the above, but also some descriptive text as to how the default is generated > should be added. > > VM.cds has a similar issue, but already has the descriptive text, so just the > "no default value" part needs to be fixed. > > Another change needed is to consistently use *filename* (italics) instead of > `filename` (monospace). Note this is how html formatting is done. For the man > page formatting, *filename* does no formatting and `filename` is displayed in > color if supported. Personally I prefer `filename`, but it seems that there > is already a strong precedence for using italics in the *arguments* list. For > example: > > *arguments*: > > - *flag name*: The name of the flag that you want to set (STRING, no > default value) > > - *string value*: (Optional) The value that you want to set (STRING, no > default value) Chris Plummer has updated the pull request incrementally with two additional commits since the last revision: - Get rid of extra empty line. - Remove quotes from around filename to be consistent with how jcmd STRING defaults are printed. - Changes: - all: https://git.openjdk.org/jdk/pull/17359/files - new: https://git.openjdk.org/jdk/pull/17359/files/7238d2d6..86fb2c37 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17359=05 - incr: https://webrevs.openjdk.org/?repo=jdk=17359=04-05 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17359.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17359/head:pull/17359 PR: https://git.openjdk.org/jdk/pull/17359
RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
Please review this PR with a simple solution for resolving duplicate `Thread` symbol issue. In https://github.com/openjdk/jdk/pull/14808 comments, there was an alternative suggestion to redefine the symbol at build time, such as using`-DThread=HotSpotThread`. That would not address issues when symbol were references as string literals. https://github.com/openjdk/jdk/pull/14808 also discussed using namespace for hotspot code, which can have multiple benefits/motivations. We could explore further using namespace with more consensus on that approach. Contributed by Chuck Rasbold and @jianglizhou. - Commit messages: - 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking Changes: https://git.openjdk.org/jdk/pull/17456/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17456=00 Issue: https://bugs.openjdk.org/browse/JDK-8311846 Stats: 10 lines in 4 files changed: 5 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17456.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17456/head:pull/17456 PR: https://git.openjdk.org/jdk/pull/17456
Re: RFR: 8323546: Clarify docs for Compiler.perfmap filename parameter, and other misc related jcmd doc cleanups [v5]
> The jcmd docs for Compiler.perfmap currently say: > > - *filename*: (Optional) The name of the map file (STRING, no default > value) > > However, there is a default, so not only should that be made more clear in > the above, but also some descriptive text as to how the default is generated > should be added. > > VM.cds has a similar issue, but already has the descriptive text, so just the > "no default value" part needs to be fixed. > > Another change needed is to consistently use *filename* (italics) instead of > `filename` (monospace). Note this is how html formatting is done. For the man > page formatting, *filename* does no formatting and `filename` is displayed in > color if supported. Personally I prefer `filename`, but it seems that there > is already a strong precedence for using italics in the *arguments* list. For > example: > > *arguments*: > > - *flag name*: The name of the flag that you want to set (STRING, no > default value) > > - *string value*: (Optional) The value that you want to set (STRING, no > default value) Chris Plummer has updated the pull request incrementally with one additional commit since the last revision: Fix issue with not allowing user to specify a filename that is equivalent to the default filename descripiton. - Changes: - all: https://git.openjdk.org/jdk/pull/17359/files - new: https://git.openjdk.org/jdk/pull/17359/files/ba64b6f2..7238d2d6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17359=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17359=03-04 Stats: 23 lines in 2 files changed: 13 ins; 2 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17359.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17359/head:pull/17359 PR: https://git.openjdk.org/jdk/pull/17359
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v4]
On Mon, 15 Jan 2024 16:03:03 GMT, Tom Rodriguez 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
> - Update
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java
>
>Co-authored-by: Andrey Turbanov
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> expStrMap = new HashMap<>();
> 57: Map> 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 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> expStrMap = new HashMap<>();
> 57: Map> 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
Re: RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case [v4]
On Mon, 15 Jan 2024 09:13:24 GMT, Joachim Kern wrote: >> In parseAllowedMask in socketTransport.c, prefixLen of mask is compared with >> a maxValue (32 for IPv4, 128 otherwise). This fails if it is larger than >> 32, because getaddrinfo seems to detect IPv4 family, if IPv6 address has set >> only some of the last 32 Bits. So we take the wrong maxValue. > > Joachim Kern has updated the pull request incrementally with one additional > commit since the last revision: > > cosmetic changes Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17374#pullrequestreview-1824662970
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v7]
On Fri, 5 Jan 2024 12:10:59 GMT, Martin Doerr wrote: > I have tried to build jextract > (https://github.com/openjdk/jextract/tree/jdk22) with LLVM > (https://github.com/llvm/llvm-project/releases/download/llvmorg-16.0.4/clang+llvm-16.0.4-powerpc64-ibm-aix-7.2.tar.xz). > I noticed that llvm mainly consists of .a files. So, I think we need to > support that for FFI compatibility with other libraries and open source > projects. Seems like this change is not sufficient for that. `clang` is compiled to `libclang.a` on AIX, but `libclang.so` on linux. I'm getting "System error: Exec format error" when trying to load `libclang.a` via `System.loadLibrary(libName);`. So the question remains: Are .a files really supposed to be dynamically loadable on AIX? If so, where is that documented? - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1894060171
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v7]
On Tue, 16 Jan 2024 08:43:34 GMT, Suchismith Roy wrote: >> src/hotspot/os/aix/os_aix.cpp line 1168: >> >>> 1166: int extension_length = 3; >>> 1167: char* file_path = NEW_C_HEAP_ARRAY(char, buffer_length + >>> extension_length + 1, mtInternal); >>> 1168: strncpy(file_path,filename, buffer_length + 1); >> >> Why not using >> `char* file_path = os::strdup (filename);` >> which would replace lines 1167+1168 >> and use the corresponding >> `os::free (file_path);` >> at the end > > Ok. Any performance advantage to using that ? No, I do not believe that it has performance advantage, but I think it is simpler to understand. - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1453249951
Re: RFR: JDK-8319382: com/sun/jdi/JdwpAllowTest.java shows failures on AIX if prefixLen of mask is larger than 32 in IPv6 case [v4]
On Mon, 15 Jan 2024 09:13:24 GMT, Joachim Kern wrote: >> In parseAllowedMask in socketTransport.c, prefixLen of mask is compared with >> a maxValue (32 for IPv4, 128 otherwise). This fails if it is larger than >> 32, because getaddrinfo seems to detect IPv4 family, if IPv6 address has set >> only some of the last 32 Bits. So we take the wrong maxValue. > > Joachim Kern has updated the pull request incrementally with one additional > commit since the last revision: > > cosmetic changes Marked as reviewed by mbaesken (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17374#pullrequestreview-1822980246
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v7]
On Wed, 20 Dec 2023 13:29:05 GMT, Joachim Kern wrote: >> Suchismith Roy has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Spaces fix > > src/hotspot/os/aix/os_aix.cpp line 1168: > >> 1166: int extension_length = 3; >> 1167: char* file_path = NEW_C_HEAP_ARRAY(char, buffer_length + >> extension_length + 1, mtInternal); >> 1168: strncpy(file_path,filename, buffer_length + 1); > > Why not using > `char* file_path = os::strdup (filename);` > which would replace lines 1167+1168 > and use the corresponding > `os::free (file_path);` > at the end Ok. Any performance advantage to using that ? - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1453094259
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v9]
> J2SE agent does not start and throws error when it tries to find the shared > library ibm_16_am. > After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load > fails.It fails to identify the shared library ibm_16_am.a shared archive file > on AIX. > Hence we are providing a function which will additionally search for .a file > on AIX ,when the search for .so file fails. Suchismith Roy has updated the pull request incrementally with three additional commits since the last revision: - Update porting_aix.cpp - Update porting_aix.cpp - Update os_aix.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/16604/files - new: https://git.openjdk.org/jdk/pull/16604/files/6a5ce4a2..212f16be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16604=08 - incr: https://webrevs.openjdk.org/?repo=jdk=16604=07-08 Stats: 6 lines in 2 files changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16604/head:pull/16604 PR: https://git.openjdk.org/jdk/pull/16604
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v8]
> J2SE agent does not start and throws error when it tries to find the shared > library ibm_16_am. > After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load > fails.It fails to identify the shared library ibm_16_am.a shared archive file > on AIX. > Hence we are providing a function which will additionally search for .a file > on AIX ,when the search for .so file fails. Suchismith Roy has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits: - Fix merge conflicts - Spaces fix - Restore lines - Remove trailing spaces. - Change return type - Change dll load function signature that does dlopen - Remove AIX macros - Add wrapper function to check extension before dlopen - merge pr/16920 - cosmetic changes - ... and 14 more: https://git.openjdk.org/jdk/compare/36f4b34f...6a5ce4a2 - Changes: https://git.openjdk.org/jdk/pull/16604/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16604=07 Stats: 28 lines in 2 files changed: 27 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16604.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16604/head:pull/16604 PR: https://git.openjdk.org/jdk/pull/16604
