8305935: Resolve multiple definition of 'jmm_<interface|version>' when statically linking with JDK native libraries #13451

jianglizhou · 2023-04-12T23:35:02Z

Rename 'jmm_<interface|version>' to 'jmm_<interface|version>_management_ext' in libmanagement_ext to resolve related linker errors when statically linking with both libmanagement and libmanagement_ext.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8305935: Resolve multiple definition of 'jmm_<interface|version>' when statically linking with JDK native libraries

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to f10e1a06

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13451/head:pull/13451
$ git checkout pull/13451

Update a local copy of the PR:
$ git checkout pull/13451
$ git pull https://git.openjdk.org/jdk.git pull/13451/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13451

View PR using the GUI difftool:
$ git pr show -t 13451

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13451.diff

Webrev

Link to Webrev Comment

…n statically linking with JDK native libraries

bridgekeeper · 2023-04-12T23:36:05Z

👋 Welcome back jiangli! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-04-12T23:37:30Z

@jianglizhou The following labels will be automatically applied to this pull request:

jmx
serviceability

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.

mlbridge · 2023-04-12T23:41:37Z

Webrevs

01: Full - Incremental (9d98f728)
00: Full (f10e1a06)

dholmes-ora · 2023-04-13T06:56:46Z

src/jdk.management/share/native/libmanagement_ext/management_ext.c

+const JmmInterface* jmm_interface_management_ext = NULL;
 static JavaVM* jvm = NULL;
-jint jmm_version = 0;
+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' ...

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. <lib-name>_<var-name> 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:
0000000000000000 l df ABS 0000000000000000 management_ext.c
...
0000000000000000 g F .text 0000000000000068 JNI_OnLoad
0000000000000008 g O .bss 0000000000000008 .hidden jvm
0000000000000000 UND 0000000000000000 JVM_GetManagement
0000000000000010 g O .bss 0000000000000008 .hidden jmm_interface
0000000000000000 g O .bss 0000000000000004 .hidden jmm_version

If we were to do this then we should have a naming convention of some kind e.g. <lib-name>_<var-name> 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.

Thanks for the pointer to stackoverflow - that made things a lot clearer.

