[PATCH] D105733: [OpaquePtr] Require matching signature in getCalledFunction()

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D105733#2868334 <https://reviews.llvm.org/D105733#2868334>, @dexonsmith wrote:

> In D105733#2868258 <https://reviews.llvm.org/D105733#2868258>, @aeubanks wrote:
>
>> we should update the documentation to reflect this and also perhaps make this UB and optimize it away to make it more obvious to a user that something is wrong
>
> I understand that many function declarations that are different with typed pointers resolve to the same type with opaque pointers. But won't we still expect mismatched function types when calling variadic functions? (Are there other legitimate cases, I wonder?)

Yeah, I'm not really sure whether this is always UB or what the exact rules would be. Aligning pointer types is clearly legal, but possibly any cast that is still compatible from an ABI perspective would be legal.

> In D105733#2868262 <https://reviews.llvm.org/D105733#2868262>, @dblaikie wrote:
>
>> Probably a reasonable path forward - though I worry this is creating some subtle technical debt where callers will expect that getCalledFunction's result is null/non-null in the same places where the Function is a direct parameter. (but, yeah, no doubt there's already technical debt that assumes they're the same type - so it's a bit of a loss either way)
>
> Scanning uses in lib/Analysis, a bunch of callers seem to trust the current header docs -- which say that `getCalledFunction()` returns the function unless it's an indirect call -- and continue as if bitcasts aren't a thing. It might be valuable (besides opaque pointer work) to update `getCalledFunction()` to see through the bitcasts even with typed pointers.
>
> E.g., see this code in `DevirtSCCRepeatedPass::run`:
>
>   for (Instruction &I : instructions(N.getFunction()))
>     if (auto *CB = dyn_cast<CallBase>(&I)) {
>       if (CB->getCalledFunction()) {
>         ++Count.Direct;
>       } else {
>         ++Count.Indirect;
>         CallHandles.insert({CB, WeakTrackingVH(CB)});
>       }
>     }

I think that would be best served by a different function. This patch preserves the current behavior of `getCalledFunction()` with opaque pointers (no "bitcasts" allowed) -- we could add an additional function like `getCalledFunctionStripCasts()` that ignores bitcasts (or for opaque pointers, allows signature mismatch) and use that where profitable. Worth noting that we already have isIndirectCall(), which is not the same as getCalledFunction()!=nullptr, that one does consider bitcasts not to be indirect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105733/new/

https://reviews.llvm.org/D105733