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

hoodmane · 2025-05-12T23:07:11Z

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.

…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 llvm#139580

llvmbot · 2025-05-12T23:07:39Z

@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 · 2025-05-12T23:17:03Z

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 · 2025-05-13T13:23:37Z

clang/lib/Sema/SemaChecking.cpp

@@ -168,7 +168,7 @@ bool Sema::checkArgCount(CallExpr *Call, unsigned DesiredArgCount) {



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

I think that's covered by:

assert(ArgCount > DesiredArgCount && "should have diagnosed this");

since DesiredArgCount is nonnegative.

clang/lib/Sema/SemaWasm.cpp

AaronBallman · 2025-05-13T13:27:59Z

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?

hoodmane · 2025-05-13T15:21:26Z

are we sure they weren't intentionally skipping diagnostic there?

The behavior before was

error: cannot compile this scalar expression yet
    2 |   __externref_t ref = __builtin_wasm_ref_null_extern(0);
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
  ...

And there was no test coverage. So I'm pretty confident that it was a mistake.

erichkeane · 2025-05-13T15:24:45Z

And there was no test coverage. So I'm pretty confident that it was a mistake.

THAT is not a conclusion we can properly draw here :) I think you'd be shocked how bad our test coverage can be with some of these things.

The diagnostic we have "cannot compile this scalar yet" implies to me that someone THOUGHT about this, and that it would be usable with multiple args 'some day'. But yes, I'm OK now with having a diagnostic there since it couldn't be emitted anyway. We'll see what fallout happens,

github-actions · 2025-05-13T15:27:10Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Co-authored-by: Mariya Podchishchaeva <[email protected]>

hoodmane · 2025-05-13T15:33:06Z

I am quite confident this was never intended to have any arguments since the instruction that it lowers to has no arguments and never is going to gain arguments:
https://webassembly.github.io/spec/core/syntax/instructions.html#reference-instructions

BuiltinWasmRefNullExtern should be exactly parallel to BuiltinWasmRefNullFunc, any differences in logic in the two is a mistake. @sbc100 can back me up on this as the target expert.

hoodmane · 2025-05-13T15:34:44Z

Thanks for the reviews @AaronBallman and @erichkeane!

dschuff

LGTM from the wasm side. As @hoodmane noted, the underlying wasm instruction has no arguments.

hoodmane · 2025-05-13T16:49:48Z

Can somebody merge when it's appropriate? I don't have merge rights.

AaronBallman · 2025-05-13T16:54:42Z

Can somebody merge when it's appropriate? I don't have merge rights.

Yup, we can land for you once precommit CI goes green. Thank you for letting us know!

hoodmane · 2025-05-13T19:59:48Z

CI looks green now.

llvmbot added clang Clang issues not falling into any other category backend:WebAssembly clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 12, 2025

hoodmane changed the title ~~[Clang] Fix Sema::checkArgCount when calling a function that wants ze…~~ [Clang] Fix Sema::checkArgCount for 0-arg functions May 12, 2025

Revert accidental change

f0ca59b

AaronBallman requested review from AaronBallman, erichkeane, Fznamznon and Sirraide May 13, 2025 13:02

erichkeane approved these changes May 13, 2025

View reviewed changes

Fznamznon approved these changes May 13, 2025

View reviewed changes

Apply suggestions from code review

94b7497

Co-authored-by: Mariya Podchishchaeva <[email protected]>

hoodmane force-pushed the fix-sema-check-arg-count-0-args branch from 3654df3 to 94b7497 Compare May 13, 2025 15:31

dschuff approved these changes May 13, 2025

View reviewed changes

erichkeane merged commit 3b3adef into llvm:main May 13, 2025
11 checks passed

hoodmane deleted the fix-sema-check-arg-count-0-args branch May 13, 2025 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hoodmane commented May 12, 2025 •

edited

Loading

llvmbot commented May 12, 2025 •

edited

Loading

hoodmane commented May 12, 2025

erichkeane left a comment

Fznamznon left a comment

Fznamznon May 13, 2025

hoodmane May 13, 2025

AaronBallman commented May 13, 2025

hoodmane commented May 13, 2025

erichkeane commented May 13, 2025

github-actions bot commented May 13, 2025 •

edited

Loading

hoodmane commented May 13, 2025

hoodmane commented May 13, 2025

dschuff left a comment

hoodmane commented May 13, 2025

AaronBallman commented May 13, 2025

hoodmane commented May 13, 2025

		@@ -168,7 +168,7 @@ bool Sema::checkArgCount(CallExpr *Call, unsigned DesiredArgCount) {

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

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

Conversation

hoodmane commented May 12, 2025 • edited Loading

llvmbot commented May 12, 2025 • edited Loading

hoodmane commented May 12, 2025

erichkeane left a comment

Choose a reason for hiding this comment

Fznamznon left a comment

Choose a reason for hiding this comment

Fznamznon May 13, 2025

Choose a reason for hiding this comment

hoodmane May 13, 2025

Choose a reason for hiding this comment

AaronBallman commented May 13, 2025

hoodmane commented May 13, 2025

erichkeane commented May 13, 2025

github-actions bot commented May 13, 2025 • edited Loading

hoodmane commented May 13, 2025

hoodmane commented May 13, 2025

dschuff left a comment

Choose a reason for hiding this comment

hoodmane commented May 13, 2025

AaronBallman commented May 13, 2025

hoodmane commented May 13, 2025

hoodmane commented May 12, 2025 •

edited

Loading

llvmbot commented May 12, 2025 •

edited

Loading

github-actions bot commented May 13, 2025 •

edited

Loading