8328614: hsdis: dlsym can't find decode symbol

[robehn]

Hi, please consider.

8327045 hide these symbols.
Tested with gcc and clang, and llvm and binutils backend.

I didn't find any use of the "DLL_ENTRY", so I removed it.

Thanks, Robbin

---

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-8328614: hsdis: dlsym can't find decode symbol (Bug - P3)

Reviewers

• Magnus Ihse Bursie (@magicus - Reviewer)
• Ludovic Henry (@luhenry - Committer)
• Hamlin Li (@Hamlin-Li - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18400

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back rehn! 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]

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

8328614: hsdis: dlsym can't find decode symbol

Reviewed-by: ihse, luhenry, mli

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

• 71c5bbc: 8329527: Opcode.IFNONNULL.primaryTypeKind() is not ReferenceType
• 58e39c1: 8329884: Serial: Fix build failure due to ‘Copy’ has not been declared
• 19a99d0: 8326744: Class-File API transition to Second Preview
• b4dddde: 8329840: Fix ZPhysicalMemorySegment::_end type
• dfaf11a: 8329898: Revert one use of markWord.is_unlocked back to is_neutral
• fbc1e66: 8328181: C2: assert(MaxVectorSize >= 32) failed: vector length should be >= 32
• a887fd2: 8316991: Reduce nullable allocation merges
• e702646: 8200566: DistributionPointFetcher fails to fetch CRLs if the DistributionPoints field contains more than one DistributionPoint and the first one fails
• 7e5ef79: 8323116: [REDO] Computational test more than 2x slower when AVX instructions are used
• 9467720: 8329875: Serial: Move preservedMarks.inline.hpp to serialFullGC.cpp
• ... and 253 more: https://git.openjdk.org/jdk/compare/eebcc2181fe27f6aa10559233c7c58882a146f56...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]

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

• build
• hotspot-compiler

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]

Webrevs

• 02: Full - Incremental (b4003e9f)
• 01: Full - Incremental (28862745)
• 00: Full (65e3cb17)

[robehn]

@magicus is this how you want us to export these symbols?

[magicus]

is this how you want us to export these symbols?

Close but no cigar. :-)

Use JNIEXPORT instead, that is properly defined for this purpose and works on all compilers. You will need to also add:

#include "jni.h"

If this is not picked up correctly, let me know and I'll help you get the include paths correctly in the build.

[magicus]

Also, apologies for forgetting to check hsdis when I changed the visibility. :-(

[robehn]

is this how you want us to export these symbols?

Close but no cigar. :-)

Use JNIEXPORT instead, that is properly defined for this purpose and works on all compilers. You will need to also add:

#include "jni.h"

If this is not picked up correctly, let me know and I'll help you get the include paths correctly in the build.

It's stand alone library, should we really make it dependent on the JDK?

[magicus]

Unless you start calling JDK functions, you will not make a program less stand-alone by including jni.h. In this case, you will only use a compile-time definition.

Ideally, we should have had more general EXPORT definitions separate from the rest of the JNI code, but someone started doing things that way, and well, here we are, 25 years later and now JNIEXPORT is everywhere in the JDK source base. :( I'd say that access to JNIEXPORT is about 50% of the reason jni.h is included...

[theRealAph]

is this how you want us to export these symbols?

Close but no cigar. :-)
Use JNIEXPORT instead, that is properly defined for this purpose and works on all compilers. You will need to also add:

#include "jni.h"

If this is not picked up correctly, let me know and I'll help you get the include paths correctly in the build.

It's stand alone library, should we really make it dependent on the JDK?

No. And neither should we compile or link it with "-fvisibility=hidden". That is the root of
this problem.

[magicus]

And neither should we compile or link it with "-fvisibility=hidden". That is the root of this problem.

If you suggest that we should not compile hsdis with hidden visibility, I disagree. I have been working hard on unifying build of native libraries across the entire product, to fix holes where we have not used a consistent way of compiling and/or linking. There is no reason to tread hsdis differently. If I restore using hidden visibility as an option that all native libraries, except hsdis, must opt in to, then we are just back to square one, and suddenly someone will forget about it. Instead, now we set -fvisibility=hidden in configure so nobody can forget about it.

