fn_addr_eq: we actually can guarantee basically nothing

[RalfJung]

View all comments

Fixes #160202.
See that issue for context and discussion.

[rustbot]

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, joboet

[the8472]

One potentially remaining use we were able to come up with is a set of callbacks.

If deduplicating additions to the set by "the runtime behavior should happen at least once" works for you then this should work fine.

View changes since the review

[RalfJung]

That seems incredibly niche.

Would you like to see it spelled out in the docs?

[the8472]

Well, the more general idea was that it might be helpful to also have a list of the few things that it can be used for, it might help users to figure out which side they fall on.

That's just the only practical thing that came to mind.

[asquared31415]

Wouldn't cases like the "different unsafe preconditions" example above cause incorrect results for this kind of use case? Equality returning true does not guarantee that they have the same behavior, so deduplicating in that scenario is not correct.

[RalfJung]

If the callbacks are all required to be safe to call, then calling fewer of them due to deduplication cannot introduce UB.

There is some sense in which equal functions have the same behavior -- they have the same asm code, after all. We might be able to give a guarantee of the sort "if neither function causes UB for the given arguments, then calling f is equivalent to calling g"?

Cc @rust-lang/opsem

[the8472]

Suppose there is a bunch of function pointers. They are compared and called.
All function called are identified at compile time. All calls are inlined.
Functions are never called, so their body is optimized out.
Functions are merged together. All function have the same address.

Ok, having reread that... I guess that depends on whether the comparison lets the function addresses escape like black_box would? Then the optimizer can't remove their bodies. I think even whole-program optimization wouldn't allow it since something could temporarily encrypt the bits of the pointers in some way.

[tmiasko]

Not fully arbitrarily, no. It's still the case that it must be source code that can compile to the same asm as other source code. The docs have a "guaranteed not equal" example of two functions that print different strings; that guarantee sounds legit to me.

The example perfectly demonstrates my argument from #162140 (comment). Those functions are never called. Their bodies could be optimized out as unreachable, function merged, and function pointer would compare equal.

[RalfJung]

Hm... I don't think LLVM considers this a legal transformation today, but I'm also not sure how to forbid it in the spec.

I have removed the example.

[nikic]

Hm... I don't think LLVM considers this a legal transformation today, but I'm also not sure how to forbid it in the spec.

LLVM won't do that transform today, but I think it would be legal? Comparison only escapes the address, not the provenance, so the function cannot be called, and optimizing it to unreachable is allowed.

[clarfonthey]

Would also just add here: in general, while there may be niche cases that this can be legitimately useful, in pretty much all cases you're also encouraged to refactor your code anyway to avoid the pitfalls. For example, if you make your functions instead be ZSTs that implement CustomTrait and use Arc<dyn CustomTrait>, now all of a sudden you can use Arc::ptr_eq and expect it to mean what you want. In general avoiding function pointers is just more likely to always do the correct thing and not be noticeably worse in performance in ways that matter.

View changes since the review

[programmerjake]

I think you mean Arc::ptr_eq.

that said, comparing &dyn CustomTrait with std::ptr::addr_eq also has pitfalls, so I wouldn't necessarily recommend it. e.g. two different &dyn CustomTrait can compare equal with addr_eq, but have different behavior:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=9c9028b37168b6e402b51eb4996b2eef

[clarfonthey]

Right, I forgot that vtables are genuinely different even though the pointers may be the same, especially for ZSTs.

That said, if you use Arc::ptr_eq, you should refer to the same reference-counted version of a dyn CustomTrait, even if the unsized pointers may compare identically. And really, my point was that there are ways of achieving infallible equality in ways that make more sense for your program that don't rely on function pointer equality.

[theemathas]

I believe that this is not legitimate. Suppose that one of the callbacks exposes the provenance of one pointer, while the other callback doesn't, and they have the same machine code. This deduplication can cause a pointer's provenance to not be exposed, even though it would have been exposed without the deduplication. This can technically increase the amount of UB.

View changes since the review

[RalfJung]

If the library just says "I will call some of these callbacks but not all of them", that's legitimate.

[theemathas]

Similar issues can happen without exposed provenance if one of the callbacks mutates some byte by adding provenance to it.

[clarfonthey]

