8351322: Parameterize link option for pthreads #23930

snake66 · 2025-03-06T10:39:27Z

Replace hardcoded instances of -lpthread with $(LIBPTHREAD), so that it's possible to parameterize this for platforms that use different flags for enabling posix threads.

This work is a continuation of the work done by Greg Lewis in 1, but generalized for the full JDK, and set at the configure stage.

Sponsored by: The FreeBSD Foundation
Co-authored-by: Greg Lewis [email protected]

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8351322: Parameterize link option for pthreads (Enhancement - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer) Review applies to 6a2c0e53
Erik Joelsson (@erikj79 - Reviewer)
Magnus Ihse Bursie (@magicus - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23930

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Replace hardcoded instances of `-lpthread` with `$(LIBPTHREAD)`, so that it's possible to parameterize this for platforms that use different flags for enabling posix threads. This work is a continuation of the work done by Greg Lewis in [1], but generalized for the full JDK, and set at the configure stage. Sponsored by: The FreeBSD Foundation Co-authored-by: Greg Lewis <[email protected]> [1]: battleblow/jdk23u@dbd90aa

bridgekeeper · 2025-03-06T10:40:21Z

👋 Welcome back snake66! 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 · 2025-03-06T10:41:35Z

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

8351322: Parameterize link option for pthreads

Reviewed-by: erikj, ihse, 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 30 new commits pushed to the master branch:

4867a4c: 8351280: Mark Assertion Predicates useless instead of replacing them by a constant directly
64caf08: 8350572: ZGC: Enhance z_verify_safepoints_are_blocked interactions with VMError
fb0efbe: 8333578: Fix uses of overaligned types induced by ZCACHE_ALIGNED
99547c5: 8346825: [JVMCI] Remove NativeImageReinitialize annotation
ec683a1: 8351419: java.net.http: Cleanup links in HttpResponse and module-info API doc comments
f61f520: 8350325: [PPC64] ConvF2HFIdealizationTests timeouts on Power8
783eda9: 8350266: [PPC64] Interpreter: intrinsify Thread.currentThread()
19b9f11: 8351392: C2 crash: failed: Expected Bool, but got OpaqueMultiversioning
18931d0: 8351399: AIX: clang pollutes the burned-in library search paths of the generated executables / Second try with a better solution than JDK8348663
0887262: 8344301: Refine stylesheet for API docs
... and 20 more: https://git.openjdk.org/jdk/compare/cfab88b1a2351a187bc1be153be96ca983a7776c...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 (@dholmes-ora, @erikj79, @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 · 2025-03-06T10:42:15Z

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

build
client
core-libs
net
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 · 2025-03-06T10:45:04Z

Webrevs

01: Full - Incremental (46999723)
00: Full (6a2c0e53)

dholmes-ora

Abstracting this out seems reasonable to me, though I should say I thought we already used -pthread rather than -lpthread.

Needs build team approval before integrating.

snake66 · 2025-03-06T13:27:15Z

Abstracting this out seems reasonable to me, though I should say I thought we already used -pthread rather than -lpthread.

I noticed there were a few places that used -pthread by default. I left these alone in this PR.

vieiro · 2025-03-06T13:53:31Z

make/test/JtregNativeHotspot.gmk

-  BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libnativeStack += -lpthread
-  BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libTestUnloadedClass += -lpthread
-  BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exeGetCreatedJavaVMs := -lpthread
+  BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libbootclssearch_agent += $(LIBPTREAD)


Hi. Should this read $(LIBPTHREAD) instead (i.e., missing H)?
Could be me too, I need new reading glasses...

You're absolutely right.

@snake66 Making changes to makefiles is tricky, since a misspelled variable do not get any warning but is just silently ignored.

For a change like this, that is a pure refactoring that is not supposed to change the output, I highly recommend using the COMPARE_BUILD system. Unfortunately, this is severely underdocumented. There is a short paragraph at https://openjdk.org/groups/build/doc/building.html under "Developing the Build System Itself", but in short, what I'd do is to create a diff files of these changes compared to a baseline (e.g. master, a specific build or commit), make sure you have the baseline checked out, and then run make jdk-image test-image COMPARE_BUILD=PATCH=my_patch.diff. This will build the targets twice, one without the patch and one with the patch, and then automatically run the compare script to analyze any differences. For this particular patch, there should be none. This would likely have caught this typo. (Given that the test-image is actually compared; I suddenly got a bit uncertain about that...)

@vieiro Thanks! That's well spotted, I'll fix!

@magicus Thanks for this info! I'll give it a try without fixing the typo first, to see if it would catch it.

erikj79 · 2025-03-06T14:11:11Z

/reviewers 2 reviewers

openjdk · 2025-03-06T14:12:11Z

@erikj79
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

erikj79

What is the intended way of using this? Do you run make with LIBPTHREAD=-pthread or do you apply a patch on libraries.m4 for the specific way of linking to pthread?

erikj79 · 2025-03-06T14:15:38Z

make/autoconf/libraries.m4

@@ -139,7 +139,7 @@ AC_DEFUN_ONCE([LIB_SETUP_LIBRARIES],

  # Threading library
  if test "x$OPENJDK_TARGET_OS" = xlinux || test "x$OPENJDK_TARGET_OS" = xaix; then
-    BASIC_JVM_LIBS="$BASIC_JVM_LIBS -lpthread"
+    BASIC_JVM_LIBS="$BASIC_JVM_LIBS $(LIBPTHREAD)"


If you specifically need this to be resolved in the makefile rather than here, then please add a comment explaining why, otherwise use a shell script variable reference.

Suggested change

BASIC_JVM_LIBS="$BASIC_JVM_LIBS $(LIBPTHREAD)"

BASIC_JVM_LIBS="$BASIC_JVM_LIBS $LIBPTHREAD"

Yes, this is incorrect. Remember that m4 are shell scripts so you need to use shell syntax here. (I know it is confusing).

@erikj79 There will be a later patch to libraries.m4 that sets the variable based on the target platform, and then populates the LIBPTHREAD variable in spec.gmk. (https://github.com/openjdk/jdk/pull/23930/files#diff-56172cd2ec5804a5f764a6d0d5970da6144b024a06e008571f9822b2dc83cc36R147) That means the parenthesis should stay, right?

I'm not sure what you mean now. The link is to this very patch, which does what you describe -- sets LIBPTHREAD according to OS and stores it in spec.gmk.

And no, the paranthesis should not stay. If you keep them, the variable will be re-evaluated in make every time BASIC_JVM_LIBS is evaluated. That is not needed; by dropping the parenthesis the actual value to be used will be inserted as a string constant. Which is what we want, since we know the value at configure time.

magicus · 2025-03-06T15:47:58Z

What is the intended way of using this? Do you run make with LIBPTHREAD=-pthread or do you apply a patch on libraries.m4 for the specific way of linking to pthread?

This is in preparation of the upcoming BSD port, which uses -pthread instead of -pthread. It was me who suggested that this is done separately with the existing code, to minimize the patch of the BSD port.

magicus · 2025-03-06T15:56:43Z

make/Hsdis.gmk

@@ -127,7 +127,7 @@ ifeq ($(HSDIS_BACKEND), binutils)
    HSDIS_LDFLAGS += -L$(MINGW_GCC_LIB_PATH) -L$(MINGW_SYSROOT_LIB_PATH)
    MINGW_DLLCRT := $(MINGW_SYSROOT_LIB_PATH)/dllcrt2.o
    HSDIS_TOOLCHAIN_LIBS := $(MINGW_DLLCRT) -lmingw32 -lgcc -lgcc_eh -lmoldname \
-        -lmingwex -lmsvcrt -lpthread -ladvapi32 -lshell32 -luser32 -lkernel32
+        -lmingwex -lmsvcrt $(LIBPTHREAD) -ladvapi32 -lshell32 -luser32 -lkernel32
  else


The hsdis build is very weird and outside the normal integrated JDK build. I recommend you leave this instance alone. If you want to port hsdis to BSD you are likely to have to rewrite large parts of the compilation logic here anyway.

Ack, I'll drop that one from the patch.

dholmes-ora · 2025-03-07T00:18:14Z

@magicus why can't we just use -pthread everywhere? My recollection is that -pthread both sets compiler directives needed for pthread programming and links to libpthread, so it seems to be what we should be using. ??

magicus · 2025-03-07T11:31:54Z

@dholmes-ora Good question. I don't know the answer. I know we used -pthread exclusively on Solaris, and that we've used -lpthread on Linux (but apparently not in all cases?). I guess this difference would not have been introduced when the Linux port was created from the Solaris port unless it made sense. So maybe at that time, GCC did not support -pthread. Otoh, that might have changed by now.

Regardless, I would still like to see this change. We have generalized almost all system libraries to variable (like $LIBM, $LIBDL etc), and this is the last remaining (I think).

We can then check if we can turn -lpthread into -pthread on Linux as a follow up.

magicus

This looks good now. Thanks!

magicus · 2025-03-10T11:07:38Z

Another follow-up is if it would hurt to include $LIBPTHREAD for all Hotspot tests, to avoid the huge list. @dholmes-ora Do you have anything coming to mind directly that would make that infeasible, or is it just a matter of testing to add it and see if any tests fail?

magicus · 2025-03-10T13:46:21Z

@snake66 To integrate this, you need to give the command /integrate as a comment. Then I (or someone else) with committer permissions can sponsor the PR.

openjdk bot added the rfr Pull request is ready for review label Mar 6, 2025

openjdk bot added serviceability [email protected] build [email protected] client [email protected] core-libs [email protected] net [email protected] labels Mar 6, 2025

dholmes-ora approved these changes Mar 6, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 6, 2025

vieiro reviewed Mar 6, 2025

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Mar 6, 2025

erikj79 reviewed Mar 6, 2025

View reviewed changes

magicus reviewed Mar 6, 2025

View reviewed changes

snake66 added 2 commits March 6, 2025 18:35

Revert changes to make/Hsdis.gmk

a5165b5

Fix typo PTREAD->PTHREAD

a0a49b8

Use shell var syntax in libraries.m4

4699972

magicus approved these changes Mar 10, 2025

View reviewed changes

erikj79 approved these changes Mar 10, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8351322: Parameterize link option for pthreads #23930

8351322: Parameterize link option for pthreads #23930

snake66 commented Mar 6, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 6, 2025

openjdk bot commented Mar 6, 2025 •

edited

Loading

openjdk bot commented Mar 6, 2025

mlbridge bot commented Mar 6, 2025 •

edited

Loading

dholmes-ora left a comment

snake66 commented Mar 6, 2025

vieiro Mar 6, 2025

magicus Mar 6, 2025

snake66 Mar 6, 2025

erikj79 commented Mar 6, 2025

openjdk bot commented Mar 6, 2025

erikj79 left a comment

erikj79 Mar 6, 2025

magicus Mar 6, 2025

snake66 Mar 6, 2025

magicus Mar 6, 2025

magicus commented Mar 6, 2025

magicus Mar 6, 2025

snake66 Mar 6, 2025

dholmes-ora commented Mar 7, 2025

magicus commented Mar 7, 2025

magicus left a comment

magicus commented Mar 10, 2025

magicus commented Mar 10, 2025

	BASIC_JVM_LIBS="$BASIC_JVM_LIBS $(LIBPTHREAD)"
	BASIC_JVM_LIBS="$BASIC_JVM_LIBS $LIBPTHREAD"

8351322: Parameterize link option for pthreads #23930

Are you sure you want to change the base?

8351322: Parameterize link option for pthreads #23930

Conversation

snake66 commented Mar 6, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 6, 2025

openjdk bot commented Mar 6, 2025 • edited Loading

openjdk bot commented Mar 6, 2025

mlbridge bot commented Mar 6, 2025 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

snake66 commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikj79 commented Mar 6, 2025

openjdk bot commented Mar 6, 2025

erikj79 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

Choose a reason for hiding this comment

magicus commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora commented Mar 7, 2025

magicus commented Mar 7, 2025

magicus left a comment

Choose a reason for hiding this comment

magicus commented Mar 10, 2025

magicus commented Mar 10, 2025

snake66 commented Mar 6, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 6, 2025 •

edited

Loading

mlbridge bot commented Mar 6, 2025 •

edited

Loading