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.

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

Implement sysroot handling #419

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

mati865
Copy link
Collaborator

@mati865 mati865 commented Feb 15, 2025

Fixes #406

@mati865
Copy link
Collaborator Author

mati865 commented Feb 15, 2025

Right now this handles only linker scripts part based on my guess how it should work. Unfortunately I couldn't find any information in the docs but with this change integration tests fail with errors like:

wild: /home/mateusz/Projects/wild/wild/tests/build/cpp-integration.cc-no-pie-aarch64.wild
ld: /home/mateusz/Projects/wild/wild/tests/build/cpp-integration.cc-no-pie-aarch64.ld
.dynamic.DT_BIND_NOW
  wild
  ld 0x0

.dynamic.DT_FLAGS
  wild 0x8
  ld

So, at least linking is successful.

I still need to fix -L= according to the docs and reconsider if this could be done better.

@davidlattimore
Copy link
Owner

Thanks for working on this! I tried patching your PR and the tests all passed for me. It's strange that ld isn't setting DT_FLAGS=0x8 (BIND_NOW), given that we should be passing -Wl,-z,now to the compiler when linking.

Boxing a String probably isn't necessary and removing the Box will likely help to eliminate the clippy warning.

@mati865
Copy link
Collaborator Author

mati865 commented Feb 16, 2025

Boxing a String probably isn't necessary and removing the Box will likely help to eliminate the clippy warning.

Yeah, it's still WIP. I had started with Path, hence this Box. I haven't touched this since publishing the draft because of other things, but I'll be back to it later today.

@mati865
Copy link
Collaborator Author

mati865 commented Feb 16, 2025

So, I searched more extensively, because that sysroot application seemed dangerously broad to me and found where it's described: https://sourceware.org/binutils/docs/ld/File-Commands.html
My guess wasn't that far off, as I missed the condition to use sysroot only when linker script is within the sysroot for absolute paths 😉 Also added = and $SYSROOT handling in the scripts.

This looks mostly good now but still needs some love. I'd like to get your opinion on 556ccc3 (#419) "Is it any better?" commit. I'm not happy with either way, WDYT? Maybe you have a better idea?

Tests and -L= on the CLI are still on my todo list, but probably I'll get to it tomorrow.

@davidlattimore
Copy link
Owner

So, I searched more extensively, because that sysroot application seemed dangerously broad to me and found where it's described: https://sourceware.org/binutils/docs/ld/File-Commands.html
My guess wasn't that far off, as I missed the condition to use sysroot only when linker script is within the sysroot for absolute paths 😉 Also added = and $SYSROOT handling in the scripts.

Ah, nice find, that makes sense.

This looks mostly good now but still needs some love. I'd like to get your opinion on 556ccc3 (#419) "Is it any better?" commit. I'm not happy with either way, WDYT? Maybe you have a better idea?

Either looks fine to me.

@mati865 mati865 force-pushed the push-quunoluzxxrw branch 3 times, most recently from 21b4fd0 to 126fb46 Compare February 17, 2025 20:42
@mati865 mati865 marked this pull request as ready for review February 18, 2025 22:31
@mati865 mati865 changed the title WIP: Implement sysroot handling Implement sysroot handling Feb 18, 2025
@davidlattimore
Copy link
Owner

Thanks for taking this on and for figuring out some of the more tricky details involved!

@davidlattimore davidlattimore merged commit f9bd9ca into davidlattimore:main Feb 18, 2025
11 checks passed
@mati865 mati865 deleted the push-quunoluzxxrw branch February 18, 2025 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement --sysroot support
2 participants