error on empty export_name#155515
Conversation
folkertdev
added
the
I-lang-nominated
|
Some changes occurred in compiler/rustc_attr_parsing |
rustbot
added
A-attributes
|
rustbot has assigned @jdonszelmann. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Should we consider a symbol name with only white spaces as empty? |
|
Hmm, I don't know that it's useful at all but it apparently does work https://godbolt.org/z/dMqhK6hbx " ":
.Lfunc_begin0:
.cfi_startproc
.file 1 "/app" "example.rs"
.loc 1 2 16 prologue_end |
| //~^ ERROR `export_name` may not contain null characters | ||
| fn null_terminated() {} | ||
|
|
||
| #[export_name = "\0"] |
There was a problem hiding this comment.
did we not already have a test for these? thanks for making one in that case
There was a problem hiding this comment.
We did test that the error code is emitted in tests/ui/error-codes/E0648.rs, but now that this condition is checked in the parser a parser test seemed appropriate, plus this tests some extra inputs.
|
r=me on implementation after lang approves this one, technically a breaking change |
jdonszelmann
added
S-waiting-on-t-lang
traviscross
added
I-lang-radar
|
Makes sense to me. Thanks @folkertdev. @rfcbot fcp merge lang |
|
@folkertdev r=me in 10 days when the fcp runs out :) |
|
Yeah, the bot seems kind of stuck though. @traviscross maybe just try again? edit: it's being looked into #t-infra > rfcbot unresponsive |
|
(The bot has been fixed.) @rfcbot fcp merge lang |
|
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: 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. |
rust-rfcbot
added
proposed-final-comment-period
|
Usual analysis: this is the kind of place where I wonder about making it a lint or a hard error, but here I think hard error is the right choice since
@rfcbot reviewed |
rust-rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
traviscross
removed
the
I-lang-nominated
traviscross
removed
the
P-lang-drag-1
7 participants
fixes #155495
Using an empty string as the name makes LLVM make up a name. However this name can be inconsistent between compilation units, which is UB and can cause linking errors, and some parts of LLVM just crash on the empty name (see the linked issue).
As far as we know there is only one valid pattern that could use this, a
#[used]static that is not referenced by the program at all. That is not UB, but theexport_nameis not required for that to work, just normal rust name mangling would do fine.Technically this is a breaking change, but it seems unlikely that this actually breaks code in the wild that wasn't already broken. I'll leave it up to T-lang to determine what is required here (crater run, FCW, ...), but my gut feeling is that we could just merge this and nobody would notice.