Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v27]
On Tue, 27 Feb 2024 07:48:00 GMT, Suchismith Roy wrote: >> 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 one additional > commit since the last revision: > >remove redundant logic Thank you everyone for your inputs, It was great learning from all of them. I understood about secure coding guidelines in practice, which was a great experience. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1965971937
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v27]
> 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 one additional commit since the last revision: remove redundant logic - Changes: - all: https://git.openjdk.org/jdk/pull/16604/files - new: https://git.openjdk.org/jdk/pull/16604/files/1943445d..57914589 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16604&range=26 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16604&range=25-26 Stats: 2 lines in 1 file changed: 0 ins; 2 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: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Mon, 26 Feb 2024 20:21:55 GMT, Magnus Ihse Bursie wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Rename LANG to LINK_TYPE Thanks for the run down on the history of the build system! I'm sorry it took me a day to respond, I must have missed this in my inbox while going over the GitHub activity of the day. Given that the function was meant for the older build system, it now seems reasonable to replace it with this newer solution. In the worst case scenario, a backout is possible, as was suggested. I would have said that LANG is an ok name and that there was no need to rename it to LINK_TYPE after the context given and the knowledge that future work was planned for it, had I read this earlier though :( This is also the first time I've ever had an objection to a Pull Request. Feels weird, really - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1965949353
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v26]
On Mon, 26 Feb 2024 11:24:13 GMT, Suchismith Roy wrote:
>> 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 one additional
> commit since the last revision:
>
>Address review comments
src/hotspot/os/aix/os_aix.cpp line 1175:
> 1173: // Shared object in .so format dont have braces, hence they get
> removed for archives with members.
> 1174: if (result == nullptr && pointer_to_dot != nullptr &&
> strcmp(pointer_to_dot, old_extension) == 0) {
> 1175: if (strcmp(pointer_to_dot, old_extension) == 0) {
Can you please remove this redundancy?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1503728978
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v26]
On Mon, 26 Feb 2024 11:24:13 GMT, Suchismith Roy wrote: >> 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 one additional > commit since the last revision: > >Address review comments Okay - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1902581556
Re: RFR: 8313710: jcmd: typo in the documentation of JFR.start and JFR.dump [v3]
On Tue, 26 Dec 2023 14:15:17 GMT, Taizo Kurashige wrote: >> Hi, >> >> I fixed the typos for JFR.start and JFR.dump. >> Acconding to issue's description, there is some typo in JFR.stop >> documentation, but I couldn't find that. I confirmed that there is no such >> typo in this repository. So I thought there was no need to fix JFR.stop >> documentation. >> >> I confirmed that the fixes are reflected and that all of the jdk_jfr tests >> pass. >> >> Could someone please review it? > > Taizo Kurashige has updated the pull request incrementally with one > additional commit since the last revision: > > 8313710: jcmd: typo in the documentation of JFR.start and JFR.dump These changes seem fine to me. Hopefully @egahlin can also approve. BTW please merge your branch with master so that it is up to date. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16413#pullrequestreview-1902559381 PR Comment: https://git.openjdk.org/jdk/pull/16413#issuecomment-1965825683
Re: RFR: 8313710: jcmd: typo in the documentation of JFR.start and JFR.dump [v3]
On Tue, 26 Dec 2023 14:15:17 GMT, Taizo Kurashige wrote: >> Hi, >> >> I fixed the typos for JFR.start and JFR.dump. >> Acconding to issue's description, there is some typo in JFR.stop >> documentation, but I couldn't find that. I confirmed that there is no such >> typo in this repository. So I thought there was no need to fix JFR.stop >> documentation. >> >> I confirmed that the fixes are reflected and that all of the jdk_jfr tests >> pass. >> >> Could someone please review it? > > Taizo Kurashige has updated the pull request incrementally with one > additional commit since the last revision: > > 8313710: jcmd: typo in the documentation of JFR.start and JFR.dump Could someone please review this PR? (Or will the review be done after https://bugs.openjdk.org/browse/JDK-8324089 is resolved?) - PR Comment: https://git.openjdk.org/jdk/pull/16413#issuecomment-1965659547
Integrated: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
On Mon, 26 Feb 2024 20:20:31 GMT, Jiangli Zhou wrote: > Please help review this trivial fix for resolving `ld: error: duplicate > symbol: closeDescriptors` when static linking with both libjdwp and libjava, > thanks. This pull request has now been integrated. Changeset: 0901dede Author:Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/0901dedefe16afa3f7222723b3fec7a22d9df675 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c Reviewed-by: cjplummer, sspitsyn - PR: https://git.openjdk.org/jdk/pull/18013
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Tue, 27 Feb 2024 00:34:49 GMT, Serguei Spitsyn wrote: >> Jiangli Zhou has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Address plummercj's comment and make forkedChildProcess static. >> - Revert src/java.base/unix/native/libjava/childproc.c and >> src/java.base/unix/native/libjava/childproc.h. Will handle those with >> JDK-8326714. > > Marked as reviewed by sspitsyn (Reviewer). Thanks for the review, @sspitsyn. - PR Comment: https://git.openjdk.org/jdk/pull/18013#issuecomment-1965631861
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Wed, 31 Jan 2024 14:22:44 GMT, Kevin Walls wrote:
> Introduce the jcmd "VM.debug" to implement access to a useful set of the
> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...".
>
> Not recommended for live production use. Calling these "debug" utilities,
> and not including them in the jcmd help output, is to remind us they are not
> general customer-facing tools.
Kevin, thank you for working on this! I've posted several comments/questions.
src/hotspot/share/services/diagnosticCommand.cpp line 1200:
> 1198: void VMDebugDCmd::find() {
> 1199: if (!_arg1.has_value()) {
> 1200: output()->print_cr("missing argument");
I'm thinking if it would be useful to tell what arguments are expected? This is
for all cases where the `"missing argument"` message is returned.
src/hotspot/share/services/diagnosticCommand.cpp line 1245:
> 1243: }
> 1244: } else if (strcmp("find", _subcommand.value()) == 0) {
> 1245: if (!UnlockDiagnosticVMOptions) {
Would it make sense to require enabling `UnlockDiagnosticVMOptions` for all
sub-commands, so that it is clear this is not for live production use?
-
PR Review: https://git.openjdk.org/jdk/pull/17655#pullrequestreview-1902349676
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1503520152
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1503518811
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Mon, 26 Feb 2024 22:55:06 GMT, Jiangli Zhou wrote: >> Please help review this trivial fix for resolving `ld: error: duplicate >> symbol: closeDescriptors` when static linking with both libjdwp and libjava, >> thanks. > > Jiangli Zhou has updated the pull request incrementally with two additional > commits since the last revision: > > - Address plummercj's comment and make forkedChildProcess static. > - Revert src/java.base/unix/native/libjava/childproc.c and > src/java.base/unix/native/libjava/childproc.h. Will handle those with > JDK-8326714. Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18013#pullrequestreview-1902299044
Re: RFR: 8326524: Rename agent_common.h
On Thu, 22 Feb 2024 19:38:26 GMT, Kim Barrett wrote: > Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/agent_common/agent_common.h to > agent_common.hpp. > > The #include updates were performed mechanically, and builds would fail if > there were mistakes. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. The only change to the renamed file is a > copyright update, since no code changes were required. > > Testing: mach5 tier1 Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17970#pullrequestreview-1902294366
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Mon, 26 Feb 2024 23:50:11 GMT, Chris Plummer wrote: > Looks good. Thanks for the quick review, @plummercj. - PR Comment: https://git.openjdk.org/jdk/pull/18013#issuecomment-1965539618
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Mon, 26 Feb 2024 22:55:06 GMT, Jiangli Zhou wrote: >> Please help review this trivial fix for resolving `ld: error: duplicate >> symbol: closeDescriptors` when static linking with both libjdwp and libjava, >> thanks. > > Jiangli Zhou has updated the pull request incrementally with two additional > commits since the last revision: > > - Address plummercj's comment and make forkedChildProcess static. > - Revert src/java.base/unix/native/libjava/childproc.c and > src/java.base/unix/native/libjava/childproc.h. Will handle those with > JDK-8326714. Looks good. - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18013#pullrequestreview-1902255565
Integrated: JDK-8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to load agent library
On Wed, 21 Feb 2024 21:13:36 GMT, Alex Menkov wrote:
> VirtualMachine.loadAgentPath/loadAgentLibrary can fail with
> AgentLoadException in 2 cases:
> - attach listener returns error; in the case the exception is thrown from
> HotSpotVirtualMachine.processCompletionStatus (called from
> HotSpotVirtualMachine.execute);
> - attach listener returns success, but reply does not contain Agent_onAttach
> return code ("return code: %d" message).
>
> before jdk21 if attach listener fails to load required library, it returned
> error (case 1)
> from jdk21 attach listener always returns success (case 2)
> Both cases are ok, but for 2nd case HotSpotVirtualMachine.loadAgentLibrary
> read only single line of the response message, so exception message didn't
> contain error details.
>
> The fix updates HotSpotVirtualMachine.loadAgentLibrary to read the whole
> response message.
> Walking through the code, fixed some other minor things in attach listener
> and attach API implementation (commented in the code)
>
> Testing:
> - test/jdk/com/sun/tools;
> - tier1,tier2,tier5-svc
This pull request has now been integrated.
Changeset: fc67c2b4
Author:Alex Menkov
URL:
https://git.openjdk.org/jdk/commit/fc67c2b4f17216d4adcc0825d0f378ae4f150025
Stats: 150 lines in 6 files changed: 109 ins; 16 del; 25 mod
8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to
load agent library
Reviewed-by: sspitsyn, cjplummer
-
PR: https://git.openjdk.org/jdk/pull/17954
Re: RFR: 8324680: Replace NULL with nullptr in JVMTI generated code
On Thu, 15 Feb 2024 08:46:43 GMT, Serguei Spitsyn wrote: > This enhancement replaces uses of NULL with nullptr in the XML-description > files for JVMTI. These are the files `hotsport/share/prims/jvmti.xml` and > `hotspot/share/prims/jvmti*.xls`. > > The following files are auto-generated from the `jvmti.xml` and `*.xsl files` > in the `build//variant-server/gensrc/jvmtifiles': `jvmti.h`, > `jvmti.html`, `jvmtiEnter.cpp`, `jvmtiEnterTrace.cpp`, `jvmtiEnv.hpp` > > This addresses a category of NULL uses that wasn't dealt with by: > [JDK-8299837](https://bugs.openjdk.org/browse/JDK-8299837). > > Testing: >- TBD: run mach5 tiers1-3 Filed new Enhancement: [8326716](https://bugs.openjdk.org/browse/JDK-8326716): JVMTI spec: clarify what nullptr means for C/C++ developers - PR Comment: https://git.openjdk.org/jdk/pull/17866#issuecomment-1965485369
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Mon, 26 Feb 2024 20:37:45 GMT, Chris Plummer wrote: >> Jiangli Zhou has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Address plummercj's comment and make forkedChildProcess static. >> - Revert src/java.base/unix/native/libjava/childproc.c and >> src/java.base/unix/native/libjava/childproc.h. Will handle those with >> JDK-8326714. > > src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 65: > >> 63: // by this function. This function returns 0 on failure >> 64: // and 1 on success. >> 65: static int > > I think you should also make forkedChildProcess static. It was added at the > same time. Updated. - PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503414683
Re: RFR: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c [v2]
On Mon, 26 Feb 2024 22:15:00 GMT, Jiangli Zhou wrote: >> src/java.base/unix/native/libjava/childproc.h line 134: >> >>> 132: int closeSafely(int fd); >>> 133: int isAsciiDigit(char c); >>> 134: int closeDescriptors(void); >> >> It seems that most of the APIs in this file should be static. I don't think >> you should selectively deal with just one of them because of the conflict. >> Since this ends up being a more involved change, and is in a different >> component than the jdwp change, it should probably have a separate PR. > > @plummercj thanks for looking into this! Sounds good to make the additional > local functions static in these files. Perhaps we can use two different FRs. > I'll make the current one to handle the ones in > src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c. Filed https://bugs.openjdk.org/browse/JDK-8326714. - PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503414392
Re: RFR: 8324680: Replace NULL with nullptr in JVMTI generated code
On Thu, 15 Feb 2024 08:46:43 GMT, Serguei Spitsyn wrote: > This enhancement replaces uses of NULL with nullptr in the XML-description > files for JVMTI. These are the files `hotsport/share/prims/jvmti.xml` and > `hotspot/share/prims/jvmti*.xls`. > > The following files are auto-generated from the `jvmti.xml` and `*.xsl files` > in the `build//variant-server/gensrc/jvmtifiles': `jvmti.h`, > `jvmti.html`, `jvmtiEnter.cpp`, `jvmtiEnterTrace.cpp`, `jvmtiEnv.hpp` > > This addresses a category of NULL uses that wasn't dealt with by: > [JDK-8299837](https://bugs.openjdk.org/browse/JDK-8299837). > > Testing: >- TBD: run mach5 tiers1-3 Now, we this section: Function Return Values JVM TI functions always return an [error code](http://100.110.26.5:8080/view/loom/job/loom-fibers-branch-build/lastSuccessfulBuild/artifact/loom/build/linux-x64/images/docs/specs/jvmti.html#ErrorSection) via the [jvmtiError](http://100.110.26.5:8080/view/loom/job/loom-fibers-branch-build/lastSuccessfulBuild/artifact/loom/build/linux-x64/images/docs/specs/jvmti.html#jvmtiError) function return value. Some functions can return additional values through pointers provided by the calling function. In some cases, JVM TI functions allocate memory that your program must explicitly deallocate. This is indicated in the individual JVM TI function descriptions. Empty lists, arrays, sequences, etc are returned as `nullptr`. In the event that the JVM TI function encounters an error (any return value other than `JVMTI_ERROR_NONE`) the values of memory referenced by argument pointers is undefined, but no memory will have been allocated and no global references will have been allocated. If the error occurs because of invalid input, no action will have occurred. The `nullptr` is mentioned here. As I understand from the Alan's and David's comments above we want to clarify what does it mean for C/C++ code. - PR Comment: https://git.openjdk.org/jdk/pull/17866#issuecomment-1965473360
Re: RFR: 8326433: Make libjdwp and libjava closeDescriptors() as static function [v2]
> Please help review this trivial fix for resolving `ld: error: duplicate > symbol: closeDescriptors` when static linking with both libjdwp and libjava, > thanks. Jiangli Zhou has updated the pull request incrementally with two additional commits since the last revision: - Address plummercj's comment and make forkedChildProcess static. - Revert src/java.base/unix/native/libjava/childproc.c and src/java.base/unix/native/libjava/childproc.h. Will handle those with JDK-8326714. - Changes: - all: https://git.openjdk.org/jdk/pull/18013/files - new: https://git.openjdk.org/jdk/pull/18013/files/bb9e2791..3816d315 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18013&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18013&range=00-01 Stats: 3 lines in 3 files changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18013/head:pull/18013 PR: https://git.openjdk.org/jdk/pull/18013
Re: RFR: 8326433: Make libjdwp and libjava closeDescriptors() as static function
On Mon, 26 Feb 2024 20:40:52 GMT, Chris Plummer wrote: >> Please help review this trivial fix for resolving `ld: error: duplicate >> symbol: closeDescriptors` when static linking with both libjdwp and libjava, >> thanks. > > src/java.base/unix/native/libjava/childproc.h line 134: > >> 132: int closeSafely(int fd); >> 133: int isAsciiDigit(char c); >> 134: int closeDescriptors(void); > > It seems that most of the APIs in this file should be static. I don't think > you should selectively deal with just one of them because of the conflict. > Since this ends up being a more involved change, and is in a different > component than the jdwp change, it should probably have a separate PR. @plummercj thanks for looking into this! Sounds good to make the additional local functions static in these files. Perhaps we can use two different FRs. I'll make the current one to handle the ones in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c. - PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503379848
Re: RFR: 8326433: Make libjdwp and libjava closeDescriptors() as static function
On Mon, 26 Feb 2024 20:20:31 GMT, Jiangli Zhou wrote: > Please help review this trivial fix for resolving `ld: error: duplicate > symbol: closeDescriptors` when static linking with both libjdwp and libjava, > thanks. src/java.base/unix/native/libjava/childproc.h line 134: > 132: int closeSafely(int fd); > 133: int isAsciiDigit(char c); > 134: int closeDescriptors(void); It seems that most of the APIs in this file should be static. I don't think you should selectively deal with just one of them because of the conflict. Since this ends up being a more involved change, and is in a different component than the jdwp change, it should probably have a separate PR. src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c line 65: > 63: // by this function. This function returns 0 on failure > 64: // and 1 on success. > 65: static int I think you should also make forkedChildProcess static. It was added at the same time. - PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503271560 PR Review Comment: https://git.openjdk.org/jdk/pull/18013#discussion_r1503268736
RFR: 8326433: Make libjdwp and libjava closeDescriptors() as static function
Please help review this trivial fix for resolving `ld: error: duplicate symbol: closeDescriptors` when static linking with both libjdwp and libjava, thanks. - Commit messages: - Make closeDescriptors() as static function in src/java.base/unix/native/libjava/childproc.c and src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c. Changes: https://git.openjdk.org/jdk/pull/18013/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18013&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326433 Stats: 3 lines in 3 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18013/head:pull/18013 PR: https://git.openjdk.org/jdk/pull/18013
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution
On Sat, 24 Feb 2024 06:04:40 GMT, Julian Waters wrote: >> The idea of setting up general "toolchains" in the native build was good, >> but it turned out that we really only need a single toolchain, with a single >> twist: if it should use CC or CPP to link. This is better described by a >> specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the >> default). >> >> There is a single exception to this rule, and that is if we want to compile >> for the build platform rather than the target platform. (This is only done >> for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD >> (or TARGET_TYPE := TARGET, the default). >> >> The final odd-case was the hack for building hsdis/bin on mingw. This can be >> resolved using direct variables to SetupNativeCompilation, instead of first >> creating a toolchain. >> >> Doing this refactoring will simplify the SetupNativeCompilation code, and >> make it much clearer if we use the C or C++ linker. > > !!! > > Not to be a party pooper, but this seems like it removes pretty some useful > functionality from the build system. What if this is needed down the line? On > the motivation of this change, TOOLCHAIN_LINK_CXX is pretty clear that the > linker to be used is g++ instead of gcc for instance, while the new LANG > parameter makes it look like SetupNativeCompilation only accepts and compiles > C++ files if C++ is passed, and only C files if the new default of C is > passed (For anyone looking in on this Pull Request who isn't familiar with > the build system, it can compile a mix of both without issue). I'm not > particularly sure this is a good idea, since a lot of flexibility seems to be > lost with this change. I don't seem to see any simplification to > SetupNativeCompilation either, maybe I'm missing something? I renamed LANG to LINK_TYPE, to not make it overly general. @TheShermanTanker Are you okay with this patch now? - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1965176131
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
> The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Rename LANG to LINK_TYPE - Changes: - all: https://git.openjdk.org/jdk/pull/17986/files - new: https://git.openjdk.org/jdk/pull/17986/files/f8a18690..5f446abd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17986&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17986&range=01-02 Stats: 27 lines in 13 files changed: 0 ins; 0 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/17986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17986/head:pull/17986 PR: https://git.openjdk.org/jdk/pull/17986
Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v26]
On Sat, 24 Feb 2024 11:22:17 GMT, Dmitry Chuyko wrote: >> Compiler Control (https://openjdk.org/jeps/165) provides method-context >> dependent control of the JVM compilers (C1 and C2). The active directive >> stack is built from the directive files passed with the >> `-XX:CompilerDirectivesFile` diagnostic command-line option and the >> Compiler.add_directives diagnostic command. It is also possible to clear all >> directives or remove the top from the stack. >> >> A matching directive will be applied at method compilation time when such >> compilation is started. If directives are added or changed, but compilation >> does not start, then the state of compiled methods doesn't correspond to the >> rules. This is not an error, and it happens in long running applications >> when directives are added or removed after compilation of methods that could >> be matched. For example, the user decides that C2 compilation needs to be >> disabled for some method due to a compiler bug, issues such a directive but >> this does not affect the application behavior. In such case, the target >> application needs to be restarted, and such an operation can have high costs >> and risks. Another goal is testing/debugging compilers. >> >> It would be convenient to optionally reconcile at least existing matching >> nmethods to the current stack of compiler directives (so bypass inlined >> methods). >> >> Natural way to eliminate the discrepancy between the result of compilation >> and the broken rule is to discard the compilation result, i.e. >> deoptimization. Prior to that we can try to re-compile the method letting >> compile broker to perform it taking new directives stack into account. >> Re-compilation helps to prevent hot methods from execution in the >> interpreter. >> >> A new flag `-r` has beed introduced for some directives related to compile >> commands: `Compiler.add_directives`, `Compiler.remove_directives`, >> `Compiler.clear_directives`. The default behavior has not changed (no flag). >> If the new flag is present, the command scans already compiled methods and >> puts methods that have any active non-default matching compiler directives >> to re-compilation if possible, otherwise marks them for deoptimization. >> There is currently no distinction which directives are found. In particular, >> this means that if there are rules for inlining into some method, it will be >> refreshed. On the other hand, if there are rules for a method and it was >> inlined, top-level methods won't be refreshed, but this can be achieved by >> having rules for them. >> >> In addition, a new diagnostic command `Compiler.replace_directives... > > Dmitry Chuyko has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 44 commits: > > - Merge branch 'openjdk:master' into compiler-directives-force-update > - Merge branch 'openjdk:master' into compiler-directives-force-update > - Merge branch 'openjdk:master' into compiler-directives-force-update > - Merge branch 'openjdk:master' into compiler-directives-force-update > - Merge branch 'openjdk:master' into compiler-directives-force-update > - Merge branch 'openjdk:master' into compiler-directives-force-update > - Merge branch 'openjdk:master' into compiler-directives-force-update > - Deopt osr, cleanups > - Merge branch 'openjdk:master' into compiler-directives-force-update > - Merge branch 'openjdk:master' into compiler-directives-force-update > - ... and 34 more: https://git.openjdk.org/jdk/compare/d10f277b...6d639ace As this is an extension of the existing compiler control mechanism. - PR Comment: https://git.openjdk.org/jdk/pull/14111#issuecomment-1965157823
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v2]
> The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. Magnus Ihse Bursie has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Reword "lib" comment to fit in better - Merge branch 'master' into remove-toolchain-define - 8326583: Remove over-generalized DefineNativeToolchain solution - Changes: https://git.openjdk.org/jdk/pull/17986/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17986&range=01 Stats: 231 lines in 14 files changed: 58 ins; 142 del; 31 mod Patch: https://git.openjdk.org/jdk/pull/17986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17986/head:pull/17986 PR: https://git.openjdk.org/jdk/pull/17986
Re: RFR: 8321971: Improve the user name detection logic in perfMemory get_user_name_slow [v3]
On Wed, 20 Dec 2023 07:29:07 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to improve the code >> in `get_user_name_slow` function, which is used to identify the target JVM >> owner's user name? This addresses >> https://bugs.openjdk.org/browse/JDK-8321971. >> >> As noted in that JBS issue, in its current form, the nested loop ends up >> iterating over the directory contents of `hsperfdata_xxx` directory and then >> for each iteration it checks if the name of the entry matches the pid. This >> iteration shouldn't be needed and instead one could look for a file named >> `` within that directory. >> >> No new test has been added, given the nature of this change. Existing tier1, >> tier2, tier3 and svc_tools tests pass with this change on Linux, Windows and >> macosx. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - remove redundant if block > - merge latest from master branch > - David's review comments - reduce if blocks and release the array outside > if block > - David's review comment - punctuation > - 8321971: Improve the user name detection logic in perfMemory > get_user_name_slow Hello Johan, thank you for that input and sorry about the delay in responding. I got side tracked with a few other things. I plan to refresh this PR and address the comments, this week. - PR Comment: https://git.openjdk.org/jdk/pull/17104#issuecomment-1964357753
Re: RFR: 8326524: Rename agent_common.h
On Thu, 22 Feb 2024 19:38:26 GMT, Kim Barrett wrote: > Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/agent_common/agent_common.h to > agent_common.hpp. > > The #include updates were performed mechanically, and builds would fail if > there were mistakes. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. The only change to the renamed file is a > copyright update, since no code changes were required. > > Testing: mach5 tier1 ok, I don't want to hold this up. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17970#pullrequestreview-1900922282
Re: RFR: 8326524: Rename agent_common.h
On Fri, 23 Feb 2024 04:29:47 GMT, Kim Barrett wrote: >> test/hotspot/jtreg/vmTestbase/nsk/jvmti/Deallocate/dealloc001/dealloc001.cpp >> line 28: >> >>> 26: #include "jvmti.h" >>> 27: #include "agent_common.hpp" >>> 28: #include "JVMTITools.h" >> >> Why don't you change all of these together? It's the same or similar >> scrolling. > > As I said in our private chat, I'll look at doing that for at least some of > the remainder. I already had this one > queued up, tested, and ready to submit the PR when you made that suggestion. > But avoiding that scrolling > is why I described how those updates were done. If you believe the > (implicit) suggestion in the PR description, > you don't need to look at them at all, and can just find the renamed file in > the file list at the left in the review > window (so much less scrolling) and go look at it. Sure, but can you do the rest at the same time since they're in mostly the same files? - PR Review Comment: https://git.openjdk.org/jdk/pull/17970#discussion_r1502640501
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v25]
On Sun, 25 Feb 2024 06:32:20 GMT, Thomas Stuefe wrote: >> Suchismith Roy has updated the pull request incrementally with one >> additional commit since the last revision: >> >>remove space > > src/hotspot/os/aix/os_aix.cpp line 1173: > >> 1171: char* const pointer_to_dot = strrchr(file_path, '.'); >> 1172: char const *old_extension = ".so"; >> 1173: char const *new_extension = ".a"; > > Suggestion: > > char* const file_path = strdup(filename); > char* const pointer_to_dot = strrchr(file_path, '.'); > const char old_extension[] = ".so"; > const char new_extension[] = ".a"; > STATIC_ASSERT(sizeof(old_exception) >= sizeof(new_exception)); > > and remove runtime-assert below @tstuefe done. Anyreason why we use [] instead of the pointer. Doesn't [] convert into *(baseaddress) ? - PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1502454196
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v26]
> 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 one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.org/jdk/pull/16604/files - new: https://git.openjdk.org/jdk/pull/16604/files/664e41a4..1943445d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16604&range=25 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16604&range=24-25 Stats: 18 lines in 1 file changed: 0 ins; 10 del; 8 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: 8326583: Remove over-generalized DefineNativeToolchain solution
On Fri, 23 Feb 2024 16:23:43 GMT, Magnus Ihse Bursie wrote: > The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. I could have chosen to name the `LANG` argument e.g. `LINKER_LANG`. If you insist, I can go back and rename it like this also. But there was a reason I chose the more general `LANG`, and that is because we have other unresolved issues regarding C vs C++ in the build. We don't handle CFLAGS vs CXXFLAGS very well, and we have several convoluted fixes (that just smells "black magic") to get the build to work. My instinct is that these are highly correlated to the choice of linker -- basically, in the old build system, there were a difference between C-libs anc C++-libs that were not properly carried over to the new build system. My intention is to continue this work by aligning flags etc with this property as well. But this is future work, so one could argue that with just this patch, the name `LANG` is overly broad. I thought it was okay, in the light of future development, and the wish to keep argument names succinct, but if you object I can rename it. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1963835032
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution
On Fri, 23 Feb 2024 16:23:43 GMT, Magnus Ihse Bursie wrote: > The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. First some general remarks. The thing about generalization is that you need to take it in right enough doses -- too much is just as problematic as too little. You can represent any program with a Turing machine that can read or write, and move a head back and forth. That is extremely general, and completely hopeless to program in. The right amount of generalization is reached when it helps you express the underlying ideas in a easy-to-understand way. If you got duplication, then it means something needs to be more generalized. But if you have a general solution that is only ever used in a single way, then you have over-generalization. Secondly, trust the VCS. Keeping code around since it might be "needed down the line" is a very bad reason. If we will need it again, we can restore it from the history. My experience is that these things practically never happens -- even if you need something similar in the future, the requirements are almost always different enough that the old system did not work anyway. And now over to more specific comments about this patch. There was a historic need for this function. When it was created, we started a new build system from the ground up to consolidate a myriad of different ways to build parts of the product. There were no good standardized toolchain, and we had a requirement to really handle different toolchains. Then during the years we have chipped away at all the odds bits and pieces, until the entire build uses (basically) the same toolchain -- the only difference is the linker argument. And, of course, the orthogonal question if we're targeting the build machine or the target machine, when cross-compiling. This is a very clear concept in the rest of the build system, but it was diffused in the toolchain profiles by making it look like we just have another "profile", like it was another version of gcc. So in this PR we replace a very general idea of a "profile" with two distinct options that we really care about -- what platform to target, and how we call the linker. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1963821493
