Re: RFR: 8327645: Serial heap dump should not consume double amount of disk space [v2]
On Fri, 8 Mar 2024 02:35:08 GMT, Man Cao wrote: >> Hi all, >> >> Could anyone review this fix to make serial heap dump only write to a single >> file? >> We highly appreciate the work in >> https://bugs.openjdk.org/browse/JDK-8306441. However, many of our customers >> still need to use serial heap dump, as they have limited disk space and need >> to dump to a network socket. >> >> Tested: >> Stress tested with a fastdebug build with tests in >> `hotspot/jtreg/serviceability/dcmd/gc/HeapDump*`. >> >> -Man > > Man Cao has updated the pull request incrementally with one additional commit > since the last revision: > > Fix failure under -XX:+UseSerialGC The change seems reasonable and avoids some of the extra file overhead in the serial case. - PR Review: https://git.openjdk.org/jdk/pull/18160#pullrequestreview-1923942056
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: 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 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: 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=18013=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18013=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
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=18013=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: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Wed, 17 Jan 2024 00:14:58 GMT, Jiangli Zhou wrote: > 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. We (@AlanBateman, @cushon, @magicus, @jerboaa, @pron, @jianglizhou) discussed this topic via zoom as part of a regular static/hermetic Java discussions. The outcome favors the partial-linking/objcopy to localize symbols for hotspot. Here is a summary: - A general solution is preferred compared to resolving symbol issues case by case. - We can address this for unix-like platforms with toolings supporting partial-linking/objcopy for now. @magicus will provide additional information on supported gcc versions and considerations for Windows support. - There is also a preference to localize symbols automatically without editing the symbol list manually. In our prototype for handling freetype symbols (as mentioned in https://github.com/openjdk/jdk/pull/14808#issuecomment-1631611220), @cjmoon1 looked into using `nm` to generate symbol list and feed that into `objcopy`. That might be do-able for hotspot symbols. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1917753387
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Mon, 29 Jan 2024 09:42:20 GMT, Andrew Haley wrote: > > Maybe we could live with symbol redefinition using #define (conditionally > > for static linking in OpenJDK, as Coleen suggested earlier) for now, until > > the tooling can support symbol localizing better. Then localizing symbols > > using tools like `objcopy` can be the longer term and cleaner solution, > > instead of using namespace. What's your thoughts on that? > > I suppose so, but why? > > Why should any of this have to work on old systems? If their binutils is > broken, static linking of openjdk won't work there. We ran into issues with older gcc on linux-aarch for partial linking, but the problem may not be older gcc only(?). At the current stage, limiting static/hermetic Java runtime support to only the platforms that support partial linking and `objcopy` seems to be overly restrictive (it does simplify the requirements significantly however :-)): - The duplicate symbol problems are mostly found in JDK natives and have been resolved already. We've found very few symbol issues with hotspot code so far. As there are portable alternative solutions that can resolve the symbol issues in hotspot, choosing a less portable solution seems not too attractive currently. - As we haven't found many duplicate symbol issues with hotspot code, resolving them case by case may still be a good choice. We don't have to tie into any permanent solution during the early stage. - Based on what we learned from the static/hermetic Java prototyping and investigations, majority of the work is non-os and non-cpu specific. If we can carefully handle the platform specific part with portable solution(s), we can support static/hermetic Java for different supported platforms as a more general solution. Those are my reasonings. :-) - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1916047210
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Wed, 24 Jan 2024 09:29:16 GMT, Andrew Haley wrote: > > > I think you should be able to use ld and objcopy to merge the .o files > > > and hide all of the symbols you don't want to export. > > > > > > We also discussed about `objcopy` in [#14808 > > (comment)](https://github.com/openjdk/jdk/pull/14808#issuecomment-1631597197) > > and [#14808 > > (comment)](https://github.com/openjdk/jdk/pull/14808#issuecomment-1631611220). > > My main concern was the portability of `objcopy` approach. > > I replied: > > OK, but it is the right thing to do on Linux. If some other operating systems > don't provide useful tools, that's on them. I haven't checked, but I strongly > suspect that LLVM can do it too, so all that remains is Windows, and maybe > they can't have static linking (or maybe they have to use something like this > PR) until the right tooling is provided. > > If Windows really can't do it, that's no reason to burden systems that can. > Namespaces are not a low-cost solution for developers. Thanks, @theRealAph. Yeah, I was mainly concerned about non-unix like systems, Windows particularly. It might not work on all potentially supported compilers (`gcc`) on linux, however. To localizing symbols in `libjvm` using `objcopy`, we can first partially link (with `-r`) all hotspot `.o` into a single object file, then run `objcopy` for the output object file to localize the affected symbols. The partial linking work (https://github.com/openjdk/jdk/blob/2003610b3b52eed04de6713a2a36151d0d86d7c9/make/common/NativeCompilation.gmk#L1245) has been added already. However, during the https://github.com/openjdk/jdk/pull/14064 work, we ran into issues with partial linking on older `gcc` for linux-aarch64. The details were captured in https://github.com/openjdk/jdk/pull/14064#issuecomment-1564908324 discussion with @erikj79. Only `clang` currently work well with the partial linking and symbol localizing solution. Maybe we could live with symbol redefinition using #define (conditionally for static linking in OpenJDK, as Coleen suggested earlier) for now, until the tooling can support symbol localizing better. Then localizing symbols using tools like `objcopy` can be the longer term and cleaner solution, instead of using namespace. What's your thoughts on that? - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1909019550
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Sat, 20 Jan 2024 18:00:15 GMT, Andrew Haley wrote: > > > You could support one build by adding something like > > > -DSUPPORTS_STATIC_LINK for both .so and .a builds for Google, then use > > > that to protect the renaming. > > > > > > Thanks, @coleenp. I think that could work for all different cases. I'll > > reflect that in this PR. > > For longer term we probably still want to find a cleaner solution when the > > static support becomes more popular. > > I think you should be able to use ld and objcopy to merge the .o files and > hide all of the symbols you don't want to export. We also discussed about `objcopy` in https://github.com/openjdk/jdk/pull/14808#issuecomment-1631597197 and https://github.com/openjdk/jdk/pull/14808#issuecomment-1631611220. My main concern was the portability of `objcopy` approach. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1903359606
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Fri, 19 Jan 2024 14:00:58 GMT, Coleen Phillimore wrote: > You could support one build by adding something like -DSUPPORTS_STATIC_LINK > for both .so and .a builds for Google, then use that to protect the renaming. Thanks, @coleenp. I think that could work for all different cases. I'll reflect that in this PR. For longer term we probably still want to find a cleaner solution when the static support becomes more popular. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1901057917
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Fri, 19 Jan 2024 08:58:29 GMT, Andrew Haley wrote: > > > > It seems that we may be converging on using hotspot namespace? > > > > > > > > > Based on previous discussions I had been expecting to see a JEP on this > > > after last US summer. I was surprised to see this PR pop up in this form. > > > > > > Ah, I see. Thanks for the clarification. I had an offline conversation with > > @iklam about the namespace and JEP topic during during last August JVM > > Language Summit. Based on the feedback from the conversion, it was not very > > clear if the namespace approach was broadly acceptable. > > Using a default namespace for everything could have bad effects on debugging > and other maintenance tools. it's unlikely to be a low-cost option. Perhaps > it could be restricted to the static linking case, but that further > complicates testing. Thanks. Agreed to both points. It seems to add too much complexities if the namespace usage is restricted to static linking case only. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1901043889
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Fri, 19 Jan 2024 01:57:58 GMT, David Holmes wrote: > > It seems that we may be converging on using hotspot namespace? > > Based on previous discussions I had been expecting to see a JEP on this after > last US summer. I was surprised to see this PR pop up in this form. Ah, I see. Thanks for the clarification. I had an offline conversation with @iklam about the namespace and JEP topic during during last August JVM Language Summit. Based on the feedback from the conversion, it was not very clear if the namespace approach was broadly acceptable. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1899527918
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Wed, 17 Jan 2024 23:06:19 GMT, Coleen Phillimore wrote:
>> 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.
>
> I was reading through the other PR for StringTable and was wonder how
> difficult it would be to wrap all of hotspot in namespace hotspot {}; using
> namespace hotspot; It would need a JEP as discussed in the other PR.
>
> Alternatively if there's a #ifdef you can use for renaming the Thread to
> HotspotThread for static linking only, it might make this change less
> worrysome.
Thanks @coleenp, @dholmes-ora. For using a hotspot namespace, there are
probably similar complications like the symbol usages that the current PR
addresses in src/hotspot/os_cpu/linux_aarch64/threadLS_linux_aarch64.S and
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Thread.java. There
might also be some complications with accessing hotspot code in JNI code. Those
issues probably could be resolved relatively easily, I haven't experimented it.
It seems that we may be converging on using hotspot namespace?
For just redefining the symbol only when doing static linking, it adds more
differences between the static and non-static support. It's more useful when we
can create both `.so` and `.a` from the same set of `.o` files without having
to build two different `.o` from each c/c++ source files.
-
PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1899039171
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Wed, 17 Jan 2024 10:07:15 GMT, Andrew Haley wrote: >> 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. > > Hooboy, this is an ugly solution, with some nasty side effects such as > confusing error mesasges for developers and a very confusing debugger > experience. Let's try to find a solution with a smaller blast radius. Hi @theRealAph Thanks for looking into this! https://github.com/openjdk/jdk/pull/14808 comments touched on several options: 1. Using namespace, in smaller scope for specific class such as `StringTable` or for all hotspot code in a global scope. Most seem to prefer using a specific namespace for all hotspot code, but there were still concerns. 2. Using #define to redefine the symbol (using in the current PR) This is a somewhat hacky solution. It requires small changes without touching many source code for renaming. 3. Redefine symbol at build/compile time. This is similar to the above. 4. Direct rename in the source Earlier discussions and feedback seem to prefer options requiring non-large scale change (except hotspot namespace solution). If acceptable by everyone, direct renaming would be the least confusion causing option. Any other suggestions and ideas for resolving the `Thread` issue? Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1896255274
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: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 20:52:46 GMT, Daniel D. Daugherty wrote: > In the interest of reducing the noise in the Mach5 CI, I'm going ahead with > integrating this fix without waiting for a reply to my comment above. If > there are remaining issues, then we'll deal with them in a follow-up bug/RFE. Thumbs up, thanks! - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839594297
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 20:52:46 GMT, Daniel D. Daugherty wrote: >>> @jianglizhou - Thanks for doing the testing. Can you also do a review? We >>> need two reviewers for this change. >> >> Complete test run finished. Checking the results, looks like there's no >> issue related to JVMTIThreadState change. >> >> @dcubed-ojdk Reducing the check to `(thread->threadObj() == nullptr && >> thread->is_attaching_via_jni())` looks ok. >> >> I just rechecked all usages of setup_jvmti_thread_state(). Currently it's >> used in three cases: >> - JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() >> - JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() >> - JvmtiSampledObjectAllocEventCollector::start() >> >> JDK-8319935 ran into issue with >> JvmtiSampledObjectAllocEventCollector::start() call path. We changed >> JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample() to >> avoid sampling if there is no thread obj allocated for the attaching thread. >> We also changed JvmtiThreadState::state_for_while_locked to handle the >> attaching case and return null. @dcubed-ojdk and @dholmes-ora, is there a >> case JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() >> might also see an attaching thread without the thread obj allocated? >> >> JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() call path >> probably is not affected by this case. > > @jianglizhou and @sspitsyn - Thanks for the reviews. > > In the interest of reducing the noise in the Mach5 CI, I'm going ahead with > integrating this fix without waiting for a reply to my comment above. If there > are remaining issues, then we'll deal with them in a follow-up bug/RFE. > > @dcubed-ojdk and @dholmes-ora, is there a case > > JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() > > might also see an attaching thread without the thread obj allocated? > > JvmtiVMObjectAllocEventCollector is the code path that we ran into with the > internal stress test, but in the case that tripped, we had a thread obj > allocated. If we ran the same code path without the thread obj allocated, > then we would return `nullptr` which was the goal of your original fix. > > I don't know if that specific test case is actually reached by any of our > tests, but if such a case occurred, it did not result in the guarantee() > firing or any other failure mode that I saw. The `guarantee(state != nullptr) > failed: exiting thread called setup_jvmti_thread_state` has not been seen in > Mach5 Tier[1-8] testing with this fix in place and I didn't see any other > test failures that could be tracked back to problems with this code. > > A JvmtiVMObjectAllocEventCollector object is created in 34 places in jvm.cpp > alone and I haven't checked all of those. I only checked out the one use in > JVM_GetStackAccessControlContext. Thanks. I was wondering if we would need further work to handle `JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector()` with something similar to the JvmtiSampledObjectAllocEventCollector change, if it's possible to trigger the `guarantee(state != nullptr)` for attaching thread with no allocated thread obj. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839626256
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935 [v2]
On Mon, 4 Dec 2023 18:16:52 GMT, Daniel D. Daugherty wrote: >> In the fix for the following bug: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> JvmtiThreadState::state_for_while_locked() was changed to >> return nullptr for attaching JNI threads regardless of whether >> that JNI thread/JavaThread had a java.lang.Thread object. >> >> We should only filter out a JavaThread that's attaching via JNI >> if it has no java.lang.Thread object. >> >> This fix has been tested with Mach5 Tier[1-7] and there are no related test >> failures. >> Mach5 Tier8 is in process. >> >> I'm going to need @jianglizhou to rerun her testing for: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> since the test(s) for that fix are not yet integrated in the jdk/jdk repo. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > dholmes CR - adjust a comment. Marked as reviewed by jiangli (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16934#pullrequestreview-1763177973
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Mon, 4 Dec 2023 05:16:59 GMT, Jiangli Zhou wrote: >> In the fix for the following bug: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> JvmtiThreadState::state_for_while_locked() was changed to >> return nullptr for attaching JNI threads regardless of whether >> that JNI thread/JavaThread had a java.lang.Thread object. >> >> We should only filter out a JavaThread that's attaching via JNI >> if it has no java.lang.Thread object. >> >> This fix has been tested with Mach5 Tier[1-7] and there are no related test >> failures. >> Mach5 Tier8 is in process. >> >> I'm going to need @jianglizhou to rerun her testing for: >> >> [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one >> JvmtiThreadState is created for one JavaThread associated with attached >> native thread >> >> since the test(s) for that fix are not yet integrated in the jdk/jdk repo. > > @dcubed-ojdk Thanks for the notification. I just ran one of our affected test > 100 times with JDK-8312174 change rolled back and with both following applied: > > - https://git.openjdk.org/jdk/pull/16642 > - https://github.com/openjdk/jdk/pull/16934 > > All 100 runs passed without failure. I'm going to run all tests tonight, will > report back later tomorrow if I see any issue. > @jianglizhou - Thanks for doing the testing. Can you also do a review? We > need two reviewers for this change. Complete test run finished. Checking the results, looks like there's no issue related to JVMTIThreadState change. @dcubed-ojdk Reducing the check to `(thread->threadObj() == nullptr && thread->is_attaching_via_jni())` looks ok. I just rechecked all usages of setup_jvmti_thread_state(). Currently it's used in three cases: - JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() - JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() - JvmtiSampledObjectAllocEventCollector::start() JDK-8319935 ran into issue with JvmtiSampledObjectAllocEventCollector::start() call path. We changed JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample() to avoid sampling if there is no thread obj allocated for the attaching thread. We also changed JvmtiThreadState::state_for_while_locked to handle the attaching case and return null. @dcubed-ojdk and @dholmes-ora, is there a case JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() might also see an attaching thread without the thread obj allocated? JvmtiDynamicCodeEventCollector::JvmtiDynamicCodeEventCollector() call path probably is not affected by this case. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839282331
Re: RFR: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
On Sat, 2 Dec 2023 17:15:57 GMT, Daniel D. Daugherty wrote: > In the fix for the following bug: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > JvmtiThreadState::state_for_while_locked() was changed to > return nullptr for attaching JNI threads regardless of whether > that JNI thread/JavaThread had a java.lang.Thread object. > > We should only filter out a JavaThread that's attaching via JNI > if it has no java.lang.Thread object. > > This fix has been tested with Mach5 Tier[1-7] and there are no related test > failures. > Mach5 Tier8 is in process. > > I'm going to need @jianglizhou to rerun her testing for: > > [JDK-8319935](https://bugs.openjdk.org/browse/JDK-8319935) Ensure only one > JvmtiThreadState is created for one JavaThread associated with attached > native thread > > since the test(s) for that fix are not yet integrated in the jdk/jdk repo. @dcubed-ojdk Thanks for the notification. I just ran one of our affected test 100 times with JDK-8312174 change rolled back and with both following applied: - https://git.openjdk.org/jdk/pull/16642 - https://github.com/openjdk/jdk/pull/16934 All 100 runs passed without failure. I'm going to run all tests tonight, will report back later tomorrow if I see any issue. - PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1837858621
Re: RFR: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread [v5]
On Wed, 29 Nov 2023 23:06:10 GMT, Daniel D. Daugherty wrote: > A belated thumbs up. Sorry I didn't get back to this review before the fix > was integrated. Still thanks for reviewing the change, @dcubed-ojdk. > > I found just a nit comment that could be more clear. The particular issue occurred when `JavaThread::allocate_threadObj` was allocating and initializing the Thread instance. When the allocation of the Thread object triggered sampling, it could create a `JvmtiThreadState` with null thread oop with the bug. It seems "is being allocated" describes the issue more accurately. > src/hotspot/share/prims/jvmtiExport.cpp line 3143: > >> 3141: >> 3142: // If the current thread is attaching from native and its Java >> thread object >> 3143: // is being allocated, things are not ready for allocation sampling. > > nit - typo: s/is being allocated/has not been allocated/ Please see other comment. - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1834148374 PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1410956027
Integrated: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread
On Mon, 13 Nov 2023 21:52:22 GMT, Jiangli Zhou wrote: > Please review JvmtiThreadState::state_for_while_locked change to handle the > state->get_thread_oop() null case. Please see > https://bugs.openjdk.org/browse/JDK-8319935 for details. This pull request has now been integrated. Changeset: da7bcfcf Author:Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/da7bcfcf6e45486a0427e0ceaba74d52acbd722f Stats: 28 lines in 2 files changed: 22 ins; 4 del; 2 mod 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread Reviewed-by: manc, dholmes, sspitsyn - PR: https://git.openjdk.org/jdk/pull/16642
Re: RFR: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread [v5]
On Tue, 28 Nov 2023 21:28:31 GMT, Jiangli Zhou wrote: >> This fix looks good to me, so approved now. >> I assume it is for 22. Is it correct? > >> This fix looks good to me, so approved now. I assume it is for 22. Is it >> correct? > > Thanks for the careful review, @sspitsyn! The fix is for 22. We probably > should also consider back-porting to JDK 11 to prevent any potential changes > in the area accidentally reintroducing the bug. The > https://bugs.openjdk.org/browse/JDK-8312174 change has been back-ported to > 11, which resolved the crashes by luck. > > I'll request backport after this fix is integrated. > @jianglizhou Thank you for filing the sub-task. You have already seen some > crashes. Even though you do not have a standalone test case, it is still > valuable if you describe a test scenario (at least, surfacely) which helped > to observe the problem. Could you, add it to the sub-task report, please? Hi @sspitsyn I'll comment on https://bugs.openjdk.org/browse/JDK-8320614 later, thanks. - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1832193011
Re: RFR: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread [v2]
On Thu, 16 Nov 2023 21:58:17 GMT, Man Cao wrote: >> Jiangli Zhou has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Don't try to setup_jvmti_thread_state for obj allocation sampling if the >> current thread is attaching from native and is allocating the thread oop. >> That's to make sure we don't create a 'partial' JvmtiThreadState. > > Thanks. The latest change to > `JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample()` > looks OK to me. Skipping a few allocations for JVMTI allocation sampler is > better than resulting in a problematic `JvmtiThreadState` instance. > > My main question is if we can now change > `if (state == nullptr || state->get_thread_oop() != thread_oop) ` to `if > (state == nullptr)` in `JvmtiThreadState::state_for_while_locked()`. I > suspect we would never run into a case of `state != nullptr && > state->get_thread_oop() != thread_oop` with the latest change, even with > virtual threads. This is backed up by testing with > https://github.com/openjdk/jdk/commit/00ace66c36243671a0fb1b673b3f9845460c6d22 > not triggering any failure. > > If we run into such as a case, it could still be problematic as > `JvmtiThreadState::state_for_while_locked()` would allocate a new > `JvmtiThreadState` instance pointing to the same JavaThread, and it does not > delete the existing instance. > > Could anyone with deep knowledge on JvmtiThreadState and virtual threads > provide some feedback on this change and > https://bugs.openjdk.org/browse/JDK-8319935? @AlanBateman, do you know who > would be the best reviewer for this? @caoman @dholmes-ora Thank you for the reviews and discussions in this thread. - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1832198014
Re: RFR: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread [v5]
On Tue, 28 Nov 2023 20:43:15 GMT, Serguei Spitsyn wrote: > This fix looks good to me, so approved now. I assume it is for 22. Is it > correct? Thanks for the careful review, @sspitsyn! The fix is for 22. We probably should also consider back-porting to JDK 11 to prevent any potential changes in the area accidentally reintroducing the bug. The https://bugs.openjdk.org/browse/JDK-8312174 change has been back-ported to 11, which resolved the crashes by luck. I'll request backport after this fix is integrated. - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1830777696
Re: RFR: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread [v5]
On Tue, 28 Nov 2023 05:08:58 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 98:
>>
>>> 96:state->get_thread_oop() != thread_oop)) {
>>> 97: // Check if java_lang_Thread already has a link to the
>>> JvmtiThreadState.
>>> 98: if (thread_oop != nullptr) { // thread_oop can be null during
>>> early VMStart.
>>
>> This comment is another case of `state->get_thread_oop()` being null. We
>> should merge this comment with the new comment about attaching native thread.
>
> This also was caught by my eyes. :)
> With the lines 99-101 in place the only case when `thread_oop` can be equal
> to `nullptr` is when `thread->threadObj() == nullptr`. My understanding is
> that it can be for a detached thread only.
> I would suggest to add an assert after the line 101: ` assert(thread_oop !=
> nullptr, "sanity check");`
> Full testing with this assert should help to identify if it can be fired. If
> such cases are found then they need to be fixed. Then we can remove the check
> at the line 104.
> The `JvmtiThreadState` constructor also allows for `thread_oop` to be
> `nullptr`.
> Some cleanup will be needed to get rid of unneeded checks there as well.
@sspitsyn For the above suggestions, it seems cleaner/safer to handle the
clean-ups in a separate RFE with full testing including the vthread cases.
There are additional comments in
https://github.com/openjdk/jdk/pull/16642#issuecomment-1815379890 related to
this as well. Those could be handled together and require through testing
including the vthread support.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1408228229
Re: RFR: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread [v3]
On Wed, 22 Nov 2023 01:24:46 GMT, Serguei Spitsyn wrote: > Thank you for filing and fixing this issue! I'm kind of late here. Sorry for > that. Is it hard to create a JTreg test for an attaching native thread? I can > help if you have a standalone prototype. You can look for some examples in > the folder: `test/hotspot/jtreg/serviceability/jvmti/vthread`. Hi @sspitsyn we don't have an extracted standalone test case (yet) to demonstrate the crashes. The crashes could not reproduce consistently. Outside the debugger (lldb), I ran the test (one of the affected ones) 10 times/per-iteration in order to reproduce. I found the crashes could be affected by both timing and memory layout. During the investigation, I noticed the problem became hidden when I increased allocation size for ThreadsList::_threads (as one of the experiments that I did, I wanted to mprotect the memory to be read-only in order to find who trashed the memory, so was trying to allocate memory up to page boundary). That's the reason why I added noreg-hard tag earlier. I gave some more thoughts today. Perhaps, we could write a whitebox test to check the JvmtiThreadState, without being able to consistently trigger crashes. We could add a WhiteBox api to iterate the JvmtiThreadState list and validate if all the JavaThread pointers were valid after detaching. The test would need to create native threads to attach and detach before the check. That could more reliably test the 1-1 mapping of JvmtiThreadState and JavaThread. What do you think? Thanks for volunteering to help with the test. I created https://bugs.openjdk.org/browse/JDK-8320614 today. Should I assign it to you? - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1823672341
Re: RFR: 8319935: Ensure only one JvmtiThreadState is created for one JavaThread associated with attached native thread [v2]
On Fri, 17 Nov 2023 02:51:03 GMT, Jiangli Zhou wrote: >> Jiangli Zhou has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Don't try to setup_jvmti_thread_state for obj allocation sampling if the >> current thread is attaching from native and is allocating the thread oop. >> That's to make sure we don't create a 'partial' JvmtiThreadState. > >> Thanks. The latest change to >> `JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample()` >> looks OK to me. Skipping a few allocations for JVMTI allocation sampler is >> better than resulting in a problematic `JvmtiThreadState` instance. >> >> My main question is if we can now change `if (state == nullptr || >> state->get_thread_oop() != thread_oop) ` to `if (state == nullptr)` in >> `JvmtiThreadState::state_for_while_locked()`. I suspect we would never run >> into a case of `state != nullptr && state->get_thread_oop() != thread_oop` >> with the latest change, even with virtual threads. This is backed up by >> testing with >> [00ace66](https://github.com/openjdk/jdk/commit/00ace66c36243671a0fb1b673b3f9845460c6d22) >> not triggering any failure. >> >> If we run into such as a case, it could still be problematic as >> `JvmtiThreadState::state_for_while_locked()` would allocate a new >> `JvmtiThreadState` instance pointing to the same JavaThread, and it does not >> delete the existing instance. >> >> Could anyone with deep knowledge on JvmtiThreadState and virtual threads >> provide some feedback on this change and >> https://bugs.openjdk.org/browse/JDK-8319935? @AlanBateman, do you know who >> would be the best reviewer for this? > > @caoman and I discussed about his suggestion on changing `if (state == > nullptr || state->get_thread_oop() != thread_oop)` check in person today. > Since it may affect vthread, my main concern is that our current testing may > not cover that sufficiently. The suggestion could be worked by a separate > enhancement bug. > > > @jianglizhou - I fixed a typo in the bug's synopsis line. Change this > > > PR's title: s/is create/is created/ > > > Thanks, @dcubed-ojdk! > > Now, the PR title needs to be fixed accordingly. Done, thanks for the reminder! - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1823599300
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v3]
On Tue, 21 Nov 2023 23:32:13 GMT, Serguei Spitsyn wrote: >> Jiangli Zhou has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add a check for a thread is_attaching_via_jni, based on David Holmes' >> comment. > > src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 100: > >> 98: assert(state->get_thread_oop() != nullptr, "incomplete state"); >> 99: } >> 100: #endif > > Nit: I would suggest to write this assert in the form: > > // Make sure we don't see an incomplete state. An incomplete state can cause > // a duplicate JvmtiThreadState being created below and bound to the > 'thread' > // incorrectly, which leads to stale JavaThread* from the JvmtiThreadState > // after the thread exits. >assert(state == nullptr || state->get_thread_oop() != nullptr, "incomplete > state"); > > The `#ifdef ASSERT` and `#endif` are not needed then. Changed as suggested. - PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1402771379
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v3]
On Wed, 22 Nov 2023 02:53:23 GMT, David Holmes wrote:
>> src/hotspot/share/prims/jvmtiExport.cpp line 3144:
>>
>>> 3142: // If the current thread is attaching from native and its thread
>>> oop is being
>>> 3143: // allocated, things are not ready for allocation sampling.
>>> 3144: if (thread->is_Java_thread()) {
>>
>> Nit: There is no need for this check at line 3144.
>> There was already check for `!thread->is_Java_thread()` and return with
>> false at line 3138:
>>
>> if (!thread->is_Java_thread() || thread->is_Compiler_thread()) {
>> return false;
>> }
>
> +1
Indeed, removed. Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1402770901
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v5]
> Please review JvmtiThreadState::state_for_while_locked change to handle the > state->get_thread_oop() null case. Please see > https://bugs.openjdk.org/browse/JDK-8319935 for details. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Address Serguei Spitsyn's comments/suggestions: - Remove the redundant thread->is_Java_thread() check from JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample(). - Change the assert in JvmtiThreadState::state_for_while_locked to avoid #ifdef ASSERT. - Changes: - all: https://git.openjdk.org/jdk/pull/16642/files - new: https://git.openjdk.org/jdk/pull/16642/files/de7fac6d..7c366df0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16642=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16642=03-04 Stats: 12 lines in 2 files changed: 0 ins; 5 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/16642.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16642/head:pull/16642 PR: https://git.openjdk.org/jdk/pull/16642
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v4]
> Please review JvmtiThreadState::state_for_while_locked change to handle the > state->get_thread_oop() null case. Please see > https://bugs.openjdk.org/browse/JDK-8319935 for details. Jiangli Zhou 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 four additional commits since the last revision: - Merge branch 'master' into JDK-8319935 - Add a check for a thread is_attaching_via_jni, based on David Holmes' comment. - Don't try to setup_jvmti_thread_state for obj allocation sampling if the current thread is attaching from native and is allocating the thread oop. That's to make sure we don't create a 'partial' JvmtiThreadState. - 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread - Changes: - all: https://git.openjdk.org/jdk/pull/16642/files - new: https://git.openjdk.org/jdk/pull/16642/files/7c0214e2..de7fac6d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16642=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16642=02-03 Stats: 30285 lines in 831 files changed: 17825 ins; 7190 del; 5270 mod Patch: https://git.openjdk.org/jdk/pull/16642.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16642/head:pull/16642 PR: https://git.openjdk.org/jdk/pull/16642
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v2]
On Thu, 16 Nov 2023 09:48:48 GMT, David Holmes wrote: >> Jiangli Zhou has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Don't try to setup_jvmti_thread_state for obj allocation sampling if the >> current thread is attaching from native and is allocating the thread oop. >> That's to make sure we don't create a 'partial' JvmtiThreadState. > > src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 87: > >> 85: // Don't add a JvmtiThreadState to a thread that is exiting. >> 86: return nullptr; >> 87: } > > I'm wondering if there should also be an `is_jni_attaching` check here? That seems to be a good idea. It would cover other cases that we haven't seen yet. Added a check as suggested, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1399647867
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v3]
> Please review JvmtiThreadState::state_for_while_locked change to handle the > state->get_thread_oop() null case. Please see > https://bugs.openjdk.org/browse/JDK-8319935 for details. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Add a check for a thread is_attaching_via_jni, based on David Holmes' comment. - Changes: - all: https://git.openjdk.org/jdk/pull/16642/files - new: https://git.openjdk.org/jdk/pull/16642/files/c2f83e8a..7c0214e2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16642=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16642=01-02 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16642.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16642/head:pull/16642 PR: https://git.openjdk.org/jdk/pull/16642
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v2]
On Tue, 14 Nov 2023 20:57:09 GMT, Jiangli Zhou wrote: >> Please review JvmtiThreadState::state_for_while_locked change to handle the >> state->get_thread_oop() null case. Please see >> https://bugs.openjdk.org/browse/JDK-8319935 for details. > > Jiangli Zhou has updated the pull request incrementally with one additional > commit since the last revision: > > Don't try to setup_jvmti_thread_state for obj allocation sampling if the > current thread is attaching from native and is allocating the thread oop. > That's to make sure we don't create a 'partial' JvmtiThreadState. > Thanks. The latest change to > `JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample()` > looks OK to me. Skipping a few allocations for JVMTI allocation sampler is > better than resulting in a problematic `JvmtiThreadState` instance. > > My main question is if we can now change `if (state == nullptr || > state->get_thread_oop() != thread_oop) ` to `if (state == nullptr)` in > `JvmtiThreadState::state_for_while_locked()`. I suspect we would never run > into a case of `state != nullptr && state->get_thread_oop() != thread_oop` > with the latest change, even with virtual threads. This is backed up by > testing with > [00ace66](https://github.com/openjdk/jdk/commit/00ace66c36243671a0fb1b673b3f9845460c6d22) > not triggering any failure. > > If we run into such as a case, it could still be problematic as > `JvmtiThreadState::state_for_while_locked()` would allocate a new > `JvmtiThreadState` instance pointing to the same JavaThread, and it does not > delete the existing instance. > > Could anyone with deep knowledge on JvmtiThreadState and virtual threads > provide some feedback on this change and > https://bugs.openjdk.org/browse/JDK-8319935? @AlanBateman, do you know who > would be the best reviewer for this? @caoman and I discussed about his suggestion on changing `if (state == nullptr || state->get_thread_oop() != thread_oop)` check in person today. Since it may affect vthread, my main concern is that our current testing may not cover that sufficiently. The suggestion could be worked by a separate enhancement bug. - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1815667615
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v2]
On Mon, 13 Nov 2023 23:04:19 GMT, Man Cao wrote: >> Jiangli Zhou has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Don't try to setup_jvmti_thread_state for obj allocation sampling if the >> current thread is attaching from native and is allocating the thread oop. >> That's to make sure we don't create a 'partial' JvmtiThreadState. > > src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 94: > >> 92: // The state->get_thread_oop() may be null if the state is created >> during >> 93: // the allocation of the thread oop when a native thread is attaching. >> Make >> 94: // sure we don't create a new state for the JavaThread. > > I think it is important to maintain `JvmtiThreadState::_thread_oop_h` > correctly for the attached native thread. In the existing logic, with and > without the proposed change, `JvmtiThreadState::_thread_oop_h` could stay > null for an attached native thread, which seems wrong. > > It may be OK since `JvmtiThreadState::_thread_oop_h` is only used by support > for virtual threads. It is unlikely that an attached native thread becomes a > carrier for a virtual thread. However, it is probably still desirable to > update `JvmtiThreadState::_thread_oop_h` to the correct java.lang.Thread oop. Thanks for the input @caoman. I updated the PR to avoid creating a JvmtiThreadState during attaching and allocating thread oop. I think that avoids a incomplete JvmtiThreadState being created, which is seems to be cleaner. - PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1393334558
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread [v2]
> Please review JvmtiThreadState::state_for_while_locked change to handle the > state->get_thread_oop() null case. Please see > https://bugs.openjdk.org/browse/JDK-8319935 for details. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Don't try to setup_jvmti_thread_state for obj allocation sampling if the current thread is attaching from native and is allocating the thread oop. That's to make sure we don't create a 'partial' JvmtiThreadState. - Changes: - all: https://git.openjdk.org/jdk/pull/16642/files - new: https://git.openjdk.org/jdk/pull/16642/files/959305be..c2f83e8a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16642=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16642=00-01 Stats: 24 lines in 2 files changed: 19 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16642.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16642/head:pull/16642 PR: https://git.openjdk.org/jdk/pull/16642
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread
On Tue, 14 Nov 2023 03:10:20 GMT, David Holmes wrote: > Is this a case where the code should be checking for `is_attaching_via_jni()`? That's a good question. I think maybe we should try to completely avoid the situation where a 'partial' `JvmtiThreadState` is created when a native thread is attaching and is in the middle of allocating the thread oop. Looking at `JvmtiSampledObjectAllocEventCollector::start()`, I think we can check if the current JavaThread `is_attaching_via_jni()` and the `threadObj()` is null. If that's the case, don't try `setup_jvmti_thread_state()` as things are not ready. In `JvmtiThreadState::state_for_while_locked` we probably want to assert that thread->threadObj() is not null if thread->jvmti_thread_state() not null, to make sure that we don't see a incomplete `JvmtiThreadState`. @caoman, I think this can also address your input on keeping `JvmtiThreadState::_thread_oop_h` always properly initialized for the attaching native thread. I tested it and it seems to work well. I'll update this PR. - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1811187153
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread
On Mon, 13 Nov 2023 23:21:56 GMT, Man Cao wrote: >> Please review JvmtiThreadState::state_for_while_locked change to handle the >> state->get_thread_oop() null case. Please see >> https://bugs.openjdk.org/browse/JDK-8319935 for details. > > src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 95: > >> 93: // the allocation of the thread oop when a native thread is attaching. >> Make >> 94: // sure we don't create a new state for the JavaThread. >> 95: if (state == nullptr || (state->get_thread_oop() != nullptr && > > In my limited testing with and without virtual threads, when `state` is not > null, `state->get_thread_oop()` is always either null or equal to > `thread_oop`. So I suspect this whole if is equivalent to `if (state == > nullptr)` in practice. > > I think it is better to split this if into two conditions. Here is my > proposed change: > https://github.com/openjdk/jdk/commit/00ace66c36243671a0fb1b673b3f9845460c6d22. > It adds a `JvmtiThreadState::set_thread_oop()` method, and checks the above > observation with `state->get_thread_oop()`, and addresses the problem of > `JvmtiThreadState::_thread_oop_h` incorrectly staying null for attached > native thread. > > I tested it with the 1 virtual threads running Thread.sleep() example > from https://openjdk.org/jeps/425, and `make test TEST=jdk_loom`. If my understand of [JavaThread::rebind_to_jvmti_thread_state_of](https://github.com/openjdk/jdk/blob/fe0ccdf5f8a5559a608d2e2cd2b6aecbe245c5ec/src/hotspot/share/runtime/javaThread.cpp#L1819) is correct, state->get_thread_oop() could be either `JavaThread::_threadObj` or `JavaThread::_jvmti_vthread`, ignoring the null case here. The `state` could be null if the virtual thread is unmounted and the thread is null, or if a JvmtiThreadState has not been created for the thread. Currently the `JvmtiThreadState::_thread_oop_h` is 'sealed' once a JvmtiThreadState is created. It can never be altered before a JvmtiThreadState is destroyed. Adding a `JvmtiThreadState:set_thread_oop()` changes that assumption. Let's wait for others who are more involved in this area to comment as well. I just took a look of your https://github.com/openjdk/jdk/commit/00ace66c36243671a0fb1b673b3f9845460c6d22 change. I think it does not address the specific issue that we were seeing in our tests. At the time when `JvmtiThreadState::state_for_while_locked` is called for a JavaThread associated with a native thread being attached, the thread oop is being allocated. There is no non-null thread oop to be set into `JvmtiThreadState::_thread_oop_h`. I'll address other comments later. Thanks, @caoman. - PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1391895645
Re: RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread
On Mon, 13 Nov 2023 22:15:18 GMT, Daniel D. Daugherty wrote: > @jianglizhou - I fixed a typo in the bug's synopsis line. Change this PR's > title: s/is create/is created/ Thanks, @dcubed-ojdk! - PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1809279446
RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread
Please review JvmtiThreadState::state_for_while_locked change to handle the state->get_thread_oop() null case. Please see https://bugs.openjdk.org/browse/JDK-8319935 for details. - Commit messages: - 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread Changes: https://git.openjdk.org/jdk/pull/16642/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16642=00 Issue: https://bugs.openjdk.org/browse/JDK-8319935 Stats: 14 lines in 1 file changed: 9 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16642.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16642/head:pull/16642 PR: https://git.openjdk.org/jdk/pull/16642
Integrated: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code
On Tue, 29 Aug 2023 23:37:06 GMT, Jiangli Zhou wrote: > Please review this simple change from @cjmoon1 for resolving static linking > issue caused by multiple definition of 'normalize'. @cjmoon1's branch can be > found at: > https://github.com/openjdk/jdk/compare/master...cjmoon1:jdk:fix_normalize. > > The change incorporated suggestions from both @sspitsyn and myself: > > - > https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 > - > https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 This pull request has now been integrated. Changeset: 455c471e Author:Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/455c471ee36e26dd1ece61c615b8421d65359d5d Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code Co-authored-by: Chris Moon Reviewed-by: dholmes, stuefe, sspitsyn - PR: https://git.openjdk.org/jdk/pull/15477
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code [v2]
On Fri, 1 Sep 2023 21:11:03 GMT, Jiangli Zhou wrote: >> Please review this simple change from @cjmoon1 for resolving static linking >> issue caused by multiple definition of 'normalize'. @cjmoon1's branch can be >> found at: >> https://github.com/openjdk/jdk/compare/master...cjmoon1:jdk:fix_normalize. >> >> The change incorporated suggestions from both @sspitsyn and myself: >> >> - >> https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 >> - >> https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 > > Jiangli Zhou has updated the pull request incrementally with one additional > commit since the last revision: > > Update from @cjmoon1 to incorporate @dholmes-ora suggestion: > https://github.com/openjdk/jdk/commit/60eebebdcfb15357701d60ca455c0708c34c43af. Thanks for everyone's review/approval! Thanks for everyone's review/approval! - PR Comment: https://git.openjdk.org/jdk/pull/15477#issuecomment-1715301412 PR Comment: https://git.openjdk.org/jdk/pull/15477#issuecomment-1715301416
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code [v2]
On Fri, 1 Sep 2023 21:11:03 GMT, Jiangli Zhou wrote: >> Please review this simple change from @cjmoon1 for resolving static linking >> issue caused by multiple definition of 'normalize'. @cjmoon1's branch can be >> found at: >> https://github.com/openjdk/jdk/compare/master...cjmoon1:jdk:fix_normalize. >> >> The change incorporated suggestions from both @sspitsyn and myself: >> >> - >> https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 >> - >> https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 > > Jiangli Zhou has updated the pull request incrementally with one additional > commit since the last revision: > > Update from @cjmoon1 to incorporate @dholmes-ora suggestion: > https://github.com/openjdk/jdk/commit/60eebebdcfb15357701d60ca455c0708c34c43af. The changes look good to me. Pending for @dholmes-ora and @sspitsyn review/approval. - PR Review: https://git.openjdk.org/jdk/pull/15477#pullrequestreview-1607673381
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code [v2]
> Please review this simple change from @cjmoon1 for resolving static linking > issue caused by multiple definition of 'normalize'. @cjmoon1's branch can be > found at: > https://github.com/openjdk/jdk/compare/master...cjmoon1:jdk:fix_normalize. > > The change incorporated suggestions from both @sspitsyn and myself: > > - > https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 > - > https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Update from @cjmoon1 to incorporate @dholmes-ora suggestion: https://github.com/openjdk/jdk/commit/60eebebdcfb15357701d60ca455c0708c34c43af. - Changes: - all: https://git.openjdk.org/jdk/pull/15477/files - new: https://git.openjdk.org/jdk/pull/15477/files/8b1d7dec..5138a0b4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15477=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15477=00-01 Stats: 20 lines in 4 files changed: 0 ins; 0 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/15477.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15477/head:pull/15477 PR: https://git.openjdk.org/jdk/pull/15477
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code
On Thu, 31 Aug 2023 05:00:18 GMT, David Holmes wrote: > Is `normalize` the only function that causes a conflict and the other changes > are just for consistency? If so please just rename normalize to e.g. > `normalize_path` Right, only `normalize` showed up in our testing so far. I suggested @cjmoon1 to also change other functions in the file mainly for consistency purpose. Just rename normalize to `normalize_path` sounds ok to me. Will wait a bit to see if @sspitsyn has any thoughts as well. - PR Comment: https://git.openjdk.org/jdk/pull/15477#issuecomment-1701300310
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code
On Thu, 31 Aug 2023 04:53:26 GMT, David Holmes wrote: > No not namespaces, just by using C++ so it a member function, then C++ name > mangling would mean the symbol would be different to some same-named function > in some other library - in essence the class name part of the symbol would > provide the "unique" prefix. Thanks for the clarification. Yeah, C++ name mangling indeed helps in many cases. For hotspot code, we've only seen very few issues where the class names are the same, such as StringTable and Thread (with pending OpenJDK bugs filed already). - PR Comment: https://git.openjdk.org/jdk/pull/15477#issuecomment-1701292028
Re: RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code
On Wed, 30 Aug 2023 02:45:42 GMT, David Holmes wrote: > I'm sure I've said it before but there has to be a better way to handle this. > We should not have to rename perfectly well-named functions to insert a > unique prefix due to static linking conflicts. If this were C++ code rather > than C, would that fix things? Hi @dholmes-ora, thanks for looking into this PR. Yeah, we had discussed the topic in earlier similar PRs' review comments. Just to collect earlier discussions, following solutions can resolve duplicate symbol triggered static linking failure: 1. Convert affected function/variable to static whenever applicable `appendBootClassPath` in src/java.instrument/share/native/libinstrument/InvocationAdapter.c is the only caller for `normalize`. However that's shared native code. `normalize` is implemented in OS specific code, so we can't include the implementation in the same file of the caller to make `normalize` static. 2. Direct renaming, #define (or using -D) to redefine symbols for affected function/variable 3. Use namespace isolation for C++ code 4. Symbols localizing using objcopy (`--localize-symbols`) For this solution, we can partially link all `.o` for an affected native library into a single object file (`.o`) first, then run `objcopy` to localize the affected symbol(s). The resulting object file can be used for the final static linking without causing duplicate symbol issue. Partial linking/objcopy is not portable for all platforms what we should support in OpenJDK. Could you please clarify the last part of your comment? Is it related to namespace? > If this were C++ code rather than C, would that fix things? @cjmoon1's change uses direct renaming, which seems to be the most straightforward. It's possible there are other potential solutions for duplicate symbol issues with static linking, I haven't explored those yet. - PR Comment: https://git.openjdk.org/jdk/pull/15477#issuecomment-1699409048
RFR: 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code
Please review this simple change from @cjmoon1 for resolving static linking issue caused by multiple definition of 'normalize'. @cjmoon1's branch can be found at: https://github.com/openjdk/jdk/compare/master...cjmoon1:jdk:fix_normalize. The change incorporated suggestions from both @sspitsyn and myself: - https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 - https://bugs.openjdk.org/browse/JDK-8313277?focusedCommentId=14604187=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14604187 - Commit messages: - 8313277: Resolve multiple definition of 'normalize' when statically linking JDK native libraries with user code Changes: https://git.openjdk.org/jdk/pull/15477/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15477=00 Issue: https://bugs.openjdk.org/browse/JDK-8313277 Stats: 20 lines in 4 files changed: 0 ins; 0 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/15477.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15477/head:pull/15477 PR: https://git.openjdk.org/jdk/pull/15477
Integrated: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
On Mon, 17 Apr 2023 16:56:31 GMT, Jiangli Zhou wrote: > - Make functions 'static' when feasible: > - throwByName() in > src/java.security.jgss/share/native/libj2gss/NativeUtil.c. > - throwByName(), throwIOException() and throwNullPointerException() in > src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c. > - throwByName() in > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c. > - throwOutOfMemoryError() in > src/java.smartcardio/share/native/libj2pcsc/pcsc.c. > - Move throwDisconnectedRuntimeException() to > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only > used in the file. Make it static. > - Move throw_internal_error() to > src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as > it's only used in the file. Make it static. > > - Rename functions by following the existing naming usages in different > libraries code: > - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss. > - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in > libj2pks11. > - Rename throwNullPointerException() to p11ThrowNullPointerException() in > libj2pks11. > - Rename throwIOException() to p11ThrowIOException() in libj2pks11. > - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException() > in libj2pks11. This function only exists in libj2pks11. The rename is done so > the function naming is consistent with the other throw functions. > > - Remove throw_internal_error() from > src/java.management/share/native/libmanagement/management.h and > src/java.management/share/native/libmanagement/management.c. It's not used. > - Remove throw_internal_error() from > src/jdk.management/share/native/libmanagement_ext/management_ext.h and > src/jdk.management/share/native/libmanagement_ext/management_ext.c. This pull request has now been integrated. Changeset: 9bc6a212 Author:Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/9bc6a212f70eede108a8d3bc1ba1f780722b6e33 Stats: 194 lines in 25 files changed: 17 ins; 34 del; 143 mod 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/13497
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries [v2]
On Tue, 25 Apr 2023 00:47:17 GMT, Jiangli Zhou wrote: >> - Make functions 'static' when feasible: >> - throwByName() in >> src/java.security.jgss/share/native/libj2gss/NativeUtil.c. >> - throwByName(), throwIOException() and throwNullPointerException() in >> src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c. >> - throwByName() in >> src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c. >> - throwOutOfMemoryError() in >> src/java.smartcardio/share/native/libj2pcsc/pcsc.c. >> - Move throwDisconnectedRuntimeException() to >> src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only >> used in the file. Make it static. >> - Move throw_internal_error() to >> src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as >> it's only used in the file. Make it static. >> >> - Rename functions by following the existing naming usages in different >> libraries code: >> - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss. >> - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in >> libj2pks11. >> - Rename throwNullPointerException() to p11ThrowNullPointerException() in >> libj2pks11. >> - Rename throwIOException() to p11ThrowIOException() in libj2pks11. >> - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException() >> in libj2pks11. This function only exists in libj2pks11. The rename is done >> so the function naming is consistent with the other throw >> functions. >> >> - Remove throw_internal_error() from >> src/java.management/share/native/libmanagement/management.h and >> src/java.management/share/native/libmanagement/management.c. It's not used. >> - Remove throw_internal_error() from >> src/jdk.management/share/native/libmanagement_ext/management_ext.h and >> src/jdk.management/share/native/libmanagement_ext/management_ext.c. > > Jiangli Zhou has updated the pull request incrementally with one additional > commit since the last revision: > > Minor updates, as suggested by mcpowers: > - Update copyright headers in affected files. > - Formatting update in > src/java.security.jgss/share/native/libj2gss/GSSLibStub.c. > - Remove throwByName() and move the logic into gssThrowOutOfMemoryError() > (the only caller) in > src/java.security.jgss/share/native/libj2gss/NativeUtil.c. Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/13497#issuecomment-1523566931
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries [v2]
> - Make functions 'static' when feasible: > - throwByName() in > src/java.security.jgss/share/native/libj2gss/NativeUtil.c. > - throwByName(), throwIOException() and throwNullPointerException() in > src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c. > - throwByName() in > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c. > - throwOutOfMemoryError() in > src/java.smartcardio/share/native/libj2pcsc/pcsc.c. > - Move throwDisconnectedRuntimeException() to > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only > used in the file. Make it static. > - Move throw_internal_error() to > src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as > it's only used in the file. Make it static. > > - Rename functions by following the existing naming usages in different > libraries code: > - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss. > - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in > libj2pks11. > - Rename throwNullPointerException() to p11ThrowNullPointerException() in > libj2pks11. > - Rename throwIOException() to p11ThrowIOException() in libj2pks11. > - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException() > in libj2pks11. This function only exists in libj2pks11. The rename is done so > the function naming is consistent with the other throw functions. > > - Remove throw_internal_error() from > src/java.management/share/native/libmanagement/management.h and > src/java.management/share/native/libmanagement/management.c. It's not used. > - Remove throw_internal_error() from > src/jdk.management/share/native/libmanagement_ext/management_ext.h and > src/jdk.management/share/native/libmanagement_ext/management_ext.c. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Minor updates, as suggested by mcpowers: - Update copyright headers in affected files. - Formatting update in src/java.security.jgss/share/native/libj2gss/GSSLibStub.c. - Remove throwByName() and move the logic into gssThrowOutOfMemoryError() (the only caller) in src/java.security.jgss/share/native/libj2gss/NativeUtil.c. - Changes: - all: https://git.openjdk.org/jdk/pull/13497/files - new: https://git.openjdk.org/jdk/pull/13497/files/9d319df6..fcb35192 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13497=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13497=00-01 Stats: 35 lines in 25 files changed: 0 ins; 6 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/13497.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13497/head:pull/13497 PR: https://git.openjdk.org/jdk/pull/13497
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries [v2]
On Mon, 24 Apr 2023 22:41:53 GMT, Mark Powers wrote:
> Update copyrights to 2023.
Done, thanks.
> src/java.security.jgss/share/native/libj2gss/GSSLibStub.c line 201:
>
>> 199: cb = malloc(sizeof(struct gss_channel_bindings_struct));
>> 200: if (cb == NULL) {
>> 201: gssThrowOutOfMemoryError(env,NULL);
>
> While you're fixing this, add a space between arguments, e.g. `(env,NULL)
> `becomes `(env, NULL)`.
Done, thanks.
> src/java.security.jgss/share/native/libj2gss/NativeUtil.c line 456:
>
>> 454:
>> 455: /* Throws a Java Exception by name */
>> 456: static void throwByName(JNIEnv *env, const char *name, const char *msg)
>> {
>
> Why can't you move the few lines of `throwByName()` into
> `gssThrowOutOfMemoryError()` and totally remove `throwByName()`?
Updated as suggested, thanks.
-
PR Comment: https://git.openjdk.org/jdk/pull/13497#issuecomment-1521007212
PR Review Comment: https://git.openjdk.org/jdk/pull/13497#discussion_r1175910048
PR Review Comment: https://git.openjdk.org/jdk/pull/13497#discussion_r1175910110
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
On Mon, 17 Apr 2023 16:56:31 GMT, Jiangli Zhou wrote: > - Make functions 'static' when feasible: > - throwByName() in > src/java.security.jgss/share/native/libj2gss/NativeUtil.c. > - throwByName(), throwIOException() and throwNullPointerException() in > src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c. > - throwByName() in > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c. > - throwOutOfMemoryError() in > src/java.smartcardio/share/native/libj2pcsc/pcsc.c. > - Move throwDisconnectedRuntimeException() to > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only > used in the file. Make it static. > - Move throw_internal_error() to > src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as > it's only used in the file. Make it static. > > - Rename functions by following the existing naming usages in different > libraries code: > - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss. > - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in > libj2pks11. > - Rename throwNullPointerException() to p11ThrowNullPointerException() in > libj2pks11. > - Rename throwIOException() to p11ThrowIOException() in libj2pks11. > - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException() > in libj2pks11. This function only exists in libj2pks11. The rename is done so > the function naming is consistent with the other throw functions. > > - Remove throw_internal_error() from > src/java.management/share/native/libmanagement/management.h and > src/java.management/share/native/libmanagement/management.c. It's not used. > - Remove throw_internal_error() from > src/jdk.management/share/native/libmanagement_ext/management_ext.h and > src/jdk.management/share/native/libmanagement_ext/management_ext.c. Gentle ping, please help review this PR. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/13497#issuecomment-1520559203
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
On Wed, 19 Apr 2023 11:01:07 GMT, Jaikiran Pai wrote: > Even if it doesn't get enrolled in the pre-submit testing, I think the fact > that there will be a build within the JDK which can catch these issues is a > good thing. It might catch the issue(s) after the integration, but I think > it's still a good improvement to have these issues identified by running a > specific variant of the JDK build process. Agreed, thanks. - PR Comment: https://git.openjdk.org/jdk/pull/13497#issuecomment-1514918976
Re: RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
On Mon, 17 Apr 2023 16:56:31 GMT, Jiangli Zhou wrote: > - Make functions 'static' when feasible: > - throwByName() in > src/java.security.jgss/share/native/libj2gss/NativeUtil.c. > - throwByName(), throwIOException() and throwNullPointerException() in > src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c. > - throwByName() in > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c. > - throwOutOfMemoryError() in > src/java.smartcardio/share/native/libj2pcsc/pcsc.c. > - Move throwDisconnectedRuntimeException() to > src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only > used in the file. Make it static. > - Move throw_internal_error() to > src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as > it's only used in the file. Make it static. > > - Rename functions by following the existing naming usages in different > libraries code: > - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss. > - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in > libj2pks11. > - Rename throwNullPointerException() to p11ThrowNullPointerException() in > libj2pks11. > - Rename throwIOException() to p11ThrowIOException() in libj2pks11. > - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException() > in libj2pks11. This function only exists in libj2pks11. The rename is done so > the function naming is consistent with the other throw functions. > > - Remove throw_internal_error() from > src/java.management/share/native/libmanagement/management.h and > src/java.management/share/native/libmanagement/management.c. It's not used. > - Remove throw_internal_error() from > src/jdk.management/share/native/libmanagement_ext/management_ext.h and > src/jdk.management/share/native/libmanagement_ext/management_ext.c. Hi Jaikiran, Thanks for looking into this. > is there some way these issues can be caught when code changes are being > reviewed in future? Or would this require explicit run of some tool to > identify these issues? The symbol issues are reported as linker errors when statically linking the executable with JDK native libraries and libjvm. I just created a branch with the preliminary makefile changes for optionally statically linking JDK, which can be used to demonstrate/identify the issues: https://github.com/jianglizhou/jdk/pull/new/JDK-8303796 The branch is based on our prototype on JDK 11, but mostly reworked. It re-uses the existing JDK make infrastructure based on the feedback from Severin Gehwolf in https://bugs.openjdk.org/browse/JDK-8303796. Still need to clean up the makefile changes before making it ready for review. Early feedback/input is also welcomed. > Once that work is done and integrated in the JDK build, would that catch > issues like these across different components of the JDK, before such changes > get merged in? Yes, one option would be to include the JDK static build in pre-submit testing, which can catch any changes breaking the build. That would need to be discussed broadly with other members in OpenJDK. - PR Comment: https://git.openjdk.org/jdk/pull/13497#issuecomment-1513574228
RFR: 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries
- Make functions 'static' when feasible: - throwByName() in src/java.security.jgss/share/native/libj2gss/NativeUtil.c. - throwByName(), throwIOException() and throwNullPointerException() in src/java.smartcardio/unix/native/libj2pcsc/pcsc_md.c. - throwByName() in src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c. - throwOutOfMemoryError() in src/java.smartcardio/share/native/libj2pcsc/pcsc.c. - Move throwDisconnectedRuntimeException() to src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c since it's only used in the file. Make it static. - Move throw_internal_error() to src/jdk.management/unix/native/libmanagement_ext/OperatingSystemImpl.c as it's only used in the file. Make it static. - Rename functions by following the existing naming usages in different libraries code: - Rename throwOutOfMemoryError() to gssThrowOutOfMemoryError() in libj2gss. - Rename throwOutOfMemoryError() to p11ThrowOutOfMemoryError() in libj2pks11. - Rename throwNullPointerException() to p11ThrowNullPointerException() in libj2pks11. - Rename throwIOException() to p11ThrowIOException() in libj2pks11. - Rename throwPKCS11RuntimeException() to p11ThrowPKCS11RuntimeException() in libj2pks11. This function only exists in libj2pks11. The rename is done so the function naming is consistent with the other throw functions. - Remove throw_internal_error() from src/java.management/share/native/libmanagement/management.h and src/java.management/share/native/libmanagement/management.c. It's not used. - Remove throw_internal_error() from src/jdk.management/share/native/libmanagement_ext/management_ext.h and src/jdk.management/share/native/libmanagement_ext/management_ext.c. - Commit messages: - - Fix whitespaces in p11_util.c. - 8306033: Resolve multiple definition of 'throwIOException' and friends when statically linking with JDK native libraries Changes: https://git.openjdk.org/jdk/pull/13497/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13497=00 Issue: https://bugs.openjdk.org/browse/JDK-8306033 Stats: 162 lines in 25 files changed: 17 ins; 28 del; 117 mod Patch: https://git.openjdk.org/jdk/pull/13497.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13497/head:pull/13497 PR: https://git.openjdk.org/jdk/pull/13497
Integrated: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries
On Wed, 12 Apr 2023 23:35:02 GMT, Jiangli Zhou wrote: > Rename 'jmm_' to 'jmm__management_ext' > in libmanagement_ext to resolve related linker errors when statically linking > with both libmanagement and libmanagement_ext. This pull request has now been integrated. Changeset: 314bad36 Author: Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/314bad36135c6404b31a41efc48954cb5b7877fd Stats: 31 lines in 7 files changed: 3 ins; 0 del; 28 mod 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries Reviewed-by: dholmes - PR: https://git.openjdk.org/jdk/pull/13451
Re: RFR: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries [v2]
On Fri, 14 Apr 2023 05:20:48 GMT, David Holmes wrote: >> src/jdk.management/share/native/libmanagement_ext/management_ext.c line 34: >> >>> 32: #define ERR_MSG_SIZE 128 >>> 33: >>> 34: const JmmInterface* jmm_interface_management_ext = NULL; >> >> Can you add a comment before declaring the two "exported" symbols together: >> >> // These symbols are global in this library but need to be uniquely named to >> avoid conflicts >> // with same-named symbols in other libraries, when statically linking. >> >> Thanks. > > Oops! Sorry meant to add this comment to the declarations in the hpp file. Added comment as suggested (with minor adjustment) in management_ext.h. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/13451#discussion_r1167098349
Re: RFR: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries
On Wed, 12 Apr 2023 23:35:02 GMT, Jiangli Zhou wrote: > Rename 'jmm_' to 'jmm__management_ext' > in libmanagement_ext to resolve related linker errors when statically linking > with both libmanagement and libmanagement_ext. Thanks for the review and discussion. - PR Comment: https://git.openjdk.org/jdk/pull/13451#issuecomment-1508978901
Re: RFR: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries [v2]
> Rename 'jmm_' to 'jmm__management_ext' > in libmanagement_ext to resolve related linker errors when statically linking > with both libmanagement and libmanagement_ext. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Add comment suggested by dholmes-ora. - Changes: - all: https://git.openjdk.org/jdk/pull/13451/files - new: https://git.openjdk.org/jdk/pull/13451/files/f10e1a06..9d98f728 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13451=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13451=00-01 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13451.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13451/head:pull/13451 PR: https://git.openjdk.org/jdk/pull/13451
Re: RFR: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries
On Fri, 14 Apr 2023 00:54:22 GMT, Jiangli Zhou wrote: >>> I'm not familiar with the details of symbol scoping and linkage with >>> libraries, but I would have hoped there was a way to have symbols like this >>> shareable throughout the code that comprises the library without exposing >>> them to users of the library. It used to be IIRC only functions we listed >>> in the mapfiles were exposed, and these days we use explicit attributes to >>> export them. Is there not some equivalent thing for data? >> >> If my understanding is correct the mapfile is for exported symbols, which >> are in the export table. Those symbols can be dynamically looked up, e.g. >> via dlsym. By default, '-fvisibility=hidden' is used >> (https://github.com/openjdk/jdk/blob/master/make/common/modules/LibCommon.gmk#L41). >> The symbols are '.hidden' by default if not exported. E.g. jmm_interface is >> 'hidden' as objdump shows below. However, the linker error that we are >> seeing here for statically linking the libraries together is a different >> issue. The linker founds multiple ones in different object files, hence the >> failures. We'd have to change to use internal linkage for the affect >> variables to resolve the failure if feasible (without renaming). Please let >> me know if I'm missing anything. >> >> objdump: >> ./linux-x86_64-server-release/support/native/jdk.management/libmanagement_ext/management_ext.o: >> not a dynamic object >> SYMBOL TABLE: >> ldf *ABS* management_ext.c >> ... >> g F .text 0068 JNI_OnLoad >> 0008 g O .bss0008 .hidden jvm >> *UND* JVM_GetManagement >> 0010 g O .bss0008 .hidden jmm_interface >> g O .bss0004 .hidden jmm_version > >> If we were to do this then we should have a naming convention of some kind >> e.g. `_` but it strikes me as wrong as the code >> shouldn't need to know what library it is part of. In this case we do >> something as a simple point-fix, but to me it says there is a bigger problem. > > Using a naming convention as you suggested sounds good. I'm not completely > clear what's the bigger problem though. Could you please clarify? David, I was doing some more reading on the topic for our discussion in the thread and just found this: https://stackoverflow.com/questions/61663118/avoid-multiple-definition-linker-error-when-not-using-the-redefined-symbols. It has some good information on the symbol collision issue. It describes using 'objcopy' to localize or redefine symbols. It also mentions '--allow-multiple-definitions', which I didn't know before. However, '--allow-multiple-definitions' doesn't seem to be a good approach. - PR Review Comment: https://git.openjdk.org/jdk/pull/13451#discussion_r1166209365
Re: RFR: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries
On Fri, 14 Apr 2023 00:34:14 GMT, Jiangli Zhou wrote: >>> The direct renaming in this case seems to be more strait forward. >> >> If we were to do this then we should have a naming convention of some kind >> e.g. `_` but it strikes me as wrong as the code >> shouldn't need to know what library it is part of. In this case we do >> something as a simple point-fix, but to me it says there is a bigger problem. > >> I'm not familiar with the details of symbol scoping and linkage with >> libraries, but I would have hoped there was a way to have symbols like this >> shareable throughout the code that comprises the library without exposing >> them to users of the library. It used to be IIRC only functions we listed in >> the mapfiles were exposed, and these days we use explicit attributes to >> export them. Is there not some equivalent thing for data? > > If my understanding is correct the mapfile is for exported symbols, which are > in the export table. Those symbols can be dynamically looked up, e.g. via > dlsym. By default, '-fvisibility=hidden' is used > (https://github.com/openjdk/jdk/blob/master/make/common/modules/LibCommon.gmk#L41). > The symbols are '.hidden' by default if not exported. E.g. jmm_interface is > 'hidden' as objdump shows below. However, the linker error that we are seeing > here for statically linking the libraries together is a different issue. The > linker founds multiple ones in different object files, hence the failures. > We'd have to change to use internal linkage for the affect variables to > resolve the failure if feasible (without renaming). Please let me know if I'm > missing anything. > > objdump: > ./linux-x86_64-server-release/support/native/jdk.management/libmanagement_ext/management_ext.o: > not a dynamic object > SYMBOL TABLE: > ldf *ABS* management_ext.c > ... > g F .text0068 JNI_OnLoad > 0008 g O .bss 0008 .hidden jvm > *UND* JVM_GetManagement > 0010 g O .bss 0008 .hidden jmm_interface > g O .bss 0004 .hidden jmm_version > If we were to do this then we should have a naming convention of some kind > e.g. `_` but it strikes me as wrong as the code shouldn't > need to know what library it is part of. In this case we do something as a > simple point-fix, but to me it says there is a bigger problem. Using a naming convention as you suggested sounds good. I'm not completely clear what's the bigger problem though. Could you please clarify? - PR Review Comment: https://git.openjdk.org/jdk/pull/13451#discussion_r1166165671
Re: RFR: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries
On Thu, 13 Apr 2023 23:07:23 GMT, David Holmes wrote: >> I'm not familiar with the details of symbol scoping and linkage with >> libraries, but I would have hoped there was a way to have symbols like this >> shareable throughout the code that comprises the library without exposing >> them to users of the library. It used to be IIRC only functions we listed in >> the mapfiles were exposed, and these days we use explicit attributes to >> export them. Is there not some equivalent thing for data? > >> The direct renaming in this case seems to be more strait forward. > > If we were to do this then we should have a naming convention of some kind > e.g. `_` but it strikes me as wrong as the code shouldn't > need to know what library it is part of. In this case we do something as a > simple point-fix, but to me it says there is a bigger problem. > I'm not familiar with the details of symbol scoping and linkage with > libraries, but I would have hoped there was a way to have symbols like this > shareable throughout the code that comprises the library without exposing > them to users of the library. It used to be IIRC only functions we listed in > the mapfiles were exposed, and these days we use explicit attributes to > export them. Is there not some equivalent thing for data? If my understanding is correct the mapfile is for exported symbols, which are in the export table. Those symbols can be dynamically looked up, e.g. via dlsym. By default, '-fvisibility=hidden' is used (https://github.com/openjdk/jdk/blob/master/make/common/modules/LibCommon.gmk#L41). The symbols are '.hidden' by default if not exported. E.g. jmm_interface is 'hidden' as objdump shows below. However, the linker error that we are seeing here for statically linking the libraries together is a different issue. The linker founds multiple ones in different object files, hence the failures. We'd have to change to use internal linkage for the affect variables to resolve the failure if feasible (without renaming). Please let me know if I'm missing anything. objdump: ./linux-x86_64-server-release/support/native/jdk.management/libmanagement_ext/management_ext.o: not a dynamic object SYMBOL TABLE: ldf *ABS* management_ext.c ... g F .text 0068 JNI_OnLoad 0008 g O .bss 0008 .hidden jvm *UND* JVM_GetManagement 0010 g O .bss 0008 .hidden jmm_interface g O .bss 0004 .hidden jmm_version - PR Review Comment: https://git.openjdk.org/jdk/pull/13451#discussion_r1166157315
Re: RFR: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries
On Thu, 13 Apr 2023 20:04:15 GMT, Jiangli Zhou wrote: >> src/jdk.management/share/native/libmanagement_ext/management_ext.c line 36: >> >>> 34: const JmmInterface* jmm_interface_management_ext = NULL; >>> 35: static JavaVM* jvm = NULL; >>> 36: jint jmm_version_management_ext = 0; >> >> Is there not a way to stop these from being exported from the library, as I >> assume they are not actually intended for external use. ?? > >> Is there not a way to stop these from being exported from the library, as I >> assume they are not actually intended for external use. ?? > > Good question. > > We could make those as weak symbols as long as there is no concern about > portability. In our current prototype on JDK 11, we use weak symbol to help > detect if we are dealing with a statically linked binary at runtime then > handle some of the cases conditionally (dynamic vs. static). Around the end > of last year, I became aware that there could be issues in some cases on > macos with weak symbols (e.g. without '-undefined dynamic_lookup'). I'm > planning to look into alternatives (besides using weak symbol) when porting > the support to the latest OpenJDK code base. > > Another approach is using 'objcopy' > (https://web.mit.edu/gnu/doc/html/binutils_4.html) to localize the > problematic symbols in the object file. It was suggested by our C++ > colleague. We used that to hide symbols in libfreetype and libharfbuzz (there > were problems when user code requires its own specific statically linked > libfreetype and libharfbuzz due to versioning difference). So we first > partially link all .o for the particular native library (in JDK) to form one > .o file, then run 'objcopy' to localize the symbols. That hides the affected > symbols during final link time. We had to do that since we don't control > libfreetype and libharfbuzz. It worked for our specific case (for linux). > It's probably not a general solution. > > The direct renaming in this case seems to be more strait forward. Any > thoughts? Forgot to mention that using 'static' effectively resolves the symbol issue when feasible, like the 'jvm' variable case. That doesn't work for the 'jmm_interface' and 'jmm_version' ... - PR Review Comment: https://git.openjdk.org/jdk/pull/13451#discussion_r1166010486
Re: RFR: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries
On Thu, 13 Apr 2023 06:56:46 GMT, David Holmes wrote: > Is there not a way to stop these from being exported from the library, as I > assume they are not actually intended for external use. ?? Good question. We could make those as weak symbols as long as there is no concern about portability. In our current prototype on JDK 11, we use weak symbol to help detect if we are dealing with a statically linked binary at runtime then handle some of the cases conditionally (dynamic vs. static). Around the end of last year, I became aware that there could be issues in some cases on macos with weak symbols (e.g. without '-undefined dynamic_lookup'). I'm planning to look into alternatives (besides using weak symbol) when porting the support to the latest OpenJDK code base. Another approach is using 'objcopy' (https://web.mit.edu/gnu/doc/html/binutils_4.html) to localize the problematic symbols in the object file. It was suggested by our C++ colleague. We used that to hide symbols in libfreetype and libharfbuzz (there were problems when user code requires its own specific statically linked libfreetype and libharfbuzz due to versioning difference). So we first partially link all .o for the particular native library (in JDK) to form one .o file, then run 'objcopy' to localize the symbols. That hides the affected symbols during final link time. We had to do that since we don't control libfreetype and libharfbuzz. It worked for our specific case (for linux). It's probably not a general solution. The direct renaming in this case seems to be more strait forward. Any thoughts? - PR Review Comment: https://git.openjdk.org/jdk/pull/13451#discussion_r1165975548
RFR: 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries
Rename 'jmm_' to 'jmm__management_ext' in libmanagement_ext to resolve related linker errors when statically linking with both libmanagement and libmanagement_ext. - Commit messages: - 8305935: Resolve multiple definition of 'jmm_' when statically linking with JDK native libraries Changes: https://git.openjdk.org/jdk/pull/13451/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13451=00 Issue: https://bugs.openjdk.org/browse/JDK-8305935 Stats: 28 lines in 7 files changed: 0 ins; 0 del; 28 mod Patch: https://git.openjdk.org/jdk/pull/13451.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13451/head:pull/13451 PR: https://git.openjdk.org/jdk/pull/13451
Integrated: 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries
On Sat, 8 Apr 2023 01:11:08 GMT, Jiangli Zhou wrote: > Rename various 'jvm' variables to 'jvm_' to avoid duplicate symbol > problems when statically linking the launcher executable with JDK native > libraries. This pull request has now been integrated. Changeset: ce4b9955 Author: Jiangli Zhou URL: https://git.openjdk.org/jdk/commit/ce4b9955568100d6b315336321ff8903b703f19e Stats: 34 lines in 5 files changed: 0 ins; 0 del; 34 mod 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries Reviewed-by: alanb, kevinw - PR: https://git.openjdk.org/jdk/pull/13397
Re: RFR: 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries [v2]
On Mon, 10 Apr 2023 19:38:18 GMT, Jiangli Zhou wrote: >> Rename various 'jvm' variables to 'jvm_' to avoid duplicate symbol >> problems when statically linking the launcher executable with JDK native >> libraries. > > Jiangli Zhou has updated the pull request incrementally with one additional > commit since the last revision: > > Update management.c and management_ext.c to define 'jvm' as static. Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/13397#issuecomment-1503553861
Re: RFR: 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries [v2]
> Rename various 'jvm' variables to 'jvm_' to avoid duplicate symbol > problems when statically linking the launcher executable with JDK native > libraries. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Update management.c and management_ext.c to define 'jvm' as static. - Changes: - all: https://git.openjdk.org/jdk/pull/13397/files - new: https://git.openjdk.org/jdk/pull/13397/files/0fa6a4b3..40b1a82a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13397=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13397=00-01 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13397.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13397/head:pull/13397 PR: https://git.openjdk.org/jdk/pull/13397
Re: RFR: 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries
On Mon, 10 Apr 2023 13:52:39 GMT, Alan Bateman wrote: >> Rename various 'jvm' variables to 'jvm_' to avoid duplicate symbol >> problems when statically linking the launcher executable with JDK native >> libraries. > > src/java.management/share/native/libmanagement/management.c line 36: > >> 34: const JmmInterface* jmm_interface = NULL; >> 35: JavaVM* jvm_management = NULL; >> 36: jint jmm_version = 0; > > Is there any reason why these field can't be static? Thanks. My understanding is that 'static' gives internal linkage. The static variable is limited to the scope of the translate unit that declares it. It seems to be okay to use 'static' for the 'jvm' variables in [management.c](https://github.com/openjdk/jdk/pull/13397/files/0fa6a4b3984d91c124ee2adb9d6e1facdc63c156#diff-1717ac36c4bbefab688a4e75104417bec3687f78108096c2cca3af4ee552ab11) and [management_ext.c](https://github.com/openjdk/jdk/pull/13397/files#diff-0fa91a6686c9e5dc77bdef6981235785524108950075e58d2004853dc66e1977) to resolve the symbol issue. It's problematic for the usages in jdk.crypto.cryptoki code. I'll change management.c and management_ext.c to define 'jvm' as 'static' as suggested. - PR Review Comment: https://git.openjdk.org/jdk/pull/13397#discussion_r1162008444
RFR: 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries
… with JDK native libraries - Commit messages: - 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries Changes: https://git.openjdk.org/jdk/pull/13397/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13397=00 Issue: https://bugs.openjdk.org/browse/JDK-8305761 Stats: 36 lines in 5 files changed: 0 ins; 0 del; 36 mod Patch: https://git.openjdk.org/jdk/pull/13397.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13397/head:pull/13397 PR: https://git.openjdk.org/jdk/pull/13397
Withdrawn: 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries
On Fri, 7 Apr 2023 23:32:46 GMT, Jiangli Zhou wrote: > Rename various 'jvm' variables to 'jvm_' to avoid duplicate symbol > problems when statically linking the launcher executable with JDK native > libraries. > > 8305761: Resolve multiple definition of 'jvm' when statically linking with > JDK native libraries This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/13396
RFR: 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries
Rename various 'jvm' variables to 'jvm_' to avoid duplicate symbol problems when statically linking the launcher executable with JDK native libraries. 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries - Commit messages: - Resolve linker failure due to multiple definition of 'jvm' when statically linking with JDK native libraries. Changes: https://git.openjdk.org/jdk/pull/13396/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13396=00 Issue: https://bugs.openjdk.org/browse/JDK-8305761 Stats: 130 lines in 5 files changed: 82 ins; 0 del; 48 mod Patch: https://git.openjdk.org/jdk/pull/13396.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13396/head:pull/13396 PR: https://git.openjdk.org/jdk/pull/13396
