Anonymous allocations in statics get duplicated for multiple codegen units

[RalfJung]

According to analysis performed by @bjorn3, this code actually leads to two allocations containing 42, i.e., FOO and BAR point to different things. The linker later merges the two, so the issue is currently not directly observable. However, it becomes observable when things are mutable.

I think this is a bug. When a static is defined as pub static BAR: &i32 = crate::a::FOO;, IMO we should guarantee that BAR and FOO have the same value. I see no lee-way here for duplicating the memory they both point to.

Cc @oli-obk

[RalfJung]

IIUC #116564 is an attempt to fix this.

[RalfJung]

IIUC #116564 is an attempt to fix this.

I was wrong.

But I also don't think I understand this example. This is all in the same crate. We could evaluate the static FOO once, which will create two allocations, and then BAR points to the inner of these two. Where does the duplicate AllocId come from...?

[oli-obk]

Oh neat you found a single crate repro.

The issue is just that llvm treats alloc ids without a name as codegen unit local and thus generates one per unit. We need to give them a name, but we only want to give those a name that come from a static.

[RalfJung]

I didn't find it, you did. 😂

[RalfJung]

The issue is just that llvm treats alloc ids without a name as codegen unit local and thus generates one per unit. We need to give them a name, but we only want to give those a name that come from a static.

That example fails even with -Ccodegen-units=1 so multiple codegen units can't be the issue I think?

[RalfJung]

I think this happens because the codegen backend does not cache allocations. There is a function that codegens an allocation, and if the same AllocId is requested multiple times that just generates multiple LLVM allocations.

[oli-obk]

Ah good point. That won't help us cross crate tho

[oli-obk]

The issue is just that llvm treats alloc ids without a name as codegen unit local and thus generates one per unit. We need to give them a name, but we only want to give those a name that come from a static.

That example fails even with -Ccodegen-units=1 so multiple codegen units can't be the issue I think?

Yea, the modules are not needed: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=a447edb36a144bd8863b11f743a37937

[RalfJung]

Ah good point. That won't help us cross crate tho

Yeah, adding such a cache does not replace your planned "named inner allocations".

[RalfJung]

Is there an example of this that doesn't need static mut? We want to get rid of static mut anyway so I hope this is useful even for static with interior mutability...

[oli-obk]

yea, the issues are the same with static with interior mutability. But I think then we need unleash mode for the test, but I'll check.

[oli-obk]

lol no, not even with unleash mode. Our interning-time checks are too solid: https://rust.godbolt.org/z/65o9PnvPq

[RalfJung]

Ah right, static mut is the only one that allows inner mutable allocations. That's quite explicit in the new interner.

But as far as pointer identity goes, we don't need mutability to make this an example, do we? The cross-crate version of this should also trigger the problem?

pub static FOO: &[i32] = &[42];
pub static BAR: &[i32] = &*FOO;

fn main() {
    assert_eq!(FOO.as_ptr(), BAR.as_ptr());
}

[RalfJung]

@cjgillot I seem to recall this issue coming up in the GVN pass... now that this is fixed, is there cleanup that could be done in GVN?

[cjgillot]

Function pointer AllocIds have the same duplication behaviour, and that's by design.
So cleaning-up GVN means distinguishing fn pointer AllocId from static/memory AllocId. I haven't thought where vtable AllocIds lie in this split.

[bjorn3]

Vtables are duplicated too. They have to for the same reason functions are duplicated and statics can't be generic. cg_clif duplicates vtables for each cgu and I believe cg_ssa does too.

[RalfJung]

I am not convinced GVN needs to treat them nearly as carefully as it currently does, but I also don't fully understand the current situation. Let's discuss in a new issue: #123670.