Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not link -ldl and -lpthread unconditionally #2949

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Do not link -ldl and -lpthread unconditionally #2949

wants to merge 1 commit into from

Conversation

CohenArthur
Copy link
Member

No description provided.

@CohenArthur CohenArthur force-pushed the do-not-link-against-dl-pthread-unconditionally branch from bd03e12 to 9721ca7 Compare April 12, 2024 14:56
@tschwinge
Copy link
Member

Yes, that's going in the right direction. 👍

At least partially, this again (like #2942 "Check for cargo when building rust language") needs to go into the top-level configure, in order to be able to gracefully disable GCC/Rust for --enable-languages=all configurations.

@CohenArthur CohenArthur force-pushed the do-not-link-against-dl-pthread-unconditionally branch from 9721ca7 to 6d95466 Compare April 12, 2024 15:04
@CohenArthur
Copy link
Member Author

Yes, that's going in the right direction. 👍

At least partially, this again (like #2942 "Check for cargo when building rust language") needs to go into the top-level configure, in order to be able to gracefully disable GCC/Rust for --enable-languages=all configurations.

yeah, I saw that Jakub had mentioned this but it does seem a bit too late to make such a change, no? Jakub had also mentioned erroring out, which is what I chose to do here

@CohenArthur CohenArthur marked this pull request as ready for review April 12, 2024 15:05
@tschwinge
Copy link
Member

It's late no matter whether in the top-level configure, or gcc/configure. ;-|

@tschwinge
Copy link
Member

Gracefully disable for implicit "rust" in --enable-languages=all, error out for explicit --enable-languages=rust.

@tschwinge
Copy link
Member

Only crab1 should then be linked against these libraries, not all GCC things generally.

@CohenArthur
Copy link
Member Author

Only crab1 should then be linked against these libraries, not all GCC things generally.

right, so add -ldl -lpthread to something like CRAB1_LIBS instead? I think we have RUST_LIBS for this already, I'll check.

Gracefully disable for implicit "rust" in --enable-languages=all, error out for explicit --enable-languages=rust.

so you're forcing me to do more autotools work, do you like seeing me suffer or something? :P will work on this next

@tschwinge
Copy link
Member

The implicit vs. explicit thing should be really easy ;-) in the top-level configure: just like how #2942 "Check for cargo when building rust language" does this, and similarly for GCC/D if gdc or GCC/Ada if gnat is not available in a suitable version.

@CohenArthur CohenArthur force-pushed the do-not-link-against-dl-pthread-unconditionally branch 3 times, most recently from df478ee to 7399ae7 Compare April 16, 2024 13:02
@CohenArthur
Copy link
Member Author

@tschwinge I've made the required changes and pushed them :) tested locally and it seems to work fine on a system where glibc < 2.34. tested with --enable-languages=all and --enable-languages=rust

@CohenArthur CohenArthur force-pushed the do-not-link-against-dl-pthread-unconditionally branch 5 times, most recently from 67cad72 to c34b0b2 Compare April 19, 2024 09:44
@CohenArthur
rust: Do not link with libdl and libpthread unconditionally
c34b0b2 
ChangeLog:

	* Makefile.tpl: Add CRAB1_LIBS variable.
	* Makefile.in: Regenerate.
	* configure: Regenerate.
	* configure.ac: Check if -ldl and -lpthread are needed, and if so, add
	them to CRAB1_LIBS.

gcc/rust/ChangeLog:

	* Make-lang.in: Remove overazealous LIBS = -ldl -lpthread line, link
	crab1 against CRAB1_LIBS.
@P-E-P
Copy link
Member

P-E-P commented May 21, 2024 •
edited

Should we merge this ? Has it already been merged upstream ? It requires a rebase anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@P-E-P P-E-P P-E-P approved these changes

@tschwinge tschwinge Awaiting requested review from tschwinge

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@CohenArthur @tschwinge @P-E-P