8351322: Parameterize link option for pthreads by snake66 · Pull Request #23930 · openjdk/jdk

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 glewis@eyesbeyond.com

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 <glewis@eyesbeyond.com> [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 32 new commits pushed to the master branch:

d90b79a: 8351046: Rename ObjectMonitor functions
e90b6bd: 8350638: Make keyboard navigation more usable in API docs
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
... and 22 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

-  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

  # 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

    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.

snake66 · 2025-03-10T15:45:59Z

/integrate

openjdk · 2025-03-10T15:47:11Z

@snake66
Your change (at version 4699972) is now ready to be sponsored by a Committer.

sendaoYan · 2025-03-11T15:08:55Z

/sponsor

openjdk · 2025-03-11T15:11:11Z

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

af9af7e: 8351145: RISC-V: only enable some crypto intrinsic when AvoidUnalignedAccess == false
83de340: 8343789: Move mutable nmethod data out of CodeCache
0de2cdd: 8351458: (ch) Move preClose to UnixDispatcher
cd9f1d3: 8286204: [Accessibility,macOS,VoiceOver] VoiceOver reads the spinner value 10 as 1 when user iterates to 10 for the first time on macOS
4cf6316: 8351414: C2: MergeStores must happen after RangeCheck smearing
8a5ed47: 8350148: Native stack overflow when writing Java heap objects into AOT cache
5928209: 8347405: MergeStores with reverse bytes order value
f984c2b: 8351505: (fs) Typo in the documentation of java.nio.file.spi.FileSystemProvider.getFileSystem()
ffa6340: 8351567: Jar Manifest test ValueUtf8Coding produces misleading diagnostic output
8d8bd0c: 8349492: Update sun/security/pkcs12/KeytoolOpensslInteropTest.java to use a recent Openssl version
... and 39 more: https://git.openjdk.org/jdk/compare/cfab88b1a2351a187bc1be153be96ca983a7776c...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-11T15:11:22Z

@sendaoYan @snake66 Pushed as commit b957e5e.

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

dholmes-ora · 2025-03-17T06:41:31Z

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?

Sorry I was out of contact for a while. I can't imagine why any test would fail if linked with libpthread, given the VM is linking to it anyway.

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

I have a vague recollection that at one time -pthread set some _POSIX_SOURCE define (or something like that) which conflicted with our use of some gcc specific things. But that was long ago so I would try it and see. Of couise it then becomes hard to classify what kind of flag this is because it isn't strictly a library flag.

snake66 · 2025-03-17T10:44:30Z

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?

(...) I can't imagine why any test would fail if linked with libpthread, given the VM is linking to it anyway.

I'm happy to test this, if you want. Won't have time this week, though.

magicus · 2025-03-20T12:37:33Z

@snake66 No need, I'm already on it: #24130

magicus · 2025-03-20T12:38:37Z

@dholmes-ora

I have a vague recollection that at one time -pthread set some _POSIX_SOURCE define (or something like that) which conflicted with our use of some gcc specific things. But that was long ago so I would try it and see. Of couise it then becomes hard to classify what kind of flag this is because it isn't strictly a library flag.

Is there any gain then in changing away from -lpthread? That is clearly defined, link with libpthread, with no side effects. "If it ain't broke..."

dholmes-ora · 2025-03-21T03:17:33Z

Is there any gain then in changing away from -lpthread? That is clearly defined, link with libpthread, with no side effects. "If it ain't broke..."

True - what we have works and using -pthread might subtly change things.

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

openjdk Bot added serviceability serviceability-dev@openjdk.org build build-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org 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

openjdk Bot added the sponsor Pull request is ready to be sponsored label Mar 10, 2025

openjdk Bot added the integrated Pull request has been integrated label Mar 11, 2025

openjdk Bot closed this Mar 11, 2025

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 11, 2025

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

Conversation

snake66 commented Mar 6, 2025 • 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 Mar 6, 2025

Uh oh!

openjdk Bot commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk Bot commented Mar 6, 2025

Uh oh!

mlbridge Bot commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

snake66 commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikj79 commented Mar 6, 2025

Uh oh!

openjdk Bot commented Mar 6, 2025

Uh oh!

erikj79 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magicus commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Mar 7, 2025

Uh oh!

magicus commented Mar 7, 2025

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

magicus commented Mar 10, 2025

Uh oh!

magicus commented Mar 10, 2025

Uh oh!

snake66 commented Mar 10, 2025

Uh oh!

openjdk Bot commented Mar 10, 2025

Uh oh!

sendaoYan commented Mar 11, 2025

Uh oh!

openjdk Bot commented Mar 11, 2025

Uh oh!

openjdk Bot commented Mar 11, 2025

Uh oh!

dholmes-ora commented Mar 17, 2025

Uh oh!

snake66 commented Mar 17, 2025

Uh oh!

magicus commented Mar 20, 2025

Uh oh!

magicus commented Mar 20, 2025

Uh oh!

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