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

ensure JSON-defined targets are consistent #133409

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

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 24, 2024

We have a check_consistency check that ensures some invariants which (presumably) the rest of the compiler relies on. However, JSON targets can easily be written in a way that violates those invariants. So this PR applies the same consistency check to JSON targets that we already enforce for built-in targets.

I have converted many of the assertions in that function to new macros that show a nice error instead of a panic; if people are okay with the general approach here, I can do that for the rest of the checks as well.

@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2024

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 24, 2024
@@ -8,5 +8,6 @@
"os": "ericos",
"linker-flavor": "ld.lld",
"linker": "rust-lld",
"relocation-model": "static",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this will cause a bunch of breakage for custom target specs out there as well... the default relocation_model is "pic", but the default for dynamic_linking is false, and that's not a coherent combination (or at least, it is rejected by our existing target checks). Therefore, all targets must explicitly set one of those two fields.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, most of our tests violate this.

So either it's actually not so bad to have "relocation_model: pic, dynamic_linking: false", or we should really use different defaults...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need some experts for linking and target specs... Cc @bjorn3 , not sure who else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made it so that JSON targets may violate this particular check. Given that we also allow uefi targets to violate it, I assume this invariant is not too critical.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The combination of relocation model set to PIC and no dynamic linking is definitively fine when static_position_independent_executables is enabled. In fact static_position_independent_executables requires either PIC or PIE. When static_position_independent_executables is false, I think it would also be fine as the linker can just fill in a fixed address in the GOT, but I'm not entirely sure linkers actually support that.

@RalfJung RalfJung changed the title Target consistency ensure JSON-defined targets are consistent Nov 24, 2024
@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the target-consistency branch 3 times, most recently from 3be78e5 to 1628e60 Compare November 24, 2024 08:12
@rust-log-analyzer

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants