[clang] [clang] Fix crash on no-prototype indirect calls with -fexperimental-call-graph-section (PR #210846)

[Roland McGrath via cfe-commits]

================
@@ -6350,13 +6351,39 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
       else if (const auto *FPT =
                    Callee.getAbstractInfo().getCalleeFunctionProtoType())
         CST = QualType(FPT, 0);
+      else if (const auto *FT =
+                   Callee.getAbstractInfo().getCalleeFunctionType())
+        CST = QualType(FT, 0);
       else
         llvm_unreachable(
             "Cannot find the callee type to generate callee_type metadata.");
 
       // Set type identifier metadata of indirect calls for call graph section.
-      if (!CST.isNull())
+      if (!CST.isNull()) {
+        if (!CST->isFunctionProtoType()) {
+          // For unprototyped callees, reconstruct a prototype from the argument
+          // types passed at the call site (after default argument promotion).
----------------
frobtech wrote:

I think it merits some more detailed comments here about the reasoning, and a mention of possible alternatives / complications.

In particular, the basic rationale of using the types after default argument promotions is that these are the types that the actual callee's definition must be using in its prototype-style definition for the call to be thoroughly valid.  However, some further wrinkles are worth noting.

Firstly, if the actual definition is K&R-style, then (_I think_) the canonical semantics is that the K&R-declared type of the parameter is actually promoted for the expected type used in the call, and then the callee is implicitly casting to the declared type.  So e.g. `void foo(x) short x; { ... }` actually expects to be called with an `int` even though the `x` parameter variable inside the function will be `short`.  So by that logic, at a K&R definition site, the "what is my own signature" computation there ought to actually be `void(int)` rather than `void(short)` as the same function declared as a prototype would yield.

The second general class of wrinkles is about the strictness of intent in the source that you want to hold for your signature matching.  We don't have many options: we're comparing a hash, so the only matching is exact equality.  Thus the only choices to make are in normalization before hashing.

If you decide your program is only "correct" if caller and callee used precisely the same types, you can be very strict: basically no normalization.  It seems wise to apply default argument promotions at least, since deeming a caller "incorrect" for passing `(short)x + 1` for a parameter declared `short` (where the expression is promoted to `int` regardless--and in a prototype-declared call would technically be re-narrowed in the call!) seems a bridge too far.

But what beyond default argument promotions?  If you want to consider any call that's technically valid in standard C, then each argument only has to be [compatible](https://cppreference.com/c/language/type#Compatible_types) and even gets a few exceptions from that such as for signedness.

With `-fsanitize-cfi-icall-experimental-normalize-integers`, there is normalization for different integer types with entirely the same ABI (same bit-width, same signedness).  AFAICT that's actually not "compatible" by the C standard, e.g. `int` vs `long` or `long` vs `long long` for a pair that's the same actual size.

But that doesn't normalize away signedness, while a mismatch of signedness is actually an exception beyond "compatible" that's valid in standard C for calls to functions declared without prototypes (or for a call to a variadic function vs the type passed to `va_arg`).  So if you have a callee that takes an `unsigned int` and you want callers that say `0` to be considered in its equivalence set for possible callees, you need to normalize away signedness.  For CFI, it seems very plausible that you wouldn't want to loosen that way and consider such a callee valid for that caller.  But for call graph analysis perhaps you'd rather err on the side of considering a callee possible.  Do we need a separate switch to decide about that normalization?  Does it need to be separate for CFI vs call-graph?

OTOH, "compatible" admits e.g. any `enum` type vs its underlying type.  So do you want to normalize away to basically all integer types of a given bit-width being the same, whether signed or not, `enum` or not?  Again, it depends on whether you'd rather err on the side of considering every callee that C rules actually make it not UB to use for a given function pointer, or err on the side of excluding things it seems like code that's especially clean would definitely avoid.

The most fundamental thing to make sure is well-noted in this code is that the normalization done here (even if just default promotions) must stay mated with the normalization logic done to choose a self-signature at a K&R-style definition site.

https://github.com/llvm/llvm-project/pull/210846