Skip to content
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

8264047: Duplicate global variable 'jvm' in libjavajpeg and libawt #3155

Closed

Conversation

@jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Mar 23, 2021

The suggestion is to rename 'jvm' variable in libjavajpeg to the_jvm so this conflict no longer occurs when libjavajpeg.a and libawt.a are being linked into one native image.

Testing: test/jdk/javax/imageio jtreg tests. GHA pre-integration tests running too.

Thoughts?


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264047: Duplicate global variable 'jvm' in libjavajpeg and libawt

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3155/head:pull/3155
$ git checkout pull/3155

Update a local copy of the PR:
$ git checkout pull/3155
$ git pull https://git.openjdk.java.net/jdk pull/3155/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3155

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3155.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 23, 2021

👋 Welcome back sgehwolf! 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 openjdk bot added the rfr label Mar 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2021

@jerboaa The following label will be automatically applied to this pull request:

  • 2d

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the 2d label Mar 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 23, 2021

Webrevs

@jerboaa
Copy link
Contributor Author

@jerboaa jerboaa commented Mar 25, 2021

Anyone?

@prrace
Copy link
Contributor

@prrace prrace commented Mar 25, 2021

Anyone?

I guess I don't understand why this is the right solution.
If the symbol -in both cases - is made internal won't that be better ? I can't think why it needs to be exported.
Is the AWT one being used by some other client library ? If so may be that other library needs its own.
And calling it "jvm" is common so are these the only two ?

@jerboaa
Copy link
Contributor Author

@jerboaa jerboaa commented Mar 26, 2021

Anyone?

I guess I don't understand why this is the right solution.

Thanks for the feedback. I'm open to other approaches. I'm by no means an expert in this area, so you probably know better what the best solution would be.

If the symbol -in both cases - is made internal won't that be better ? I can't think why it needs to be exported.

I don't know if that'll help the static linking case, I'll give it a try.

Is the AWT one being used by some other client library ? If so may be that other library needs its own.

I don't think it is. libawt.so and libjavajpeg.so have each their own. The problem starts when static libraries of them libawt.a and libjavajpeg.a get linked into one executable. Then we end up with a name clash on the jvm symbol. See the bug for some details.

And calling it "jvm" is common so are these the only two ?

Apparently so. At least no other conflicts were noticed when the following client libraries are in the mix:

libjavajpeg.a
liblcms.a
libfontmanager.a
libawt_headless.a
libawt.a
libharfbuzz.a

@prrace
Copy link
Contributor

@prrace prrace commented Mar 26, 2021

The list of libraries doesn't seem to include xawt. Although I'd be astonished if it defined one since it links with awt.
But it also suggests you are only looking at one platform and perhaps that is because the problem is very specific
to your toolchain ? Actually why are you seeing it now and it has not been a problem before ?

@jerboaa
Copy link
Contributor Author

@jerboaa jerboaa commented Mar 26, 2021

The list of libraries doesn't seem to include xawt.

Yes. This is in context of Graal VM's native images. AFAIK there is no full xawt support yet. This is about the conflict of libawt and libjavajpeg. There are probably more.

But it also suggests you are only looking at one platform and perhaps that is because the problem is very specific
to your toolchain ? Actually why are you seeing it now and it has not been a problem before ?

Yes. This is on Linux x86_64 and with an OpenJDK built with GCC 10+. With gcc 10+ -fno-common is default:
https://gcc.gnu.org/gcc-10/porting_to.html

So the libjavajpeg.a, and libawt.a got compiled with -fno-common. Then when being linked into an executable together, the linker screams with this:

