Implement EII for statics

[JonathanBrouwer]

View all comments

This PR implements EII for statics. I've tried to mirror the implementation for functions in a few places, this causes some duplicate code but I'm also not really sure whether there's a clean way to merge the implementations.

This does not implement defaults for static EIIs yet, I will do that in a followup PR

[JonathanBrouwer]

r? @jdonszelmann

This code touches quite a few areas of the compiler I'm not super familiar yet with, so please review carefully whether I'm doing any stupid things :3

View changes since this review

[rustbot]

jdonszelmann is currently at their maximum review capacity.
They may take a while to respond.

[rustbot]

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

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

The parser was modified, potentially altering the grammar of (stable) Rust
which would be a breaking change.

cc @fmease

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[jdonszelmann]

I'm not sure I understand when this can fail

[jdonszelmann]

or why this sanity check is necessary. I believe it may be, but the comment doesn't explain what scenario this matters in or what bug we previously had with this.

[JonathanBrouwer]

I've added explanation to the comment, the assert is indeed simply a sanity check

[bjorn3]

The fact that eii_impls is a list seems to imply that a single item can define multiple EIIs at the same time. Wouldn't that result in multiple statics aliasing each other with this PR? Especially with static mut that would be rather dangerous. But even without, I'm not sure that is not UB.

[JonathanBrouwer]

Disallowing static mut EIIs makes sense to me, but more because they're hard to use correctly than that they're inherently unsound

[bjorn3]

Currently if you have two extern statics with different symbol names, they would never alias. I can very much imagine that there will be crates which break if two EIIs defined by said crate alias. Breaking both as in causing UB and as in misbehaving without UB. Also it requires having support for aliasing at the object file level, while if each static can define only a single EII, then we can get away without any aliasing support at the object file level. The definition can just have the same symbol name as the EII. And if the EII has a default, this default can either be put in a separate object file only linked when the default is used or be defined as weak symbol.

[JonathanBrouwer]

Right, I agree this is also undesirable
So what I will do is:

• Disallow multiple eii impls on 1 static
• Disallow eii on mut statics

[JonathanBrouwer]

@rustbot author

[rustbot]

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

[jdonszelmann]

That doesn't help though, EIIs inherently generate two symbol names for one static. Even if one is extern. And the definition can't get the same name. It should be available under the name given to the implementation as well imo. The same is true for functions atm.

[jdonszelmann]

multiple impls does make that worse, but it's already a problem to begin with

[jdonszelmann]

also, could you add a test that both have the same address?

[bjorn3]

That doesn't help though, EIIs inherently generate two symbol names for one static. Even if one is extern.

There is no inherent reason why the EII declaration and the impl have to use different symbol names afaik. If you only defining a single EII per static, then tcx.symbol_name() on the impl can return the symbol name of the declaration.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[JonathanBrouwer]

I've made the following changes:

• Disallow multiple eii impls on 1 static
• Disallow eii on mut statics
• add a test that eii decl and impl have the same address

@rustbot ready

If you only defining a single EII per static, then tcx.symbol_name() on the impl can return the symbol name of the declaration.

EIIs don't do anything other than what foreign statics already do, so I don't understand why this would be needed. With foreign statics the foreign static and the static that implements the foreign static also have two symbols referring to the same address.

[jdonszelmann]

r=me after rebase on #152384

[jdonszelmann]

@rustbot blocked

[bjorn3]

With foreign statics the foreign static and the static that implements the foreign static also have two symbols referring to the same address.

With foreign statics the foreign static directly uses the symbol name of the target at the object file level. That is how the linker knows to match up the foreign static and the definition of the static. At the object file level symbols don't have an identity separate from their symbol name. Any two symbols with the same name are merged together.

[JonathanBrouwer]

Oh right, indeed the difference is that there is an explicit alias with EII statics, which there isn't with foreign statics...

For the following code

#[eii(hello)]
static HELLO: u64;

#[hello]
static HELLO_IMPL: u64 = 5;

The generated LLVM IR is:

@_RNvCs8Dmd1bSLMH1_6simple5HELLO = alias i64, ptr @_RNvCs8Dmd1bSLMH1_6simple10HELLO_IMPL
@_RNvCs8Dmd1bSLMH1_6simple10HELLO_IMPL = internal constant [8 x i8] c"\05\00\00\00\00\00\00\00", align 8

So while there are indeed two different symbol names, one is explicitly defined as an alias to the other one, so these should still be merged together by the linker. So I still don't see where the problem is, sorry

[rust-bors]

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