Re: RFR: 8351322: Parameterize link option for pthreads [v2]
On Mon, 17 Mar 2025 06:41:31 GMT, David Holmes wrote: >> @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. ?? > >> 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. > > 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. - PR Comment: https://git.openjdk.org/jdk/pull/23930#issuecomment-2729033496
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 15:56:43 GMT, Magnus Ihse Bursie wrote: >> 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 >> >> [1]: >> https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 > > make/Hsdis.gmk line 131: > >> 129: HSDIS_TOOLCHAIN_LIBS := $(MINGW_DLLCRT) -lmingw32 -lgcc -lgcc_eh >> -lmoldname \ >> 130: -lmingwex -lmsvcrt $(LIBPTHREAD) -ladvapi32 -lshell32 -luser32 >> -lkernel32 >> 131: 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/23930#discussion_r1983780437
Re: RFR: 8351322: Parameterize link option for pthreads [v2]
> 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 > > [1]: > https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 snake66 has updated the pull request incrementally with three additional commits since the last revision: - Use shell var syntax in libraries.m4 - Fix typo PTREAD->PTHREAD - Revert changes to make/Hsdis.gmk - Changes: - all: https://git.openjdk.org/jdk/pull/23930/files - new: https://git.openjdk.org/jdk/pull/23930/files/6a2c0e53..46999723 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23930&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23930&range=00-01 Stats: 641 lines in 3 files changed: 0 ins; 0 del; 641 mod Patch: https://git.openjdk.org/jdk/pull/23930.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23930/head:pull/23930 PR: https://git.openjdk.org/jdk/pull/23930
RFR: 8351322: Parameterize link option for pthreads
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 [1]: https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 - Commit messages: - 8351322: Parameterize link option for pthreads Changes: https://git.openjdk.org/jdk/pull/23930/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23930&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8351322 Stats: 661 lines in 10 files changed: 9 ins; 0 del; 652 mod Patch: https://git.openjdk.org/jdk/pull/23930.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23930/head:pull/23930 PR: https://git.openjdk.org/jdk/pull/23930
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 12:46:25 GMT, David Holmes wrote: > 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. - PR Comment: https://git.openjdk.org/jdk/pull/23930#issuecomment-2703853406
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 14:15:38 GMT, Erik Joelsson wrote: >> 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 >> >> [1]: >> https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 > > make/autoconf/libraries.m4 line 142: > >> 140: # Threading library >> 141: if test "x$OPENJDK_TARGET_OS" = xlinux || test "x$OPENJDK_TARGET_OS" >> = xaix; then >> 142: 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. > > Suggestion: > > BASIC_JVM_LIBS="$BASIC_JVM_LIBS $LIBPTHREAD" @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? - PR Review Comment: https://git.openjdk.org/jdk/pull/23930#discussion_r1983779126
Integrated: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 10:39:27 GMT, snake66 wrote: > 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 > > [1]: > https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 This pull request has now been integrated. Changeset: b957e5ed Author:Harald Eilertsen URL: https://git.openjdk.org/jdk/commit/b957e5ed1a8b77e01aad1bb574e4914131cdbfa6 Stats: 660 lines in 9 files changed: 9 ins; 0 del; 651 mod 8351322: Parameterize link option for pthreads Reviewed-by: erikj, ihse, dholmes - PR: https://git.openjdk.org/jdk/pull/23930
Re: RFR: 8351322: Parameterize link option for pthreads
On Thu, 6 Mar 2025 13:53:31 GMT, Antonio Vieiro wrote: >> 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 >> >> [1]: >> https://github.com/battleblow/jdk23u/commit/dbd90aa8ab0b7f5e4865864a7c63d975daacabf4 > > make/test/JtregNativeHotspot.gmk line 886: > >> 884: BUILD_HOTSPOT_JTREG_LIBRARIES_JDK_LIBS_libnativeStack := >> java.base:libjvm >> 885: else >> 886: 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... @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. - PR Review Comment: https://git.openjdk.org/jdk/pull/23930#discussion_r1983764371
