[Clang] Fix Sema::checkArgCount for 0-arg functions

[hoodmane]

When calling a function that expects zero arguments with one argument, Call->getArg(1) will trap when trying to format the diagnostic.

This also seems to improve the rendering of the diagnostic some of the time. Before:

$ ./bin/clang -c a.c
a.c:2:30: error: too many arguments to function call, expected 2, have 4
    2 |   __builtin_annotation(1, 2, 3, 4);
      |                           ~  ^

After:

$ ./bin/clang -c a.c
a.c:2:30: error: too many arguments to function call, expected 2, have 4
    2 |   __builtin_annotation(1, 2, 3, 4);
      |                              ^~~~

Split from #139580.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-webassembly

Author: Hood Chatham (hoodmane)

Changes

…ro arguments with one argument

In this case, Call->getArg(1) will trap when trying to format the diagnostic. It also improves the rendering of the diagnostic some of the time: Before:

$ ./bin/clang -c a.c
a.c:2:30: error: too many arguments to function call, expected 2, have 4
    2 |   __builtin_annotation(1, 2, 3, 4);
      |                           ~  ^

After:

$ ./bin/clang -c a.c
a.c:2:30: error: too many arguments to function call, expected 2, have 4
    2 |   __builtin_annotation(1, 2, 3, 4);
      |                              ^~~~

Split from #139580.

---

Full diff: https://github.com/llvm/llvm-project/pull/139638.diff

4 Files Affected:

• (modified) clang/lib/Sema/SemaChecking.cpp (+1-1)
• (modified) clang/lib/Sema/SemaWasm.cpp (+2-6)
• (modified) clang/test/Sema/builtins-wasm.c (+1)
• (modified) clang/test/Sema/builtins.c (+1-1)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 97f623f61a405..5d0d5861d4026 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -168,7 +168,7 @@ bool Sema::checkArgCount(CallExpr *Call, unsigned DesiredArgCount) {
 
   return Diag(Range.getBegin(), diag::err_typecheck_call_too_many_args)
          << 0 /*function call*/ << DesiredArgCount << ArgCount
-         << /*is non object*/ 0 << Call->getArg(1)->getSourceRange();
+         << /*is non object*/ 0 << Range;
 }
 
 static bool checkBuiltinVerboseTrap(CallExpr *Call, Sema &S) {
diff --git a/clang/lib/Sema/SemaWasm.cpp b/clang/lib/Sema/SemaWasm.cpp
index c0fa05bc17609..a76601f38e404 100644
--- a/clang/lib/Sema/SemaWasm.cpp
+++ b/clang/lib/Sema/SemaWasm.cpp
@@ -52,7 +52,7 @@ static bool CheckWasmBuiltinArgIsInteger(Sema &S, CallExpr *E,
 }
 
 bool SemaWasm::BuiltinWasmRefNullExtern(CallExpr *TheCall) {
-  if (TheCall->getNumArgs() != 0)
+  if (SemaRef.checkArgCount(TheCall, 0))
     return true;
 
   TheCall->setType(getASTContext().getWebAssemblyExternrefType());
@@ -62,12 +62,8 @@ bool SemaWasm::BuiltinWasmRefNullExtern(CallExpr *TheCall) {
 
 bool SemaWasm::BuiltinWasmRefNullFunc(CallExpr *TheCall) {
   ASTContext &Context = getASTContext();
-  if (TheCall->getNumArgs() != 0) {
-    Diag(TheCall->getBeginLoc(), diag::err_typecheck_call_too_many_args)
-        << 0 /*function call*/ << /*expected*/ 0 << TheCall->getNumArgs()
-        << /*is non object*/ 0;
+  if (SemaRef.checkArgCount(TheCall, 0))
     return true;
-  }
 
   // This custom type checking code ensures that the nodes are as expected
   // in order to later on generate the necessary builtin.
diff --git a/clang/test/Sema/builtins-wasm.c b/clang/test/Sema/builtins-wasm.c
index beb430616233a..1aae365c95aff 100644
--- a/clang/test/Sema/builtins-wasm.c
+++ b/clang/test/Sema/builtins-wasm.c
@@ -7,6 +7,7 @@ static __externref_t table[0];
 typedef void (*__funcref funcref_t)();
 void test_ref_null() {
   funcref_t func = __builtin_wasm_ref_null_func(0); // expected-error {{too many arguments to function call, expected 0, have 1}}
+  __externref_t ref = __builtin_wasm_ref_null_extern(0); // expected-error {{too many arguments to function call, expected 0, have 1}}
 }
 
 void test_table_size(__externref_t ref, void *ptr, int arr[]) {
diff --git a/clang/test/Sema/builtins.c b/clang/test/Sema/builtins.c
index b669ee68cdd95..310079c8f4ed0 100644
--- a/clang/test/Sema/builtins.c
+++ b/clang/test/Sema/builtins.c
@@ -75,7 +75,7 @@ void test10(void) __attribute__((noreturn));
 
 void test10(void) {
   __asm__("int3");
-  __builtin_unreachable();
+  __builtin_unreachable(1, 2, 3, 6, 7);
 
   // No warning about falling off the end of a noreturn function.
 }

[hoodmane]

I think it would be a good thing to add a test for where the ^~~ goes in a wrong call to __builtin_annotation here but I don't know how. Would appreciate advice.

[erichkeane]

I have no idea how to check the range for the diagnostic here, and I don't think we actually have a way? That part I have no problem with based on your rendering, it seems reasonable to me, and based on the 'range' variable, looks like it was the intent.

The other two changes seem correct as well, though I'm a touch concerned about what BuiltinWasmRefNullExtern was intending to do with a simple return-true. That is, are we sure they weren't intentionally skipping diagnostic there? I'd hope not! If we can find someone responsible/more knowledgable about it before committing, I'd be happier.

[Fznamznon]

A couple of nits, otherwise LGTM

[Fznamznon]

Maybe also assert that ArgCount > 0? In case checkArgCountAtLeast becomes broken.

[Fznamznon]

Suggested change

	if (SemaRef.checkArgCount(TheCall, 0))
	if (SemaRef.checkArgCount(TheCall, /*DesiredArgCount=*/ 0))

[Fznamznon]

Suggested change

	if (SemaRef.checkArgCount(TheCall, 0))
	if (SemaRef.checkArgCount(TheCall, /*DesiredArgCount=*/ 0))

[AaronBallman]

I have no idea how to check the range for the diagnostic here, and I don't think we actually have a way?

-fdiagnostics-print-source-range-info I believe?