================
@@ -136,13 +136,14 @@ const SmallVector<VAListChecker::VAListAccepter, 15>
const CallDescription VAListChecker::VaStart(CDM::CLibrary,
{"__builtin_va_start"},
/*Args=*/2,
/*Params=*/1),
+ VAListChecker::VaStartC23(CDM::CLibrary, {"__builtin_c23_va_start"}),
----------------
NagyDonat wrote:
I realized that this PR may expose a sneaky corner case.
Notice that the call description for `__builtin_va_start` only matches calls
where two arguments are specified (and the declaration has one parameter –
don't ask why this clang builtin claims to have one parameter, but apparently
it works). This pedantry ensures that `checkVAListStartCall` can rely on the
presence of two arguments. [1]
For `VAStartC23` you were right to omit the `Args` field (i.e. allow matching
any argument count) because you need to match `__builtin_c23_va_start` calls
with 1, 2 and perhaps more arguments. However, this way if the analyzer
encounters the grossly invalid expression `__builtin_c23_va_start()` (call with
no arguments), then `checkVAListStartCall` will receive a call with no
arguments, and there I fear that calls like `Call.getArgSVal(0)` (= get the
value of argument index 0) will cause an analyzer crash / assertion failure.
E.g. on current trunk the analyzer can parse [this degenerate source
file](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:17,fontUsePx:'0',j:1,lang:___c,selection:(endColumn:1,endLineNumber:5,positionColumn:1,positionLineNumber:5,selectionStartColumn:1,selectionStartLineNumber:5,startColumn:1,startLineNumber:5),source:'/*+Type+your+code+here,+or+load+an+example.+*/%0Aint+foo(void)+%7B%0A++++__builtin_c23_va_start()%3B%0A%7D%0A'),l:'5',n:'0',o:'C+source+%231',t:'0')),k:48.76414273281115,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:cclang_trunk,filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'1',intel:'0',libraryCode:'1',trim:'0',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:___c,libs:!(),options:'--analyze+-Wno-implicit-function-declaration',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+(trunk)+(Editor+%231)',t:'0')),k:46.688710992440406,l:'4',m:45.03774363106201,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+clang+19.1.0+(assertions)',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'0'),l:'5',n:'0',o:'Output+of+x86-64+clang+(trunk)+(Compiler+%231)',t:'0')),header:(),l:'4',m:54.962256368938,n:'0',o:'',s:0,t:'0')),k:51.23585726718886,l:'3',n:'0',o:'',t:'0')),l:'2',m:100,n:'0',o:'',t:'0')),version:4)
(in non-C23 mode if `-Wno-implicit-function-declaration` suppresses an error),
but with your patch I suspect (although I'm not 100% sure) that this would
crash the analyzer.
To resolve this (mostly theoretical) issue, I would suggest adding
```c++
if (Call.getNumArgs() == 0)
return; // Prevent a crash on grossly invalid code.
```
at the beginning of `checkVAListStartCall`.
After doing this, you can remove the `/*Args=*/ 2` and `/*Params=*/ 1` fields
of `VaStart` because they won't be relevant anymore (and the difference between
them may confuse the readers).
--------
[1] This is a common pattern: instead of writing ad hoc `if`s, the argument
count validation is traditionally specified in call description matching. This
is especially relevant for other checkers that match functions whose name may
reasonably collide with unrelated user-defined functions.
https://github.com/llvm/llvm-project/pull/192024
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
