[clang] [analyzer] Clean up apiModeling.llvm.ReturnValue (PR #91231)
https://github.com/NagyDonat closed https://github.com/llvm/llvm-project/pull/91231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Clean up apiModeling.llvm.ReturnValue (PR #91231)
https://github.com/steakhal approved this pull request. LGTM. I have nothing to add here. https://github.com/llvm/llvm-project/pull/91231 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Clean up apiModeling.llvm.ReturnValue (PR #91231)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Donát Nagy (NagyDonat)
Changes
This commit heavily refactors and simplifies the small and trivial checker
`apiModeling.llvm.ReturnValue`, which is responsible for modeling the peculiar
coding convention that in the LLVM/Clang codebase certain Error() methods
always return true.
Changes included in this commit:
- The call description mode is now specified explicitly (this is not the most
significant change, but it was the original reason for touching this checker).
- Previously the code provided support for modeling functions that always
return `false`; but there was no need for that, so this commit hardcodes that
the return value is `true`.
- The overcomplicated constraint/state handling logic was simplified.
- The separate `checkEndFunction` callback was removed to simplify the code.
Admittedly this means that the note tag for the "method returns false,
breaking the convention" case is placed on the method call instead of the
`return` statement; but that case will _never_ appear in practice, so this
difference is mostly academical.
- The text of the note tags was clarified.
- The descriptions in the header comment and Checkers.td were clarified.
- Some minor cleanup was applied in the associated test file.
This change is very close to NFC because it only affects a hidden
`apiModeling.llvm` checker that's only relevant during the analysis of the
LLVM/Clang codebase, and even there it doesn't affect the normal behavior of
the checker.
---
Full diff: https://github.com/llvm/llvm-project/pull/91231.diff
3 Files Affected:
- (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+1-1)
- (modified) clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp (+45-111)
- (modified) clang/test/Analysis/return-value-guaranteed.cpp (+33-33)
``diff
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 520286b57c9fdc..64414e3d37f751 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1397,7 +1397,7 @@ def CastValueChecker : Checker<"CastValue">,
Documentation;
def ReturnValueChecker : Checker<"ReturnValue">,
- HelpText<"Model the guaranteed boolean return value of function calls">,
+ HelpText<"Model certain Error() methods that always return true by
convention">,
Documentation;
} // end "apiModeling.llvm"
diff --git a/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
index c3112ebe4e7941..3da571adfa44cc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
@@ -1,4 +1,4 @@
-//===- ReturnValueChecker - Applies guaranteed return values *- C++
-*-===//
+//===- ReturnValueChecker - Check methods always returning true -*- C++
-*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -6,8 +6,13 @@
//
//===--===//
//
-// This defines ReturnValueChecker, which checks for calls with guaranteed
-// boolean return value. It ensures the return value of each function call.
+// This defines ReturnValueChecker, which models a very specific coding
+// convention within the LLVM/Clang codebase: there several classes that have
+// Error() methods which always return true.
+// This checker was introduced to eliminate false positives caused by this
+// peculiar "always returns true" invariant. (Normally, the analyzer assumes
+// that a function returning `bool` can return both `true` and `false`, because
+// otherwise it could've been a `void` function.)
//
//===--===//
@@ -18,43 +23,40 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/FormatVariadic.h"
#include
using namespace clang;
using namespace ento;
+using llvm::formatv;
namespace {
-class ReturnValueChecker : public Checker
{
+class ReturnValueChecker : public Checker {
public:
- // It sets the predefined invariant ('CDM') if the current call not break it.
void checkPostCall(const CallEvent , CheckerContext ) const;
- // It reports whether a predefined invariant ('CDM') is broken.
- void checkEndFunction(const ReturnStmt *RS, CheckerContext ) const;
-
private:
- // The pairs are in the following form: {{{class, call}}, return value}
- const CallDescriptionMap CDM = {
+ const CallDescriptionSet Methods = {
// These are known in the LLVM project: 'Error()'
- {{{"ARMAsmParser", "Error"}}, true},
- {{{"HexagonAsmParser", "Error"}},
[clang] [analyzer] Clean up apiModeling.llvm.ReturnValue (PR #91231)
https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/91231 This commit heavily refactors and simplifies the small and trivial checker `apiModeling.llvm.ReturnValue`, which is responsible for modeling the peculiar coding convention that in the LLVM/Clang codebase certain Error() methods always return true. Changes included in this commit: - The call description mode is now specified explicitly (this is not the most significant change, but it was the original reason for touching this checker). - Previously the code provided support for modeling functions that always return `false`; but there was no need for that, so this commit hardcodes that the return value is `true`. - The overcomplicated constraint/state handling logic was simplified. - The separate `checkEndFunction` callback was removed to simplify the code. Admittedly this means that the note tag for the " returns false, breaking the convention" case is placed on the method call instead of the `return` statement; but that case will _never_ appear in practice, so this difference is mostly academical. - The text of the note tags was clarified. - The descriptions in the header comment and Checkers.td were clarified. - Some minor cleanup was applied in the associated test file. This change is very close to NFC because it only affects a hidden `apiModeling.llvm` checker that's only relevant during the analysis of the LLVM/Clang codebase, and even there it doesn't affect the normal behavior of the checker. From be5b3add4fb563afbf466d6013104b80676d4475 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Mon, 6 May 2024 17:31:20 +0200 Subject: [PATCH] [analyzer] Clean up apiModeling.llvm.ReturnValue This commit heavily refactors and simplifies the small and trivial checker `apiModeling.llvm.ReturnValue`, which is responsible for modeling the peculiar coding convention that in the LLVM/Clang codebase certain Error() methods always return true. Changes included in this commit: - The call description mode is now specified explicitly (this is not the most significant change, but itwas the original reason for touching this checker). - Previously the code provided support for modeling functions that always return `false`; but there was no need for that, so this commit hardcodes that the return value is `true`. - The overcomplicated constraint/state handling logic was simplified. - The separate `checkEndFunction` callback was removed to simplify the code. Admittedly this means that the note tag for the " returns false, breaking the convention" case is placed on the method call instead of the `return` statement; but that case will _never_ appear in practice, so this difference is mostly academical. - The text of the note tags was clarified. - The descriptions in the header comment and Checkers.td were clarified. - Some minor cleanup was applied in the associated test file. This change is very close to NFC because it only affects a hidden `apiModeling.llvm` checker that's only relevant during the analysis of the LLVM/Clang codebase, and even there it doesn't affect the normal behavior of the checker. --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 2 +- .../Checkers/ReturnValueChecker.cpp | 156 +- .../test/Analysis/return-value-guaranteed.cpp | 66 3 files changed, 79 insertions(+), 145 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 520286b57c9fdc..64414e3d37f751 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1397,7 +1397,7 @@ def CastValueChecker : Checker<"CastValue">, Documentation; def ReturnValueChecker : Checker<"ReturnValue">, - HelpText<"Model the guaranteed boolean return value of function calls">, + HelpText<"Model certain Error() methods that always return true by convention">, Documentation; } // end "apiModeling.llvm" diff --git a/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp index c3112ebe4e7941..3da571adfa44cc 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp @@ -1,4 +1,4 @@ -//===- ReturnValueChecker - Applies guaranteed return values *- C++ -*-===// +//===- ReturnValueChecker - Check methods always returning true -*- C++ -*-===// // // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // See https://llvm.org/LICENSE.txt for license information. @@ -6,8 +6,13 @@ // //===--===// // -// This defines ReturnValueChecker, which checks for calls with guaranteed -// boolean return value. It ensures the return value of each function call. +// This defines ReturnValueChecker,
