Error on invalid macho section specifier

[folkertdev]

View all comments

The macho section specifier used by #[link_section = "..."] is more strict than e.g. the one for elf. LLVM will error when you get it wrong, which is easy to do if you're used to elf. So, provide some guidance for the simplest mistakes, based on the LLVM validation.

Currently compilation fails with an LLVM error, see https://godbolt.org/z/WoE8EdK1K.

The LLVM validation logic is at

https://github.com/llvm/llvm-project/blob/a0f0d6342e0cd75b7f41e0e6aae0944393b68a62/llvm/lib/MC/MCSectionMachO.cpp#L199-L203

LLVM validates the other components of the section specifier too, but it feels a bit fragile to duplicate those checks. If you get that far, hopefully the LLVM errors will be sufficient to get unstuck.

---

sidequest from #147811

r? JonathanBrouwer

specifically, is this the right place for this sort of validation? rustc_attr_parsing also does some validation.

[rustbot]

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[folkertdev]

just in case we're doing something weird that never got built or linked

@bors try jobs=aarch64-apple

[JonathanBrouwer]

specifically, is this the right place for this sort of validation? rustc_attr_parsing also does some validation.

We're trying to move as many checks as possible to attribute parsing, with the "parse, don't validate" mentality, so if an attribute gets through parsing you can assume it is correct. Therefore I'd like this to be in the attribute parser

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rust-bors]

💔 Test for 3e6930f failed: CI. Failed job:

• try - aarch64-apple (web logs, enhanced plaintext logs)

[rustbot]

These commits modify tests/rustdoc-json.
rustdoc-json is a public (but unstable) interface.

Please ensure that if you've changed the output:

• It's intentional.
• The FORMAT_VERSION in src/librustdoc-json-types is bumped if necessary.

cc @aDotInTheVoid, @obi1kenobi

[folkertdev]

@bors try jobs=aarch64-apple

[folkertdev]

btw there is some discussion at #t-lang > error on invalid macho section about whether we should FCW this first.

This change might break projects that use an invalid section specifier for a symbol that never gets linked. Currently LLVM's codegen would never see that symbol, but with this change rustc will error early.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 6c29d2f (6c29d2f2952cde2f07cef44fa298d1b9180dd914, parent: dab8d9d1066c4c95008163c7babf275106ce3f32)

[JonathanBrouwer]

r=me if T-lang is happy

View changes since this review

[tmandry]

@rfcbot fcp lang

[traviscross]

Let's do it.

@rfcbot fcp merge lang

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[joshtriplett]

@rfcbot reviewed

[nikomatsakis]

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[bjorn3]

Rfcbot didn't catch your review Niko.

[rust-bors]

☔ The latest upstream changes (presumably #155472) made this pull request unmergeable. Please resolve the merge conflicts.