That was the more general argument for alinging compilation and linking flags and behavior across the product. Regarding the symbol visibility more specifically, there is also the question of consistency across platforms. On Windows, the behavior corresponding to "-fvisibility=hidden" is always turned on. Functions can only be exported if they are explicitly annotated in the source code (or specified otherwise to the linker). So we are in any case forced to export functions on Windows.

Let's have a look at this patch. Currently we have code like:

#ifdef _WIN32
__declspec(dllexport)
#endif

Robbin proposes to change this to

#if defined(_WIN32)
__declspec(dllexport)
#elif defined(_GNU_SOURCE)
__attribute__ ((visibility ("default")))
#endif

My counter-proposal was to replace it with just JNIEXPORT. Surely you can't say that is a worse solution?

[magicus]

(In fact, I think we have a problem everywhere in the code base where someone is using __declspec(dllexport) directly)

[robehn]

(In fact, I think we have a problem everywhere in the code base where someone is using __declspec(dllexport) directly)

src/java.base/share/native/libzip/zlib/zconf.h:# define ZEXPORT __declspec(dllexport)

ZEXPORT is only defined on win32, so we must build with default on linux.

[magicus]

src/java.base/share/native/libzip/zlib/zconf.h:# define ZEXPORT __declspec(dllexport)

zlib is third party source that is copied into our repo. I did not mean that what I said for that applies to it. (Also, we should not really export any functions from zlib -- we encapsulate it in our own native shared library. I might have to look into if we are doing this wrong on Windows. But that is a separate question from this PR.)

[robehn]

src/java.base/share/native/libzip/zlib/zconf.h:# define ZEXPORT __declspec(dllexport)

zlib is third party source that is copied into our repo. I did not mean that what I said for that applies to it. (Also, we should not really export any functions from zlib -- we encapsulate it in our own native shared library. I might have to look into if we are doing this wrong on Windows. But that is a separate question from this PR.)

Okay, so the difference is that me and @theRealAph consider hsdis a thrid party library, although the JDK folks maintain it. (correct me if I'm wrong)

[magicus]

It is original code in the JDK library. zlib is copied in from elsewhere. That is what I mean by third party code.

The only reason hsdis is specially treated is due to the fact that the original implementation was based on binutils, which could not -- for legal reason -- be distributed as a compiled library alongside the JDK. That forced us to erect an artificial barrier.

There are multiple JBS issues trying to rectify this, with different approaches, to get a non-binutils hsdis library build and shipped as any other native library in the JDK.

[robehn]

How would you add jni.h ?

[theRealAph]

And neither should we compile or link it with "-fvisibility=hidden". That is the root of this problem.

If you suggest that we should not compile hsdis with hidden visibility, I disagree.

Yes, that's what I would do.

I have been working hard on unifying build of native libraries across the entire product, to fix holes where we have not used a consistent way of compiling and/or linking. There is no reason to tread hsdis differently. If I restore using hidden visibility as an option that all native libraries, except hsdis, must opt in to, then we are just back to square one, and suddenly someone will forget about it. Instead, now we set -fvisibility=hidden in configure so nobody can forget about it.

OK, OK! So please can we get this fix in?

Robbin proposes to change this to

#if defined(_WIN32)
__declspec(dllexport)
#elif defined(_GNU_SOURCE)
__attribute__ ((visibility ("default")))
#endif

My counter-proposal was to replace it with just JNIEXPORT. Surely you can't say that is a worse solution?

JNIEXPORT is better, I guess, but it does mean that hsdis is no longer standalone, and IMO it should have a pathological dependency on some JVM header file. That's the problem here.

But really, whatever works is good with me. The last thing I want to do is delay this fix any further.

Robbin has asked "How would you add jni.h ?" Is anyone going to answer?

[magicus]

Ideally, it should just be enough to add #import "jni.h". This will definitely work for capstone and llvm. However, for binutils we disable our standard C include paths, so this will fail. (I'm not sure if this is really necessary, but that is how we currently do it)

To solve it you can either set HSDIS_TOOLCHAIN_DEFAULT_CFLAGS to true for binutils, or you can add the include path to jni.h directly. The former would be the best, but in case that does not work, try adding java.base:include to EXTRA_HEADER_DIRS.

[magicus]

I apologize for the late reply. I've been just working spotty hours due to spring break.

[robehn]

Thanks, updated. (Since windows build seems to need HSDIS_TOOLCHAIN_DEFAULT_CFLAGS, just adding java.base:include seemed a good easy way)

[magicus]

Since windows build seems to need HSDIS_TOOLCHAIN_DEFAULT_CFLAGS

Riiiight. That is since we use a completely different compiler, gcc instead of cl. (Which is probably the worst hack in all of the JDK build system...)

[magicus]

Looks good to me now. Thanks for indulging me in doing it this way. :-)

