Skip to content

Don't add phobos library dependences with -defaultlib=#9831

Closed
marler8997 wants to merge 2 commits intodlang:masterfrom
marler8997:nodefaultlibs
Closed

Don't add phobos library dependences with -defaultlib=#9831
marler8997 wants to merge 2 commits intodlang:masterfrom
marler8997:nodefaultlibs

Conversation

@marler8997
Copy link
Copy Markdown
Contributor

@marler8997 marler8997 commented May 21, 2019

This PR depends on:
dlang/phobos#7021 [MERGED]
dlang/druntime#2619 [MERGED]
dlang/phobos#7050 [MERGED]
dlang/druntime#2625 [MERGED]

@dlang-bot
Copy link
Copy Markdown
Contributor

dlang-bot commented May 21, 2019

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9831"

@marler8997 marler8997 force-pushed the nodefaultlibs branch 2 times, most recently from f7ae0d4 to 5674ab1 Compare May 21, 2019 23:58
@thewilsonator thewilsonator added Review:Needs Approval Review:Needs Changelog A changelog entry needs to be added to /changelog labels May 22, 2019
@rainers
Copy link
Copy Markdown
Member

rainers commented May 22, 2019

Maybe -defaultlib=and -debuglib= should be modified to also not pass the system libraries to the linker? I guess they are added only to support linking in what might be used by phobos.

@JinShil
Copy link
Copy Markdown
Contributor

JinShil commented May 22, 2019

What about moving the default libs to the command line in dmd.conf? https://github.com/dlang/installer/blob/master/create_dmd_release/extras/linux/dmd2/linux/bin64/dmd.conf

Then DMD doesn't need to hard-code all of the default libraries and it can be customized on a per-package/distribution/platform basis. If users don't want to link in any default libraries, they just use dmd -conf= to remove the effect of the dmd.conf file, or they can use a different whatever.conf file.

@marler8997
Copy link
Copy Markdown
Contributor Author

marler8997 commented May 22, 2019

I have use cases where I want to remove the default libraries without having to remove anything else that is currently in dmd.conf. I still want the default library paths provided in dmd.conf. I may still add the phobos2 library, probably via a pragma.

So whether or not the libraries we're hard-coded, or in dmd.conf, I would still need this flag, making that change orthogonal.

@marler8997
Copy link
Copy Markdown
Contributor Author

@rainers I didn't know about -defaultlib=. Going to modify my PR per your suggestion.

@thewilsonator
Copy link
Copy Markdown
Contributor

Still need a changelog entry anywhichway.

@marler8997 marler8997 changed the title Add -nodefaultlibs Don't add phobos library dependences with -defaultlib= May 22, 2019
@marler8997 marler8997 force-pushed the nodefaultlibs branch 9 times, most recently from 9167e50 to 836f5d0 Compare May 22, 2019 12:00
@marler8997 marler8997 force-pushed the nodefaultlibs branch 2 times, most recently from 0f6fe3c to 941560d Compare May 22, 2019 12:54
@thewilsonator thewilsonator removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label May 22, 2019
@marler8997 marler8997 force-pushed the nodefaultlibs branch 3 times, most recently from d55173a to 1e951dc Compare May 30, 2019 19:00
@codecov
Copy link
Copy Markdown

codecov bot commented May 30, 2019

Codecov Report

Merging #9831 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9831      +/-   ##
==========================================
+ Coverage   84.67%   84.68%   +<.01%     
==========================================
  Files         144      144              
  Lines       74524    74524              
==========================================
+ Hits        63102    63109       +7     
+ Misses      11422    11415       -7
Impacted Files Coverage Δ
src/dmd/link.d 68.83% <100%> (+2.32%) ⬆️
src/dmd/mars.d 78.21% <0%> (+0.08%) ⬆️
src/dmd/glue.d 85.54% <0%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f49f81e...1e951dc. Read the comment docs.

@marler8997
Copy link
Copy Markdown
Contributor Author

I'm stumped on this one. Looks like on the linux platforms where the host/target are different bit-widths (Linux_32_64 and Linux_64_32), the linker is not finding the pthread library symbols.

https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3663202&isPull=true

/home/braddr/sandbox/at-client/pull-3663202-Linux_32_64/dmd/generated/linux/release/32/dmd -m64 -fPIC -w -I../../src -I../../import -Isrc -defaultlib= -debuglib= -dip1000 -L-lpthread -L-lm -L/home/braddr/sandbox/at-client/pull-3663202-Linux_32_64/druntime/generated/linux/debug/64/libdruntime.a -g -debug -unittest -ofgenerated/linux/debug/64/unittest_assert src/unittest_assert.d
/home/braddr/sandbox/at-client/pull-3663202-Linux_32_64/druntime/generated/linux/debug/64/libdruntime.a(monitor__941_8c9.o): In function `_d_monitor_staticctor':
/home/braddr/sandbox/at-client/pull-3663202-Linux_32_64/druntime/src/rt/monitor_.d:158: undefined reference to `pthread_mutexattr_init'
...

Even though you can see we are explicitly providing -L-lpthread to DMD. Maybe there's a way to select different pthread libraries for your targest host? If anyone is familiar with this issue let me know.

@CyberShadow
Copy link
Copy Markdown
Member

@wilzbach's suggestions for the changelog are still not applied and have been incorrectly marked as resolved by @thewilsonator.

@rainers
Copy link
Copy Markdown
Member

rainers commented Jun 12, 2019

Even though you can see we are explicitly providing -L-lpthread to DMD. Maybe there's a way to select different pthread libraries for your targest host? If anyone is familiar with this issue let me know.

ld is known to be picky about the order of libraries passed to it on the command line, i.e. -lpthread should better be after the libraries that reference symbols from libpthread.a. Can you add them to the end of the command line that invokes the linker?

@rainers
Copy link
Copy Markdown
Member

rainers commented Jun 12, 2019

From the top of the "build dmd" task: the failing Linux runs use GNU ld (GNU Binutils for Ubuntu) 2.26.1, while the successful ones use GNU ld version 2.25.1-31.base.66.amzn1. No idea what the difference is or whether it's just a coincidence.

@thewilsonator
Copy link
Copy Markdown
Contributor

If you could convert the shell test to D, that would be great, the less of them we have the better.

@marler8997
Copy link
Copy Markdown
Contributor Author

Yeah I'll convert the test to D.

@marler8997 marler8997 force-pushed the nodefaultlibs branch 2 times, most recently from 63cb793 to acdc375 Compare July 16, 2019 06:44
@marler8997
Copy link
Copy Markdown
Contributor Author

Ok bash test is now in D. Still need to figure out the linker issue when testing druntime though.

@PetarKirov
Copy link
Copy Markdown
Member

Yep, it looks like you need to do more adjustments to the Makefiles in druntime/test/, but apart from that it looks promising.

@marler8997
Copy link
Copy Markdown
Contributor Author

I've added a commit to put the core libs (pthread/m/dl/rt) at the end of the link command, but still getting the undefined reference errors in the linker...

@marler8997
Copy link
Copy Markdown
Contributor Author

Cleaning up old PRs that aren't going anywhere.

@marler8997 marler8997 closed this May 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants