[clang] [Sema] Check arg count for builtins with CustomTypeChecking (PR #222841)

[Aaron Ballman via cfe-commits]

=?utf-8?b?yp/htJzJtOG0h3g=?= <imlunex2011 at gmail.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/222841 at github.com>


================
@@ -145,6 +145,16 @@ bool SemaPPC::CheckPPCBuiltinFunctionCall(const TargetInfo &TI,
   //  - IsIntType: enforces any integer type
   // Lambdas centralize type checks for BCD builtin handlers
 
+  // reject calls with more args than the builtin's declared prototype
----------------
AaronBallman wrote:

> okay if i am not wrong, what you are saying here is if someone uses the variadic pattern, i think this check
> 
> ```
> (!FPT->isVariadic() &&
>     SemaRef.checkArgCountAtMost(TheCall, FPT->getNumParams()))
> ```
> 
> should catch that.. for Variadic prototype the check will be skipped totally.

Yup! It means the `CheckArgCountAtMost` lambda will look like it does something when it just early returns because there is no valid signature for a builtin. This is why we see explicit arg counts in SemaChecking.cpp in general when doing custom type checking, like: https://github.com/llvm/llvm-project/blob/0bf534a7b76c7623edb7785cd8b21d89ec63e5d9/clang/lib/Sema/SemaChecking.cpp#L221

> fair point. However, thats a larger refactor because these builtins use BuiltinConstantArgRange to enforce immediate value constraints (Arg2 must be 0 or 1 kind of things) which cant be expressed in the prototype string

But that doesn't require custom type checking. You can still have custom checking logic without specifying custom type checking:

https://github.com/llvm/llvm-project/blob/0bf534a7b76c7623edb7785cd8b21d89ec63e5d9/clang/include/clang/Basic/Builtins.td#L4772
https://github.com/llvm/llvm-project/blob/0bf534a7b76c7623edb7785cd8b21d89ec63e5d9/clang/lib/Sema/SemaChecking.cpp#L6719

so what I was thinking of is removing the custom type checking flag so you get automatic handling for things like arg counts and arg types, but leaving the other custom diagnostic handling in place.

Actually, I think there's a bug here but it may be masked by a different bug.   `__builtin_ppc_national2packed` and friends are not doing any type checking at all. So you can pass invalid types there (https://www.ibm.com/docs/en/xl-c-and-cpp-linux/16.1.1?topic=conversion-builtin-national2packed suggests you can only pass a vector of unsigned char there):

https://godbolt.org/z/4TxaMxe6f should that be accepted?
https://godbolt.org/z/K5Pz83nsW why is this an error about an undeclared identifier `vector`?

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