rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.97k stars 12.69k forks source link

Equal CTFE function pointers are unequal after codegen #84581

Open tmiasko opened 3 years ago

tmiasko commented 3 years ago
$ cat a.rs 
pub static F: fn() = f::<u32>;
pub fn f<T>() {}
$ cat b.rs 
static F: fn() = a::F;
fn main() {
    assert_eq!(a::F, crate::F);
}
$ rustc --edition=2018 -O --crate-type=lib a.rs 
$ rustc --edition=2018 -O --crate-type=bin b.rs -L. --extern a
$ ./b
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `0x561f15182780`,
 right: `0x561f15182660`'

The general issue is similar to #79738. In this case a function pointer is represented with GlobalAlloc::Function(Instance<'tcx>), which does not uniquely identify function after codegen.

cc @oli-obk

@rustbot modify labels: +A-codegen +A-const-eval

RalfJung commented 3 years ago

Hm, yeah this looks like https://github.com/rust-lang/rust/issues/79738 -- but https://github.com/rust-lang/rust/issues/79738 is more clear since function pointers additionally have a very unstable notion of equality.

@tmiasko do you think this is worth tracking separately from https://github.com/rust-lang/rust/issues/79738?

tmiasko commented 3 years ago

With functions being anonymous allocations being duplicated? Sure, we can close this favour of #79738.

tmiasko commented 6 months ago

Reopening since the fix for #79738, is not sufficient to resolve this issue.

RalfJung commented 6 months ago

I think that's exactly what I talked about here, right? The "value" of the static is some AllocId aka some Instance, but actually the same Instance can turn into different function pointers when used in different crates.

But, is there even a way to tell LLVM that b::F want to point to the instance of f::<u32> that was generated inside a?

tmiasko commented 6 months ago

Yes, it is possible to refer to an instance from a specific crate. This is how share-generics mode is implemented. Presumably, in this case we would like to identify specific instances that would need to be exported, as opposed to exporting all of them.

RalfJung commented 6 months ago

Yes, it is possible to refer to an instance from a specific crate.

How are these even identified inside the compiler? An Instance just refers to any copy of the given instance of that function, right?

Presumably, in this case we would like to identify specific instances that would need to be exported, as opposed to exporting all of them.

So the "instance in the other crate" that we want to refer to needs to be exported before we can reference it?

Yes we'd only want to export those that show up in the value of some static or const. The monomorphization collector already walks all of them to ensure we create the right mono-items:

https://github.com/rust-lang/rust/blob/339f4be04627f192ba29c33ab2251a582241a112/compiler/rustc_monomorphize/src/collector.rs#L1343-L1348

oli-obk commented 6 months ago

We could add a new GlobalAlloc kind that we use in statics to encode function pointers, and then use that to remember which crate a function is from and to always codegen it in the crate of the static if it isn't already codegenned in a dependency. That GlobalAlloc kind will be preserved cross crate and we then need to always refer to the one from the static's crate instead of relying on should_codegen_locally

RalfJung commented 6 months ago

Why would we not do that for our existing GlobalAlloc::Function?

oli-obk commented 6 months ago

Not sure how we'd do that with constants considering we don't know what function pointers they'll contain until we evaluate them downstream (at least if the constant is generic, we could start caching nongeneric or polymorphized constants)

RalfJung commented 6 months ago

GlobalAlloc::Function is never generic I think?

oli-obk commented 6 months ago

Could be with -Zpolymorphize, but I think you're right and we can just always do this, even if it's completely without an effect for constants.

Basically during interning, we also reintern AllocIds that refer to functions, and give them extra information about the crate that they were interned in.

RalfJung commented 6 months ago

With -Zpolymorphize, GlobalAlloc::Function should still be "as monomorphic as" the stuff we actually codegen.

IOW, GlobalAlloc::Function is an Instance and that should always be a single well-defined function. We just currently generate many copies of the same function under certain conditions, so we need some way to refer to a particular copy of that function and need to ensure this is reflected in the GlobalAlloc. As a starting point, this should probably be the copy used by whatever the current crate is when we actually compute the value.