I'm trying to understand how that might cause problems where you run fewer copies of a callback than otherwise expected, though. Like, if the provenance is needed and isn't provided.

[RalfJung]

expose_provenance is a side-effect. If something else relies on the side-effect having happened, then they're toast.

So this amounts to the library basically saying "callbacks may spuriously just not be run for arbitrary reasons". I don't know when that is ever useful, but it's a spec one can write and deduplication with fn_addr_eq does satisfy that spec.

I have edited the text to make this more clear. I'm still not sure this example is worth actually spelling out, but t-libs seems to prefer having an extremely artificial example over not having any example.

[clarfonthey]

To be clear, I'm not 100% sure that's a blocking opinion from libs, just, we really struggled to find examples and wanted to justify this function's existence somehow for a reason other than just documenting how bad function equality is. It does still feel worth having some kind of attribute like #[diagnostic::discouraged] to force people to allow a lint that explains why you almost always do not want to use this function.

Since, well, the main issue is that we know that if we don't offer this function, people are going to try and find a way to hack in its functionality anyway, e.g. casting to non-function pointers. So, we might as well offer a better way if it's already possible, but still heavily discourage it.

[RalfJung]

It does still feel worth having some kind of attribute like #[diagnostic::discouraged] to force people to allow a lint that explains why you almost always do not want to use this function.

I don't disagree, but I'm not signing up for being the one to implement that.

[joshlf]

Maybe just keep:

These limitations imply that comparing function pointers (via this function or via ==) is only useful in extremely niche circumstances.

...and cut the rest?

[scottmcm]

I'm pretty torn here because this feels alot to me like the LLVM bugs we know about around ptr2int+int2ptr, which we've generally filed more under "that's a bug we hope to get fixed" not "we should change the documented behaviour".

Deduping on LLVM-IR is fine, right? So this is just about whether things are also deduping on the machine code? How often are we emitting things where the former isn't doing most of the work? (Certainly having no deduping would be really bad for lots of small functions, but things like deduping swap<u32> and swap<i32> don't need binary-level deduping.)

[traviscross]

Makes sense to me. Thanks @RalfJung.

(Proposing FCP as we're retracting language guarantees. Based on discussion on the call, people will likely leave some review comments about strengthening the guidance to users in the text without strengthening the guarantees.)

@rfcbot fcp merge lang

[rust-rfcbot]

@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.

[nia-e]

An additional note per today's lang meeting: there was some support for a carveout for the case of both function pointers being controlled by the caller and known to have different observable behaviour. There exist hypothetical semantics where these could still be merged, so we'd be ruling out such semantics.

[joshtriplett]

We talked in today's @rust-lang/lang meeting about this.

Based on discussion from myself, @nikomatsakis, @tmandry, and @nia-e, we'd like to avoid going quite as far as saying it's almost never useful.

@nia-e wrote a comment about potential language-level guarantees we could still make, regarding functions with distinct observable behavior.

@nikomatsakis and I both also wanted to note that the documentation on the usefulness of this function should allow for the fact that you may be able to make productive use of it if you have detailed knowledge of implementation-specific behavior. Not being able to make as many language-level guarantees is not the same as being unable to use it at all.

[nikic]

I'm pretty torn here because this feels alot to me like the LLVM bugs we know about around ptr2int+int2ptr, which we've generally filed more under "that's a bug we hope to get fixed" not "we should change the documented behaviour".

I don't believe there are any LLVM bugs involved here. The behavior here is a consequence of Rust considering functions unnamed_addr. The alternative to this PR would be to not mark functions unnamed_addr. This still allows function merging, but requires the generation of thunks to preserve identity.

Deduping on LLVM-IR is fine, right? So this is just about whether things are also deduping on the machine code? How often are we emitting things where the former isn't doing most of the work? (Certainly having no deduping would be really bad for lots of small functions, but things like deduping swap<u32> and swap<i32> don't need binary-level deduping.)

I don't think whether you dedup on LLVM IR or on the binary is relevant, because LLVM IR can be refined (towards being more defined).

[RalfJung]

Deduping on LLVM-IR is fine, right?

No. LLVM could just as well

• inline the function
• remove the unreachable_unchecked in the function
• dedup them because now the IR is the same