/usr/bin/ld: /disk/graal/upstream-sources/graal/mandrel-build/lib/libawt.a(awt_LoadLibrary.o):/disk/openjdk/upstream-sources/openjdk-11-dev/src/java.desktop/unix/native/libawt/awt/awt_LoadLibrary.c:52: multiple definition of `jvm'; /disk/graal/upstream-sources/graal/mandrel-build/lib/libjavajpeg.a(jpegdecoder.o):/disk/openjdk/upstream-sources/openjdk-11-dev/src/java.desktop/share/native/libjavajpeg/jpegdecoder.c:67: first defined here
collect2: error: ld returned 1 exit status 

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 27, 2021

Probably it will be easy to remove this "jvm" variable in the jpeg library? Looks like it is used to call JNU_GetEnv, but the JNIEnv could be accessed from the first parameter of the jni method.

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 27, 2021

Probably it will be easy to remove this "jvm" variable in the jpeg library? Looks like it is used to call JNU_GetEnv, but the JNIEnv could be accessed from the first parameter of the jni method.

Seems these methods are used as callbacks...

@jerboaa
Copy link
Contributor Author

@jerboaa jerboaa commented Mar 29, 2021

So what's the verdict with this one? OK to rename the variable or go a different route?

mrserb
mrserb approved these changes Mar 31, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 31, 2021

@jerboaa 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:

8264047: Duplicate global variable 'jvm' in libjavajpeg and libawt

Reviewed-by: serb

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 161 new commits pushed to the master branch:

  • cb70ab0: 8263235: sanity/client/SwingSet/src/ColorChooserDemoTest.java failed throwing java.lang.NoClassDefFoundError
  • e2ec997: 8263551: Provide shared lock-free FIFO queue implementation
  • dec3447: 8264346: nullptr_t undefined in global namespace for clang+libstdc++
  • 0fa3572: 8264489: Add more logging to LargeCopyWithMark.java
  • f43d14a: 8264396: Use the blessed modifier order in jdk.internal.jvmstat
  • 6225ae6: 8264466: Cut-paste error in InterfaceCalls JMH
  • 40c3249: 8264149: BreakpointInfo::set allocates metaspace object in VM thread
  • 999c134: 8264417: ParallelCompactData::region_offset should not accept pointers outside the current region
  • 604b14c: 8264112: (fs) Reorder methods/constructor/fields in UnixUserDefinedFileAttributeView.java
  • 9061271: 8261957: [PPC64] Support for Concurrent Thread-Stack Processing
  • ... and 151 more: https://git.openjdk.java.net/jdk/compare/f84b52b84dc7ba1921b3845ae696fa9896ba4136...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.

@openjdk openjdk bot added the ready label Mar 31, 2021
@jerboaa
Copy link
Contributor Author

@jerboaa jerboaa commented Apr 6, 2021

@mrserb Thanks for the review!

@jerboaa
Copy link
Contributor Author

@jerboaa jerboaa commented Apr 6, 2021

/integrate

@openjdk openjdk bot closed this Apr 6, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 6, 2021

@jerboaa Since your change was applied there have been 220 commits pushed to the master branch:

  • 8132548: 8264359: Compiler directives should enable DebugNonSafepoints when PrintAssembly is requested
  • ec7b002: 8264626: C1 should be able to inline excluded methods
  • ff22353: 8264565: Templatize num_arguments() functions of DCmd subclasses
  • 54b4070: 8264634: CollectCLDClosure collects duplicated CLDs when dumping dynamic archive
  • 43d4a6f: 8264564: AArch64: use MOVI instead of FMOV to zero FP register
  • dc608fd: 8264411: serviceability/jvmti/HeapMonitor tests intermittently fail due to large TLAB size
  • b1a225e: 8263565: NPE was thrown when sun.jvm.hotspot.rmi.serverNamePrefix was set
  • c41cd15: 8264686: ClhsdbTestConnectArgument.java should use SATestUtils::validateSADebugDPrivileges
  • b7baca7: 8264288: Performance issue with MethodHandle.asCollector
  • 9201899: 8264729: Random check-in failing header checks.
  • ... and 210 more: https://git.openjdk.java.net/jdk/compare/f84b52b84dc7ba1921b3845ae696fa9896ba4136...master

Your commit was automatically rebased without conflicts.

Pushed as commit eb6330e.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants