8343883: Cannot resolve user specified toolchain-path for lld after JDK-8338304 #21999

sendaoYan · 2024-11-10T04:09:58Z

Hi all,
After JDK-8338304, below configure command will generate failure error: Could not find required tool for LLD. The lld linker located in path which config with option --with-toolchain-path, but the check UTIL_REQUIRE_PROGS(LLD, lld) introduced by JDK-8338304 was unable to find the lld. This PR add the condition to deal with user specidied --with-toolchain-path to find the correct lld fullpath. The change has beeen verified locally, trivial fix, the risk is low.

bash configure --with-jobs=128 --with-debug-level=release --enable-unlimited-crypto --with-jvm-variants=server --with-version-opt=f4008968 --with-zlib=system --with-toolchain-type=clang --with-toolchain-path=/home/yansendao/software/acc/x86_64/bin

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-8343883: Cannot resolve user specified toolchain-path for lld after JDK-8338304 (Bug - P4)

Reviewers

Julian Waters (@TheShermanTanker - Committer) Review applies to 6f513a3a
Magnus Ihse Bursie (@magicus - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21999

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…DK-8338304

bridgekeeper · 2024-11-10T04:10:52Z

👋 Welcome back syan! 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 · 2024-11-10T04:11:38Z

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

8343883: Cannot resolve user specified toolchain-path for lld after JDK-8338304

Reviewed-by: ihse, jwaters

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

2989d87: 8343805: RISC-V: JVM crashes on startup when disabling compressed instructions
78b8015: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
8a2a75e: 8339892: Several security shell tests don't set TESTJAVAOPTS
50b6e41: 8300732: Whitebox functions for Metaspace test should use byte size
67d1ef1: 8344018: Remove unlimited memory setting from TestScalarReplacementMaxLiveNodes
5729227: 8343250: ArrayBlockingQueue serialization not thread safe
2c1e4c3: 8343600: RISC-V: enable CRC32 intrinsic when either Zba or RVV are supported
3727f40: 8343745: Only update Last Value Assertion Predicates in Loop Unrolling
b53ee05: 8202617: javadoc generates broken links to undocumented (e.g. private) members
cfe719f: 8340565: Create separate index page for terms defined by the index tag
... and 28 more: https://git.openjdk.org/jdk/compare/f400896822c2704d8e7c66afc1efa8a4fa91acb6...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 · 2024-11-10T04:12:17Z

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

build

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 · 2024-11-10T04:15:56Z

Webrevs

01: Full - Incremental (216a0d4b)
00: Full (6f513a3a)

sendaoYan · 2024-11-10T07:43:16Z

GHA report failure 'macos-aarch64 / test - Test (tier1) ' at 'install dependcencies' stage invalid developer directory, the failure seems like environmental issue, it's unreleated to this PR.

TheShermanTanker

Nice work!

magicus · 2024-11-12T13:21:04Z

make/autoconf/flags-ldflags.m4

@@ -74,7 +74,11 @@ AC_DEFUN([FLAGS_SETUP_LDFLAGS_HELPER],
      # Clang needs the lld linker to work correctly
      BASIC_LDFLAGS="-fuse-ld=lld -Wl,--exclude-libs,ALL"
      if test "x$CXX_IS_USER_SUPPLIED" = xfalse && test "x$CC_IS_USER_SUPPLIED" = xfalse; then
-        UTIL_REQUIRE_PROGS(LLD, lld)
+        if test "x$TOOLCHAIN_PATH" != x; then
+          UTIL_REQUIRE_PROGS(LLD, lld, $TOOLCHAIN_PATH)


No, this is not correct. Did you test this patch without having lld in the toolchain path? Doing this will remove the normal lookup in $PATH and only look in $TOOLCHAIN_PATH.

After I remove the lld binary from $TOOLCHAIN_PATH, such as mv /home/yansendao/software/acc/x86_64/bin/lld /tmp/, the configure command generate configure: error: Could not find required tool for LLD as expected.

If the statement UTIL_REQUIRE_PROGS(LLD, lld)lookup in $PATH, the line 77 condition statement in this PR has make sure that, and if the configure command doesn't received option --with-toolchain-path, the normal lookup in $PATH still work.

I meant, if you have lld in your PATH but not in TOOLCHAIN_PATH and you specify a toolchain path, then your patch will introduce a regression.

magicus · 2024-11-12T13:27:04Z

My first reaction was that "You need to use UTIL_LOOKUP_TOOLCHAIN_PROGS instead". But then I checked that it did what I thought it did, and it did not... It should have prepended $TOOLCHAIN_PATH to the set of paths to search, but instead we do this by changing the actual path in TOOLCHAIN_PRE_DETECTION. I think this is a remnant of older times, before we started using our own UTIL_LOOKUP* functions.

I think the proper solution is to remove this setting of the PATH from TOOLCHAIN_PRE/POST_DETECTION, and instead update UTIL_LOOKUP_TOOLCHAIN_PROGS to use $TOOLCHAIN_PATH:$PATH as 3rd variable to UTIL_LOOKUP_PROGS (if it is not given), and then finally to use UTIL_LOOKUP_TOOLCHAIN_PROGS for LLD here in FLAGS_SETUP_LDFLAGS_HELPER.

But.

That is a bit of a risky change that can cause breakage, so I will not propose you do it now.

make/autoconf/flags-ldflags.m4

magicus · 2024-11-12T13:35:36Z

I created https://bugs.openjdk.org/browse/JDK-8344030 to track the idea of a better solution.

magicus

This looks better, thanks.

magicus · 2024-11-12T16:11:29Z

make/autoconf/flags-ldflags.m4

@@ -74,7 +74,7 @@ AC_DEFUN([FLAGS_SETUP_LDFLAGS_HELPER],
      # Clang needs the lld linker to work correctly
      BASIC_LDFLAGS="-fuse-ld=lld -Wl,--exclude-libs,ALL"
      if test "x$CXX_IS_USER_SUPPLIED" = xfalse && test "x$CC_IS_USER_SUPPLIED" = xfalse; then
-        UTIL_REQUIRE_PROGS(LLD, lld)
+        UTIL_REQUIRE_PROGS(LLD, lld, $TOOLCHAIN_PATH:$PATH)


Actually, I am not 100% sure that this will work if $TOOLCHAIN_PATH is empty. You need to check that. If not, this should probably be fixed in UTIL_CHECK_PROGS instead to allow for that, since this is a more elegant solution if empty path values are allowed.

The call tree is UTIL_REQUIRE_PROGS -> UTIL_LOOKUP_PROGS. The UTIL_LOOKUP_PROGS seems have deal with empty path.
https://github.com/openjdk/jdk/blob/master/make/autoconf/util_paths.m4#L404
And I have verify both set $TOOLCHAIN_PATH and unset $TOOLCHAIN_PATH locally.

Have lld in $PATH but not in $TOOLCHAIN_PATH works normal.

sendaoYan · 2024-11-12T17:25:31Z

Thanks all for the advice and review. The change has been verified locally.

/integrate

openjdk · 2024-11-12T17:26:12Z

Going to push as commit 63eb485.
Since your change was applied there have been 42 commits pushed to the master branch:

db85090: 8338411: Implement JEP 486: Permanently Disable the Security Manager
c12b386: 8338007: [JVMCI] ResolvedJavaMethod.reprofile can crash ciMethodData
81752c4: 8338565: Test crashed: assert(is_path_empty()) failed: invariant
e5eaa7f: 8343946: JFR: Wildcard should only work with COUNT for 'jfr view'
2989d87: 8343805: RISC-V: JVM crashes on startup when disabling compressed instructions
78b8015: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning
8a2a75e: 8339892: Several security shell tests don't set TESTJAVAOPTS
50b6e41: 8300732: Whitebox functions for Metaspace test should use byte size
67d1ef1: 8344018: Remove unlimited memory setting from TestScalarReplacementMaxLiveNodes
5729227: 8343250: ArrayBlockingQueue serialization not thread safe
... and 32 more: https://git.openjdk.org/jdk/compare/f400896822c2704d8e7c66afc1efa8a4fa91acb6...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-12T17:26:22Z

@sendaoYan Pushed as commit 63eb485.

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

8343883: Cannot resolve user specified toolchain-path for lld after J…

6f513a3

…DK-8338304

sendaoYan marked this pull request as ready for review November 10, 2024 04:10

openjdk bot added the rfr Pull request is ready for review label Nov 10, 2024

openjdk bot added the build build-dev@openjdk.org label Nov 10, 2024

TheShermanTanker approved these changes Nov 12, 2024

View reviewed changes

magicus suggested changes Nov 12, 2024

View reviewed changes

magicus reviewed Nov 12, 2024

View reviewed changes

make/autoconf/flags-ldflags.m4 Outdated Show resolved Hide resolved

lookup $TOOLCHAIN_PATH:$PATH

216a0d4

magicus approved these changes Nov 12, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 12, 2024

magicus reviewed Nov 12, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 12, 2024

openjdk bot closed this Nov 12, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 12, 2024

sendaoYan deleted the jbs8343883 branch November 12, 2024 17:36

magicus mentioned this pull request Apr 8, 2025

8344030: Improved handling of TOOLCHAIN_PATH #24525

Draft

3 tasks

8343883: Cannot resolve user specified toolchain-path for lld after JDK-8338304 #21999

8343883: Cannot resolve user specified toolchain-path for lld after JDK-8338304 #21999

Uh oh!

Conversation

sendaoYan commented Nov 10, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 10, 2024

Uh oh!

openjdk bot commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 10, 2024

Uh oh!

mlbridge bot commented Nov 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

sendaoYan commented Nov 10, 2024

Uh oh!

TheShermanTanker left a comment

Choose a reason for hiding this comment

Uh oh!

magicus Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sendaoYan Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

sendaoYan Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magicus Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

magicus commented Nov 12, 2024

Uh oh!

Uh oh!

magicus commented Nov 12, 2024

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

magicus Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

sendaoYan Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

sendaoYan Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sendaoYan commented Nov 12, 2024

Uh oh!

openjdk bot commented Nov 12, 2024

Uh oh!

openjdk bot commented Nov 12, 2024

Uh oh!

Uh oh!

sendaoYan commented Nov 10, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 10, 2024 •

edited

Loading

mlbridge bot commented Nov 10, 2024 •

edited

Loading

magicus Nov 12, 2024 •

edited

Loading

sendaoYan Nov 12, 2024 •

edited

Loading

sendaoYan Nov 12, 2024 •

edited

Loading