This is not an LLVM bug, it is a fundamental consequence of allowing deduplication of items based on whether they are equal after some transformations. If you accept that, you have to accept this as a consequence; everything in between is just logic. The only way to avoid this weirdness would be to tag functions with some sort of hash before we do any transformations (say, we hash the HIR or so), and then only allowing functions with the same hash to be merged.

@rfcbot fcp merge lang

Would you mind restarting this with opsem included? Since this is basically "do function pointers have provenance", I think opsem should be involved.

[RalfJung]

@nia-e

An additional note per today's lang meeting: there was some support for a carveout for the case of both function pointers being controlled by the caller and known to have different observable behaviour. There exist hypothetical semantics where these could still be merged, so we'd be ruling out such semantics.

We're ruling out code that relies on such semantics, for now. We can strengthen the guarantees later if we are confident that that's sound. This PR is a breaking change; reverting it is not.

@joshtriplett

@nia-e wrote a comment about potential language-level guarantees we could still make, regarding functions with distinct observable behavior.

@nikic confirmed that currently we can not easily make such a guarantee. If we accept that LLVM can do arbitrary changes to code that is provably unreachable, it follows that even functions with distinct observable behavior can be merged.

The extreme case of that which we discussed above might be unrealistic, but here's a version of this that sounds plausible:

• Function A and B have type fn(bool).
• Function A always prints "hello".
• Function B only prints "hello" if the argument is true, otherwise it prints "world".
• Note that these functions have distinct observable behavior.
• LLVM can do interprocedural range analysis to determine that B is only ever called with true. It therefore removes the branch and the possibility of printing "world".
• Now the two functions have the same IR and can be merged.

I think we should allow LLVM to do interprocedural analyses. Given that, I don't see any way to constrain function merging that gives us meaningful guarantees. We have to somehow define the set of merge candidates on the Rust AM level, and the example above demonstrates that "only functions A, B such that there exists a C that refines both A and B can be merged" does not work. Whatever definition we take has to somehow account for how the functions actually get invoked by the program. I am not sure what such a definition would look like, it sure wouldn't be pretty.

I think the only practical options here are:

• Status quo, basically everything can be merged.
• Remove unnamed_addr, nothing can be merged.
• Develop a new mechanism where the frontend tells LLVM about which functions may be merged, and which have to remain distinct. (That's the "tag functions with a hash" I mentioned in the previous comment.)

@nikomatsakis and I both also wanted to note that the documentation on the usefulness of this function should allow for the fact that you may be able to make productive use of it if you have detailed knowledge of implementation-specific behavior. Not being able to make as many language-level guarantees is not the same as being unable to use it at all.

I am not sure what exactly you have in mind here. Are you thinking of something like "functions will only be merged if they have the same LLVM IR or the same assembly at some point during the compilation"? I don't know what productive use of this guarantee could look like.

The inability to make language-level guarantees come from the inability to predict what exactly LLVM will do. Being willing to accept implementation-specific behavior doesn't make this significantly easier to predict. Sure, you can check the assembly to verify that two functions have not been merged, but then that may change after a rustc update, or after you change any part of the code and therefore steer LLVM's heuristics in a different direction.

[ChrisDenton]

Trying to desperately rescue fn_addr_eq is laudable but I would like to see us be able to point users towards something that is much less of a footgun, if that's possible. I'm not expecting that to be designed today but surely we can give the users an attribute to put on functions they wish to be distinct so the compiler can do everything possible to enable that use case? That would ultimately seem a more fruitful direction to me.

[RalfJung]

An attribute on monomorphic functions to remove unnamed_addr would be possible, yeah. (It would also have to affect our cross-crate inlining heuristics.) For generic functions we can also suppress unnamed_addr but it'd be a lot less useful.

[traviscross]

@rfcbot fcp cancel

[rust-rfcbot]

@traviscross proposal cancelled.

[traviscross]

@rfcbot fcp merge lang,opsem

[rust-rfcbot]

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

• @CAD97
• @RalfJung
• @digama0
• @joshlf
• @joshtriplett
• @nikomatsakis
• @saethlin
• @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.

[nikic]

For the record, #162209 has an experiment to not mark functions as unnamed_addr. The perf results show that not using unnamed_addr does have significant impact on binary size (e.g. 10% regression on diesel).