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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

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?)

(Does removing code make UB obvious to users? If obviousness is the goal (not runtime performance) I wonder if something could be hooked up in UBSan instead? or a guaranteed crash?)

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)});
      }
    }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105733