[TheShermanTanker]

Since windows build seems to need HSDIS_TOOLCHAIN_DEFAULT_CFLAGS

Riiiight. That is since we use a completely different compiler, gcc instead of cl. (Which is probably the worst hack in all of the JDK build system...)

I plan on adding support for using gcc to compile hsdis on Windows sometime soon (Like proper support for gcc as a toolchain on Windows, at least enough to support hsdis, so people can freely use any gcc instead of only being restricted to Cygwin), what might be an elegant solution to this issue, if you have any in mind?

[magicus]

I plan on adding support for using gcc to compile hsdis on Windows sometime soon (Like proper support for gcc as a toolchain on Windows, at least enough to support hsdis, so people can freely use any gcc instead of only being restricted to Cygwin), what might be an elegant solution to this issue, if you have any in mind?

The proper solution is, I think, to encapsulate toolchain variables/definitions into a package, and update the build system to not assume anything about toolchains. In a proper language, I'd create like a class Toolchain with everything the system would need to know to use a toolchain, and then pass a Toolchain toolchain to all invocations that need to do anything with a toolchain. Now this is not possible with make, but I hope it can be simulated using "fake namespaces" and something like a $(TOOLCHAIN)_COMPILER kind of setup.

That would require some major surgery of the build system, but I think it will come out better in the end.

After that is done, fixing so that hsdis/binutils can be compiled with gcc on Windows will be a trivial side-effect.

[theRealAph]

I apologize for the late reply. I've been just working spotty hours due to spring break.

I apologize for my bad temper. 😀

Thanks to everyone working on this. I still think that hsdis ought not to have any dependencies on HotSpot, but I'm not going to be fanatical about it.

[robehn]

Thanks to everyone working on this. I still think that hsdis ought not to have any dependencies on HotSpot, but I'm not going to be fanatical about it.

I agree, good :)

If everyone is 'okay' with this, can someone do the second review?

[robehn]

Thank you @luhenry !

[magicus]

FYI: The 2-reviewer rule only applies to Hotspot (and some other specific parts of the code base), and not generally for JDK fixes. So you had been fine to push with just one reviewer.

[robehn]

Yea, I know, but if hsdis is not an external library I considered it part of hotspot as it is the only use, hence I want two reviews. :)

[Hamlin-Li]

Looks good.
Although I'm not the right person to review this, thanks for explanation and discussion.

[robehn]

/integrate

[openjdk]

Going to push as commit 1e02a13.
Since your change was applied there have been 279 commits pushed to the master branch:

• 23d161d: 8328630: Add logging when needed symbols in dll are missing.
• 2e925f2: 8329726: Use non-short forward jumps in lightweight locking
• e75e1cb: 8329955: Class-File API ClassPrinter does not print bootstrap methods arguments
• f9bc2db: 8325371: Missing ClassFile.Option in package summary
• a8fbeec: 8329956: G1: Remove unimplemented collection_set_candidate_short_type_str
• 3b6629c: 8324673: javacserver failed during build: RejectedExecutionException
• 5fb5e6c: 8329603: G1: Merge G1BlockOffsetTablePart into G1BlockOffsetTable
• 2fcb816: 8305072: Win32ShellFolder2.compareTo is inconsistent
• 635cb3c: 8329745: Update the documentation of ServerSocket and Socket to refer to StandardSocketOptions instead of legacy SocketOptions
• 5c9f036: 8329858: G1: Make G1VerifyLiveAndRemSetClosure stateless
• ... and 269 more: https://git.openjdk.org/jdk/compare/eebcc2181fe27f6aa10559233c7c58882a146f56...master

Your commit was automatically rebased without conflicts.

[openjdk]

@robehn Pushed as commit 1e02a13.

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