-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8311661: Resolve duplicate symbol of StringTable::StringTable with JDK static linking #14808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back jiangli! A progress list of the required criteria for merging this PR into |
|
@jianglizhou The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine with me. We don't seem to have a namespace name convention in the style guide (was expecting lower case). But this would be an appropriate name to move SymbolTable to also if need be.
|
@jianglizhou This change is no longer ready for integration - check the PR body for details. |
Thanks for the review, Coleen. |
|
The JBS issue says: "That avoids having to fix the application or libraries code, which may not be practical." but I'm not sure how changing hotspot every time an application defines a conflicting symbol is more practical? This patch seems like a band-aid to make one particular application work. I think a more principled solution is needed that avoids symbol conflicts going forward, if we are serious about supporting static linking of hotspot. For instance, the symbols that should be exported from hotspot are found in several files in make/data/hotspot-symbols. Is it possible to limit the symbols exported/visible in the static library to those symbols instead? |
|
How about the namespace being HotSpotClassfile (StringTable is in the classfile directory) |
|
@JornVernee thanks for looking into this.
Changing the application or libraries code is not practical in this case as we don't have the full control of what applications or libraries may be linked with. In some cases, developers may not have access to the actual code or required insights to resolve the linkage failures. Fixing the hotspot code using namespace in this case resolves the issue from the source at once, which makes this as a more principled approach.
The linking failure involved with duplicate symbol issue concerns internal linkage vs. global linkage, which is different from symbol exporting, IIUC. When duplicating symbol with global linkages from different compilation units linked together, it causes the linking failure. On the other hand, exporting a symbol putting the symbol in the export table, so the symbol can be dynamically looked up. Please see some detailed discussions in: |
I did some search and |
I'm not suggesting that the application code should be changed. I'm suggesting that a broader solution should be found that avoids this issue in the future, rather than having to patch hotspot again for the next application that comes along and defines a symbol that conflicts. That's what I mean by "principled". I.e. we should be able to explain how to avoid symbol conflicts when statically linking applications against hotspot. I don't think that telling users that, if they have a symbol conflict: "just file a bug report and we will patch hotspot to use a different symbol" is good enough. For instance, we could put all of hotspot into a
Thanks for the pointers. So it sounds like limiting the symbols 'exported' by a static library is not really possible. |
Thanks for the clarification, @JornVernee.
Agreed.
I think using a hotspot special namespace can be a good general solution for resolving and avoiding any potential duplication symbol issue like this. The good news is that with our prototype and testing on JDK 11, we didn't find many duplicate symbol issues with hotspot code specifically. So far we only observed issues with:
Right. We've looked into following solutions for linkage failures due to symbol conflicts and each solution may be used in different cases when applicable:
|
I'll change the namespace from |
|
What solutions do you have to resolve "Thread" and "ProfileData" ? |
For the |
|
|
Ok, yeah, I'd hate for Thread to get a namespace:: prepended to the million uses of it. Maybe the same approach should be used for StringTable too, since they're solving the same problem. Is there an #if STATIC_LINK or something that surrounds this #define? |
|
If there are only 3 problematic symbols, wouldn't it be easier to configure the JDK with That way, you don't need to submit a new PR every time there's a new conflict. |
We are proposing removing |
Thanks, I haven't explored that option. Seems to be a useful quick fix when experimenting with JDK static linking. As we are working towards a longer term and more general solution, resolving the issues in hotspot code seems to be better. |
|
When you add in Thread, the namespace solution seems a lot less attractive unless you can do "using namespace hotspot;" around all the code in the JVM without altering names everywhere. Having sets of different HotSpotX namespaces seems a lot less attractive in the code and isn't really more general. So I don't think this is a good change anymore. Maybe there is a build option to wrap names as Ioi suggested or some global namespace wrapping. But then we'd just want to wrap the whole thing in namespace hotspot. This code was written before namespaces existed everywhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've changed my mind. Since static build is a build time issue, then maybe a source solution shouldn't be done for this.
| (has_dcmd_notification_event = (!UseNotificationThread && DCmdFactory::has_pending_jmx_notification())) | | ||
| (stringtable_work = StringTable::has_work()) | | ||
| (stringtable_work = JavaClassFile::StringTable::has_work()) | | ||
| (symboltable_work = SymbolTable::has_work()) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does look strange since you'd expect SymbolTable to be in the same namespace.
|
Hi, to prevent clashes like this, libraries that support static linking tend to define a global namespace or a common prefix, usually switchable via defines. IIRC sqlite does this, zlib, jemalloc... If we could pull such a thing off with a minimum of invasiveness, it would be a much more robust solution. If we introduce such a namespace, the name could be very short and non-descriptive, and then be defined (or completely switched off by default) via compile option. That way one could even link two hotspots if one wanted (only one usuable at a time for other reasons). If combined with "using" somewhere, maybe in precompiled.hpp, this solution may not be that invasive. Cheers, Thomas |
Do the libraries that you mention just enclose every .cpp and .hpp files with That doesn't look too bad to me, as we just need to do this once, and it can (probably) be done with a script.
Are you suggesting something like this? I tried it but couldn't get it to work. The |
|
(From: https://en.cppreference.com/w/cpp/language/namespace) But, if we put everything in the same |
Both zlib and jemalloc achieve name space isolations via #define, although not in C++ namespace definition. I think that's what @tstuefe was referring to.
Global namespace (as @JornVernee has suggested) does sound attractive as long as we can do it without very invasive changes. It's also a more future-proof solution. I'll do some exploration as well. |
Something like this might work. We need to enclose all declarations inside We can add the
|
|
I think we already used -fvisibility=hidden to solve problems like this. |
Right, the |
|
Does passing -Wl,--exclude-libs,ALL to gcc work for static libs? |
I just looked briefly. For |
If adding |
I just did a quick test using |
|
I found a way to hide the unwanted symbols in libjvm.a. This requires So potentially we can do this completely in the makefiles, without adding namespaces to HotSpot. |
Yeah, |
Moving to the namespace incrementally seems to be reasonable to me. Will let others to chime in on this for their thoughts. Add C++ references that I read discourages using
|
…in this PR thread. The namespace is 'hotspot_jvm', which may have less chance of collisions than 'hotspot'. - Use 'using' directive in precompiled.hpp, as suggested by others in the PR comments.
src/hotspot/share/classfile/javaClasses.hpp also needs change, otherwise it fails to build on windows. I updated the PR with suggestions incorporated. Please take a look of the updated version, thanks. |
|
Hi Jiangli, Putting the HotSpot code in a namespace is a fundamental change that should be done in a JEP. The idea of using namespaces for HotSpot has been around for a while, but so far there hasn't been a strong motivation for doing it. Perhaps static linking would finally give us a go-ahead reason. The JEP should discuss the goals, design choices, risks and alternatives. For example:
The hotspot-dev mailing list would be the best place to have such discussions. Also, at this time, many OpenJDK developers who are interested on this topic are on vacation, so we probably need to wait till the end of the summer so everyone has a chance to chime in. JEP may feel like a lot of process, but for this change, I think it's the best avenue for exploration. Thanks |
I also like the incremental namespace solution. @iklam wrote:
I agree with @iklam that we should discuss this on hotspot-dev. It will affect the whole JDK and constitute a change that is visible from outside, so we should get this right. I also think that making the hotspot linkable in a static way could be very useful. |
Thanks for the response, @iklam. Going through the JEP for the hotspot namespace makes a lot of sense. Waiting until the end of summer also sounds good to me. Maybe it's still early to ask the question, would you be willing to drive the JEP for the namespace changes? I'm happy to write up the JEP otherwise. |
I'll be happy to facilitate the discussion for this topic, but I am really not an expert on C++ namespaces. Also, I think such a JEP should be led by someone who has an interest in putting HotSpot in namespaces. |
|
I am concerned that something that JDK-8303796 claimed to be a small enhancement is in fact requiring a lot of changes in different areas of the codebase. There are already a number of "duplicate symbol definition" issues that have been filed and fixed in an ad-hoc manner in the JDK, which is the kind of whack-a-mole approach that we should not be taking. If static linking requires symbol isolation then a generally applicable approach to doing that should be investigated (in a separate project? Leyden?) and then brought to mainline via a JEP (not just for the hotspot aspect as currently suggested). |
@dholmes-ora, sorry for the slow response, I was OOO. The fixes and enhancements for JDK-8303796 include:
The overall changes are not small. Individually each fix or enhancement specifically the symbol fix is relatively small and non-complicated (except the built-in libraries lookup changes). Handling them as separate bugs and enhancements seems to be reasonable. The built-in library lookup and runtime changes may be suitable for a new JEP or putting in a Leyden project repo and propose later. It's built on top of the existing JEP 178: Statically-Linked JNI Libraries. To me, handling it with a new JEP as an enhancement on top of JEP 178 is reasonable. Any thoughts on that?
The duplicated symbol issues consist three parts:
|
|
@jianglizhou This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@jianglizhou This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
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. 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. |
Additional discussions in #17456 (comment) thread |
Move StringTable to 'hotspot_jvm' namespace.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14808/head:pull/14808$ git checkout pull/14808Update a local copy of the PR:
$ git checkout pull/14808$ git pull https://git.openjdk.org/jdk.git pull/14808/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14808View PR using the GUI difftool:
$ git pr show -t 14808Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14808.diff
Webrev
Link to Webrev Comment