Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
On Tue, 27 Feb 2024 21:46:17 GMT, Magnus Ihse Bursie wrote: >> Once [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) has been >> integrated, it is possible to do some cleanup. The goal of >> [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) was to not change >> any behavior, even if that behavior seemed odd. >> >> Now let's try to fix that. We can: >> >> a) remove JNIEXPORT from c2v functions. >> b) make debug.cpp functions exported similarly on all platforms. >> c) remove JNIEXPORT from aarch64 asm debug function. >> >> Note that this PR is [dependent >> on](https://mail.openjdk.org/pipermail/jdk-dev/2021-March/005232.html) >> https://github.com/openjdk/jdk/pull/17955. > > Magnus Ihse Bursie has updated the pull request incrementally with three > additional commits since the last revision: > > - Why did these creep back in? Merge error? > - Remove #undef JNIEXPORT > - Remove das1() Marked as reviewed by djelinski (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17967#pullrequestreview-1905367506
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
On Tue, 27 Feb 2024 21:46:17 GMT, Magnus Ihse Bursie wrote: >> Once [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) has been >> integrated, it is possible to do some cleanup. The goal of >> [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) was to not change >> any behavior, even if that behavior seemed odd. >> >> Now let's try to fix that. We can: >> >> a) remove JNIEXPORT from c2v functions. >> b) make debug.cpp functions exported similarly on all platforms. >> c) remove JNIEXPORT from aarch64 asm debug function. >> >> Note that this PR is [dependent >> on](https://mail.openjdk.org/pipermail/jdk-dev/2021-March/005232.html) >> https://github.com/openjdk/jdk/pull/17955. > > Magnus Ihse Bursie has updated the pull request incrementally with three > additional commits since the last revision: > > - Why did these creep back in? Merge error? > - Remove #undef JNIEXPORT > - Remove das1() Seems fine to me. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17967#pullrequestreview-1905265100
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
On Fri, 23 Feb 2024 12:03:37 GMT, Bernhard Urban-Forster wrote: >> Instead of discussing removal of windows/aarch64 (although the general rule >> in OpenJDK is that ports that are not maintained by anyone should be >> removed); let's stick to the question here: do we need to export `das1()` >> for debugging? > > Personally I have never used `das`/`das1` on any AArch64 port when debugging. > I guess it was somewhat handy when the backend was originally developed. I > would argue that the built-in disassembler of any debugger and `disnm` from > `debug.cpp` are good enough, and thus `das`/`das1` can be removed. What do > you think @theRealAph ? Removal makes sense - thank you @lewurm, @luhenry, and @adinn for your input! - PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1505070224
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
On Tue, 27 Feb 2024 16:11:34 GMT, Andrew Dinn wrote: >> You can probably remove the entire function given it's an indirection to >> `das` anyway. I remember using it for debugging during the first stages of >> the Windows-AArch64 port but I've long forgotten about it. > > @luhenry @magicus das1 was added when we were implementing the AArch64 port > in order to help us integrate the debugger support we provided when running > JITted code on our AArch64 simulator into gdb. Now that we have real hardware > it is redundant. Thanks @adinn for the confirmation! I've now removed it. - PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1505045060
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
> Once [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) has been > integrated, it is possible to do some cleanup. The goal of > [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) was to not change > any behavior, even if that behavior seemed odd. > > Now let's try to fix that. We can: > > a) remove JNIEXPORT from c2v functions. > b) make debug.cpp functions exported similarly on all platforms. > c) remove JNIEXPORT from aarch64 asm debug function. > > Note that this PR is [dependent > on](https://mail.openjdk.org/pipermail/jdk-dev/2021-March/005232.html) > https://github.com/openjdk/jdk/pull/17955. Magnus Ihse Bursie has updated the pull request incrementally with three additional commits since the last revision: - Why did these creep back in? Merge error? - Remove #undef JNIEXPORT - Remove das1() - Changes: - all: https://git.openjdk.org/jdk/pull/17967/files - new: https://git.openjdk.org/jdk/pull/17967/files/43c3513b..387d243b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17967=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17967=03-04 Stats: 24 lines in 4 files changed: 0 ins; 24 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17967.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17967/head:pull/17967 PR: https://git.openjdk.org/jdk/pull/17967
