8331541: [i386] linking with libjvm.so fails after JDK-8283326

[vpa1977]

Hi,

The .type _SafeFetch32_impl,@function symbol declaration contains a spurious underscore causing an undefined symbol in libjvm.so.

I am not sure about change in default flags. I have removed the flag that allows linking with undefined symbols
because having the flag on might cause bugs like this one or https://bugs.openjdk.org/browse/JDK-8329983 as the build succeeds even if some symbols are not defined.
Openjdk builds successfully without it on amd64, i386, armhf, arm64, s390, ppc64el and riscv64 with this change[1]. riscv64 build was done locally inside vm:

Finished building target 'images' in configuration 'linux-riscv64-server-release'
ubuntu@ubuntu:~/jdk$ 

Unfortunately I do not know why it was introduced, so I might be missing something.
I am happy to drop it from this one or move it to a separate issue/pr.

[1] https://launchpad.net/~vpa1977/+archive/ubuntu/october-21/+sourcepub/16076564/+listing-archive-extra

---

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-8331541: [i386] linking with libjvm.so fails after JDK-8283326 (Bug - P4)

Reviewers

• Daniel Jeliński (@djelinski - Reviewer)
• Magnus Ihse Bursie (@magicus - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19048

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8331541: [i386] linking with libjvm.so fails after JDK-8283326

Reviewed-by: djelinski, ihse

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

• 230fac8: 8331941: Make CollectedHeap::parallel_object_iterator public
• c845261: 8331924: Parallel: Remove unused MutableSpace::mangle_unused_area_complete
• ad78b7f: 8331185: Enable compiler memory limits in debug builds
• aafa15f: 8331208: Memory stress test that checks OutOfMemoryError stack trace fails
• edd47c1: 8308033: The jcmd thread dump related tests should test virtual threads
• 1aebab7: 8320995: RISC-V: C2 PopCountVI
• 0eff492: 8330278: Have SSLSocketTemplate.doClientSide use loopback address
• c6f611c: 8331798: Remove unused arg of checkErgonomics() in TestMaxHeapSizeTools.java
• 0e1dca7: 8331715: Remove virtual specifiers in ContiguousSpace
• 7f29904: 8330005: RandomGeneratorFactory.getDefault() throws exception when the runtime image only has java.base module
• ... and 78 more: https://git.openjdk.org/jdk/compare/286cbf831c2eb76e31bd69b4a93cd5ae9a821493...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@djelinski, @magicus) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

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

• hotspot-runtime

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.

[mlbridge]

Webrevs

• 00: Full (18ebbd6e)

[magicus]

/label build

[openjdk]

@magicus
The build label was successfully added.

[magicus]

I'm not sure why we have --allow-shlib-undefined, but changing this should not affect libjvm building. Do you really see a difference in build success/failure depending on that line?

[vpa1977]

I'm not sure why we have --allow-shlib-undefined, but changing this should not affect libjvm building. Do you really see a difference in build success/failure depending on that line?

Hi,

Thank you for looking at the issue!!!

Removing the setting should just prevent typos like this from getting into code base. Maybe there was some historical issue that was addressed by it. I did a rebuild on all arches I had access to validate that master does not have any more undefined symbols.

[djelinski]

Looks good to me. Mach5 and GHA agree that we don't need explicit allow-shlib-undefined anymore.

On a side note, our use of .type appears to serve no purpose other than introducing errors. Perhaps we could remove all the useless .type lines in a follow up PR.

[magicus]

Ok, I think I'm understanding this better now. The error occurs since the .type, .globl and .hidden directives do not match -- we define the global symbol to be SafeFetch32_impl but then we set the type of the non-existing symbol _SafeFetch32_impl. Somehow this tricks the linker into accepting this as an undefined symbol.

The .type directive is not without purpose -- it sets the type of the symbol to be a function. If omitted, the type will be NOTYPE. Apparently this does not break the program but there is no reason to remove the .type directives.

Instead, we should have a common macro, something like this:

#define DECLARE_FUNC(func) \
.globl func ; \
.hidden func ; \
.type func,@function ; \
func:

in a shared file, and include and use it for all symbols in our hotspot assembly files. I was thinking somewhat along those lines last time I was poking around there (when introducing .hidden for the removal of the hotspot map files), but never really got around to it. This bug really shows why we should do that.

[magicus]

So, after sleeping on this, here is my analysis/verdict:

1. The fix on renaming the .type directive looks good. This is the actual bug fix.

2. We can remove --allow-shlib-undefined. That just overrides --no-allow-shlib- undefined, which has been the default for years and years, and there is no good reason to have it. It is not strictly related to the bug fix per se, but we can do that in this PR as well to keep things simpler.

3. The reason this could happen was since we used assembler code. A similar problem in C++ code would not have been allowed by the compiler.

4. If we introduce a common macro as I suggest, we can avoid this ever happening again. So while the linker cannot guarantee the consistency when linking libjvm.so (which I really would have liked), by using a stricter scheme when defining symbols in assembly code, we can make sure to avoid it.

[magicus]

I created https://bugs.openjdk.org/browse/JDK-8331921 to track setting up proper assembly functions consistently.

[vpa1977]

/integrate

[openjdk]

@vpa1977
Your change (at version 18ebbd6) is now ready to be sponsored by a Committer.

[magicus]

/sponsor

[openjdk]

Going to push as commit 2d62215.
Since your change was applied there have been 91 commits pushed to the master branch:

• 588e314: 8331886: Allow markdown src file overrides
• 0d1216c: 8331939: Add custom hook for TestImage
• 42b1d85: 8330275: Crash in XMark::follow_array
• 230fac8: 8331941: Make CollectedHeap::parallel_object_iterator public
• c845261: 8331924: Parallel: Remove unused MutableSpace::mangle_unused_area_complete
• ad78b7f: 8331185: Enable compiler memory limits in debug builds
• aafa15f: 8331208: Memory stress test that checks OutOfMemoryError stack trace fails
• edd47c1: 8308033: The jcmd thread dump related tests should test virtual threads
• 1aebab7: 8320995: RISC-V: C2 PopCountVI
• 0eff492: 8330278: Have SSLSocketTemplate.doClientSide use loopback address
• ... and 81 more: https://git.openjdk.org/jdk/compare/286cbf831c2eb76e31bd69b4a93cd5ae9a821493...master

Your commit was automatically rebased without conflicts.

[openjdk]

@magicus @vpa1977 Pushed as commit 2d62215.

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