llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Matt Turner (mattst88) <details> <summary>Changes</summary> The CFG builder's `OmitArguments` mechanism skips argument nodes for calls whose arguments are not actually evaluated, which keeps dataflow analyses such as `-Wuninitialized` from reporting a use that never happens. It was hard-coded for `__builtin_object_size` and `__builtin_dynamic_object_size`. The information is already in the builtin definitions: the `UnevaluatedArguments` attribute, reachable through `CallExpr::isUnevaluatedBuiltinCall()`. This replaces the two explicit builtin-ID checks with that query. That widens the set from two builtins to nine — every builtin carrying `UnevaluatedArguments`. The seven newly covered ones all inspect their argument's type or constant-ness without reading its value: - `__builtin_classify_type` - `__builtin_constant_p` - `__builtin_infer_alloc_token` - `__GetExceptionInfo` - `__builtin_os_log_format_buffer_size` — the size query inspects the format string and the argument types; the arguments are evaluated by the paired `__builtin_os_log_format` call, which is a separate call expression and does not carry the attribute - `__builtin_amdgcn_processor_is` and `__builtin_amdgcn_is_invocable` — AMDGPU compile-time feature predicates Three of these were producing spurious `-Wuninitialized` diagnostics: `__builtin_classify_type`, `__builtin_constant_p` and `__builtin_os_log_format_buffer_size`. The new cases in `clang/test/Sema/uninit-variables.c` warn without this change, so they gate the fix rather than just documenting it. They use one variable per builtin deliberately — the analysis reports only the first use of a given variable, so sharing one would leave all but the first case vacuous. I did not add coverage for the two AMDGPU predicates; they would need an `amdgcn` triple and a separate test file, and they share the mechanism the other cases already pin. Happy to add it if reviewers would prefer. Full `check-clang` passes. --- Full diff: https://github.com/llvm/llvm-project/pull/217816.diff 3 Files Affected: - (modified) clang/docs/ReleaseNotes.md (+7) - (modified) clang/lib/Analysis/CFG.cpp (+1-2) - (modified) clang/test/Sema/uninit-variables.c (+26) ``````````diff diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index 8c9467ca7b742..7c24165fe3cc7 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -238,6 +238,13 @@ features cannot lower the translation-unit ABI level; - More consistent rendering of Unicode characters in diagnostic messages. +- `-Wuninitialized` no longer warns about an uninitialized variable passed to a + builtin that does not evaluate its arguments. It already made this exception + for `__builtin_object_size` and `__builtin_dynamic_object_size`; it now + applies to every builtin declared with unevaluated arguments, including + `__builtin_classify_type`, `__builtin_constant_p` and + `__builtin_os_log_format_buffer_size`. + - Fixed bug in `-Wdocumentation` so that it correctly handles explicit function template instantiations (#64087). diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp index 5263114ebca28..023eff2d50c9d 100644 --- a/clang/lib/Analysis/CFG.cpp +++ b/clang/lib/Analysis/CFG.cpp @@ -2914,8 +2914,7 @@ CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) { if (FD->hasAttr<NoThrowAttr>()) AddEHEdge = false; if (isBuiltinAssumeWithSideEffects(FD->getASTContext(), C) || - FD->getBuiltinID() == Builtin::BI__builtin_object_size || - FD->getBuiltinID() == Builtin::BI__builtin_dynamic_object_size) + C->isUnevaluatedBuiltinCall(*Context)) OmitArguments = true; } diff --git a/clang/test/Sema/uninit-variables.c b/clang/test/Sema/uninit-variables.c index 17e83de5f489a..0feee353258c1 100644 --- a/clang/test/Sema/uninit-variables.c +++ b/clang/test/Sema/uninit-variables.c @@ -583,3 +583,29 @@ void aggregate() { (void)sizeof({ struct with_explicit_field a; a; }); // no warning -- unevaluated operand } + +// Builtins that carry the UnevaluatedArguments attribute never read their +// argument's value -- only its type, or whether it is a constant -- so passing +// an uninitialized variable to one is not a use. CFGBuilder omits the argument +// sub-expressions entirely, which is what keeps them out of this analysis. One +// variable each: the analysis reports only the first use of a given variable, +// so sharing one would let a regression in all but the first go unnoticed. +int unevaluated_builtin_args(void) { + int a, b, c; + char *p, *q; + int classify = __builtin_classify_type(a); // no-warning + int constant = __builtin_constant_p(b); // no-warning + unsigned long size = __builtin_object_size(p, 0); // no-warning + unsigned long dsize = __builtin_dynamic_object_size(q, 0); // no-warning + unsigned long token = __builtin_infer_alloc_token(c); // no-warning + return classify + constant + (int)size + (int)dsize + (int)token; +} + +// __builtin_os_log_format_buffer_size is on the same list: it inspects the +// format string and the argument types to size the buffer. The arguments are +// evaluated by the paired __builtin_os_log_format call, which is a separate +// call expression and is not on the list. +unsigned long unevaluated_os_log_buffer_size(void) { + int x; + return __builtin_os_log_format_buffer_size("%d", x); // no-warning +} `````````` </details> https://github.com/llvm/llvm-project/pull/217816 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