The symbol localization using objcopy seems to achieve what we desire but has the same drawback as renaming, that you need to know the symbols will clash to localize them. And it is also not ideal that changing the code may also need a change to the build file. :(

With that in mind a simple renaming seems the least worst option for fixing this.

dholmes-ora

One small request but otherwise okay.

In wonder if @magicus or @erikj79 have anything to add from a build perspective?

Thanks for the extended discussion on the potential options here.

dholmes-ora · 2023-04-14T05:17:28Z

src/jdk.management/share/native/libmanagement_ext/management_ext.c

@@ -31,9 +31,9 @@

 #define ERR_MSG_SIZE 128

-const JmmInterface* jmm_interface = NULL;
+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.

openjdk · 2023-04-14T05:21:14Z

@jianglizhou This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8305935: Resolve multiple definition of 'jmm_<interface|version>' when statically linking with JDK native libraries

Reviewed-by: dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 25 new commits pushed to the master branch:

30a140b: 8304912: Use OperatingSystem enum in java.desktop module
1fd4006: 8305405: Compile_lock not needed in Universe::genesis()
ebeee6d: 8305404: Compile_lock not needed for InstanceKlass::implementor()
d2ce04b: 8301496: Replace NULL with nullptr in cpu/riscv
54bf370: 8170945: Collectors$Partition should override more Map methods
0826cee: 8301495: Replace NULL with nullptr in cpu/ppc
c0c3122: 8305618: Move gcold out of tier1
3651b92: 8305944: assert(is_aligned(ref, HeapWordSize)) failed: invariant
12358e6: 8305934: PPC64: Disable VMContinuations on Big Endian
287bb06: 8305902: (cs) Resolve default Charset only once in StreamEncoder and StreamDecoder
... and 15 more: https://git.openjdk.org/jdk/compare/425ef0685c584abec80454fbcccdcc6db6558f93...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

jianglizhou · 2023-04-14T17:12:28Z

Thanks for the review and discussion.

jianglizhou · 2023-04-14T21:11:27Z

/integrate

openjdk · 2023-04-14T21:13:18Z

Going to push as commit 314bad3.
Since your change was applied there have been 28 commits pushed to the master branch:

793da60: 8305403: Shenandoah evacuation workers may deadlock
2cc4bf1: 8305085: Suppress removal warning for finalize() from test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineFinalizer.java
5a78865: 8304930: Enable Link Time Optimization as an option for Visual C++
30a140b: 8304912: Use OperatingSystem enum in java.desktop module
1fd4006: 8305405: Compile_lock not needed in Universe::genesis()
ebeee6d: 8305404: Compile_lock not needed for InstanceKlass::implementor()
d2ce04b: 8301496: Replace NULL with nullptr in cpu/riscv
54bf370: 8170945: Collectors$Partition should override more Map methods
0826cee: 8301495: Replace NULL with nullptr in cpu/ppc
c0c3122: 8305618: Move gcold out of tier1
... and 18 more: https://git.openjdk.org/jdk/compare/425ef0685c584abec80454fbcccdcc6db6558f93...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-14T21:13:40Z

@jianglizhou Pushed as commit 314bad3.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8305935: Resolve multiple definition of 'jmm_<interface|version>' whe…

f10e1a0

…n statically linking with JDK native libraries

openjdk bot added the rfr Pull request is ready for review label Apr 12, 2023

openjdk bot added serviceability serviceability-dev@openjdk.org jmx jmx-dev@openjdk.org labels Apr 12, 2023

dholmes-ora reviewed Apr 13, 2023

View reviewed changes

dholmes-ora approved these changes Apr 14, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 14, 2023

Add comment suggested by dholmes-ora.

9d98f72

openjdk bot added the integrated Pull request has been integrated label Apr 14, 2023

openjdk bot closed this Apr 14, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 14, 2023

jianglizhou mentioned this pull request Jul 10, 2023

8311661: Resolve duplicate symbol of StringTable::StringTable with JDK static linking #14808

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8305935: Resolve multiple definition of 'jmm_<interface|version>' when statically linking with JDK native libraries #13451

8305935: Resolve multiple definition of 'jmm_<interface|version>' when statically linking with JDK native libraries #13451

jianglizhou commented Apr 12, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 12, 2023

openjdk bot commented Apr 12, 2023

mlbridge bot commented Apr 12, 2023 •

edited

Loading

dholmes-ora Apr 13, 2023

jianglizhou Apr 13, 2023

jianglizhou Apr 13, 2023

dholmes-ora Apr 13, 2023

dholmes-ora Apr 13, 2023 •

edited

Loading

jianglizhou Apr 14, 2023

jianglizhou Apr 14, 2023

jianglizhou Apr 14, 2023

dholmes-ora Apr 14, 2023

dholmes-ora left a comment

dholmes-ora Apr 14, 2023

dholmes-ora Apr 14, 2023

jianglizhou Apr 14, 2023

openjdk bot commented Apr 14, 2023 •

edited

Loading

jianglizhou commented Apr 14, 2023

jianglizhou commented Apr 14, 2023

openjdk bot commented Apr 14, 2023

openjdk bot commented Apr 14, 2023

8305935: Resolve multiple definition of 'jmm_<interface|version>' when statically linking with JDK native libraries #13451

8305935: Resolve multiple definition of 'jmm_<interface|version>' when statically linking with JDK native libraries #13451

Conversation

jianglizhou commented Apr 12, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 12, 2023

openjdk bot commented Apr 12, 2023

mlbridge bot commented Apr 12, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Apr 14, 2023 • edited Loading

jianglizhou commented Apr 14, 2023

jianglizhou commented Apr 14, 2023

openjdk bot commented Apr 14, 2023

openjdk bot commented Apr 14, 2023

jianglizhou commented Apr 12, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 12, 2023 •

edited

Loading

dholmes-ora Apr 13, 2023 •

edited

Loading

openjdk bot commented Apr 14, 2023 •

edited

Loading