[clang] [clang-tools-extra] [clang-tidy][dataflow] Add `bugprone-null-check-after-dereference` check (PR #84166)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 4 06:21:55 PDT 2024
https://github.com/Discookie updated https://github.com/llvm/llvm-project/pull/84166
>From 704d175fde121edaf962614d8c8d626bf8dbf156 Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Wed, 6 Mar 2024 14:10:44 +0000
Subject: [PATCH 01/14] [clang][dataflow] Add null-check after dereference
checker
---
.../bugprone/BugproneTidyModule.cpp | 3 +
.../clang-tidy/bugprone/CMakeLists.txt | 1 +
.../NullCheckAfterDereferenceCheck.cpp | 171 +++++
.../bugprone/NullCheckAfterDereferenceCheck.h | 37 ++
clang-tools-extra/clangd/TidyProvider.cpp | 3 +-
.../bugprone/null-check-after-dereference.rst | 162 +++++
.../bugprone/null-check-after-dereference.cpp | 330 +++++++++
.../Models/NullPointerAnalysisModel.h | 112 ++++
.../FlowSensitive/Models/CMakeLists.txt | 1 +
.../Models/NullPointerAnalysisModel.cpp | 625 ++++++++++++++++++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../NullPointerAnalysisModelTest.cpp | 332 ++++++++++
12 files changed, 1777 insertions(+), 1 deletion(-)
create mode 100644 clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
create mode 100644 clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
create mode 100644 clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
create mode 100644 clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index a8a23b045f80b..ddd708dd51335 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -48,6 +48,7 @@
#include "NoEscapeCheck.h"
#include "NonZeroEnumToBoolConversionCheck.h"
#include "NotNullTerminatedResultCheck.h"
+#include "NullCheckAfterDereferenceCheck.h"
#include "OptionalValueConversionCheck.h"
#include "ParentVirtualCallCheck.h"
#include "PosixReturnCheck.h"
@@ -180,6 +181,8 @@ class BugproneModule : public ClangTidyModule {
CheckFactories.registerCheck<PosixReturnCheck>("bugprone-posix-return");
CheckFactories.registerCheck<ReservedIdentifierCheck>(
"bugprone-reserved-identifier");
+ CheckFactories.registerCheck<NullCheckAfterDereferenceCheck>(
+ "bugprone-null-check-after-dereference");
CheckFactories.registerCheck<SharedPtrArrayMismatchCheck>(
"bugprone-shared-ptr-array-mismatch");
CheckFactories.registerCheck<SignalHandlerCheck>("bugprone-signal-handler");
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 1cd6fb207d762..5dbe761cb810e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -44,6 +44,7 @@ add_clang_library(clangTidyBugproneModule
NoEscapeCheck.cpp
NonZeroEnumToBoolConversionCheck.cpp
NotNullTerminatedResultCheck.cpp
+ NullCheckAfterDereferenceCheck.cpp
OptionalValueConversionCheck.cpp
ParentVirtualCallCheck.cpp
PosixReturnCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
new file mode 100644
index 0000000000000..7ef3169cc6386
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
@@ -0,0 +1,171 @@
+//===--- NullCheckAfterDereferenceCheck.cpp - clang-tidy-------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "NullCheckAfterDereferenceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/ControlFlowContext.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h"
+#include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/Any.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Error.h"
+#include <memory>
+#include <vector>
+
+namespace clang::tidy::bugprone {
+
+using ast_matchers::MatchFinder;
+using dataflow::NullCheckAfterDereferenceDiagnoser;
+using dataflow::NullPointerAnalysisModel;
+
+static constexpr llvm::StringLiteral FuncID("fun");
+
+struct ExpandedResult {
+ SourceLocation WarningLoc;
+ std::optional<SourceLocation> DerefLoc;
+};
+
+using ExpandedResultType =
+ std::pair<std::vector<ExpandedResult>, std::vector<ExpandedResult>>;
+
+static std::optional<ExpandedResultType>
+analyzeFunction(const FunctionDecl &FuncDecl) {
+ using dataflow::ControlFlowContext;
+ using dataflow::DataflowAnalysisState;
+ using llvm::Expected;
+
+ ASTContext &ASTCtx = FuncDecl.getASTContext();
+
+ if (FuncDecl.getBody() == nullptr) {
+ return std::nullopt;
+ }
+
+ Expected<ControlFlowContext> Context =
+ ControlFlowContext::build(FuncDecl, *FuncDecl.getBody(), ASTCtx);
+ if (!Context)
+ return std::nullopt;
+
+ dataflow::DataflowAnalysisContext AnalysisContext(
+ std::make_unique<dataflow::WatchedLiteralsSolver>());
+ dataflow::Environment Env(AnalysisContext, FuncDecl);
+ NullPointerAnalysisModel Analysis(ASTCtx);
+ NullCheckAfterDereferenceDiagnoser Diagnoser;
+ NullCheckAfterDereferenceDiagnoser::ResultType Diagnostics;
+
+ using LatticeState = DataflowAnalysisState<NullPointerAnalysisModel::Lattice>;
+ using DetailMaybeLatticeStates = std::vector<std::optional<LatticeState>>;
+
+ auto DiagnoserImpl = [&ASTCtx, &Diagnoser,
+ &Diagnostics](const CFGElement &Elt,
+ const LatticeState &S) mutable -> void {
+ auto EltDiagnostics = Diagnoser.diagnose(ASTCtx, &Elt, S.Env);
+ llvm::move(EltDiagnostics.first, std::back_inserter(Diagnostics.first));
+ llvm::move(EltDiagnostics.second, std::back_inserter(Diagnostics.second));
+ };
+
+ Expected<DetailMaybeLatticeStates> BlockToOutputState =
+ dataflow::runDataflowAnalysis(*Context, Analysis, Env, DiagnoserImpl);
+
+ if (llvm::Error E = BlockToOutputState.takeError()) {
+ llvm::dbgs() << "Dataflow analysis failed: " << llvm::toString(std::move(E))
+ << ".\n";
+ return std::nullopt;
+ }
+
+ ExpandedResultType ExpandedDiagnostics;
+
+ llvm::transform(Diagnostics.first,
+ std::back_inserter(ExpandedDiagnostics.first),
+ [&](SourceLocation WarningLoc) -> ExpandedResult {
+ if (auto Val = Diagnoser.WarningLocToVal[WarningLoc];
+ auto DerefExpr = Diagnoser.ValToDerefLoc[Val]) {
+ return {WarningLoc, DerefExpr->getBeginLoc()};
+ }
+
+ return {WarningLoc, std::nullopt};
+ });
+
+ llvm::transform(Diagnostics.second,
+ std::back_inserter(ExpandedDiagnostics.second),
+ [&](SourceLocation WarningLoc) -> ExpandedResult {
+ if (auto Val = Diagnoser.WarningLocToVal[WarningLoc];
+ auto DerefExpr = Diagnoser.ValToDerefLoc[Val]) {
+ return {WarningLoc, DerefExpr->getBeginLoc()};
+ }
+
+ return {WarningLoc, std::nullopt};
+ });
+
+ return ExpandedDiagnostics;
+}
+
+void NullCheckAfterDereferenceCheck::registerMatchers(MatchFinder *Finder) {
+ using namespace ast_matchers;
+
+ auto hasPointerValue =
+ hasDescendant(NullPointerAnalysisModel::ptrValueMatcher());
+ Finder->addMatcher(
+ decl(anyOf(functionDecl(unless(isExpansionInSystemHeader()),
+ // FIXME: Remove the filter below when lambdas are
+ // well supported by the check.
+ unless(hasDeclContext(cxxRecordDecl(isLambda()))),
+ hasBody(hasPointerValue)),
+ cxxConstructorDecl(hasAnyConstructorInitializer(
+ withInitializer(hasPointerValue)))))
+ .bind(FuncID),
+ this);
+}
+
+void NullCheckAfterDereferenceCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ return;
+
+ const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(FuncID);
+ assert(FuncDecl && "invalid FuncDecl matcher");
+ if (FuncDecl->isTemplated())
+ return;
+
+ if (const auto Diagnostics = analyzeFunction(*FuncDecl)) {
+ const auto &[CheckWhenNullLocations, CheckAfterDereferenceLocations] =
+ *Diagnostics;
+
+ for (const auto [WarningLoc, DerefLoc] : CheckAfterDereferenceLocations) {
+ diag(WarningLoc, "pointer value is checked even though "
+ "it cannot be null at this point");
+
+ if (DerefLoc) {
+ diag(*DerefLoc,
+ "one of the locations where the pointer's value cannot be null",
+ DiagnosticIDs::Note);
+ }
+ }
+
+ for (const auto [WarningLoc, DerefLoc] : CheckWhenNullLocations) {
+ diag(WarningLoc,
+ "pointer value is checked but it can only be null at this point");
+
+ if (DerefLoc) {
+ diag(*DerefLoc,
+ "one of the locations where the pointer's value can only be null",
+ DiagnosticIDs::Note);
+ }
+ }
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.h b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.h
new file mode 100644
index 0000000000000..e5ac27e79deb1
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.h
@@ -0,0 +1,37 @@
+//===--- NullCheckAfterDereferenceCheck.h - clang-tidy ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NULLCHECKAFTERDEREFERENCECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NULLCHECKAFTERDEREFERENCECHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds checks for pointer nullability after a pointer has already been
+/// dereferenced, using the data-flow framework.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/null-check-after-dereference.html
+class NullCheckAfterDereferenceCheck : public ClangTidyCheck {
+public:
+ NullCheckAfterDereferenceCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+
+ // The data-flow framework does not support C because of AST differences.
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NULLCHECKAFTERDEREFERENCECHECK_H
diff --git a/clang-tools-extra/clangd/TidyProvider.cpp b/clang-tools-extra/clangd/TidyProvider.cpp
index b658a80559937..cf7b4dff16070 100644
--- a/clang-tools-extra/clangd/TidyProvider.cpp
+++ b/clang-tools-extra/clangd/TidyProvider.cpp
@@ -219,9 +219,10 @@ TidyProvider disableUnusableChecks(llvm::ArrayRef<std::string> ExtraBadChecks) {
"-bugprone-use-after-move",
// Alias for bugprone-use-after-move.
"-hicpp-invalid-access-moved",
- // Check uses dataflow analysis, which might hang/crash unexpectedly on
+ // Checks use dataflow analysis, which might hang/crash unexpectedly on
// incomplete code.
"-bugprone-unchecked-optional-access",
+ "-bugprone-null-check-after-dereference",
// ----- Performance problems -----
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
new file mode 100644
index 0000000000000..b4910867c2017
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
@@ -0,0 +1,162 @@
+.. title:: clang-tidy - bugprone-null-check-after-dereference
+
+bugprone-null-check-after-dereference
+=====================================
+
+.. note::
+
+ This check uses a flow-sensitive static analysis to produce its
+ results. Therefore, it may be more resource intensive (RAM, CPU) than the
+ average clang-tidy check.
+
+This check identifies redundant pointer null-checks, by finding cases where the
+pointer cannot be null at the location of the null-check.
+
+Redundant null-checks can signal faulty assumptions about the current value of
+a pointer at different points in the program. Either the null-check is
+redundant, or there could be a null-pointer dereference earlier in the program.
+
+.. code-block:: c++
+
+ int f(int *ptr) {
+ *ptr = 20; // note: one of the locations where the pointer's value cannot be null
+ // ...
+ if (ptr) { // bugprone: pointer is checked even though it cannot be null at this point
+ return *ptr;
+ }
+ return 0;
+ }
+
+Supported pointer operations
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Pointer null-checks
+-------------------
+
+The checker currently supports null-checks on pointers that use
+``operator bool``, such as when being used as the condition
+for an `if` statement.
+
+.. code-block:: c++
+
+ int f(int *ptr) {
+ if (ptr) {
+ if (ptr) { // bugprone: pointer is re-checked after its null-ness is already checked.
+ return *ptr;
+ }
+
+ return ptr ? *ptr : 0; // bugprone: pointer is re-checked after its null-ness is already checked.
+ }
+ return 0;
+ }
+
+Pointer dereferences
+--------------------
+
+Pointer star- and arrow-dereferences are supported.
+
+.. code-block:: c++
+
+ struct S {
+ int val;
+ };
+
+ void f(int *ptr, S *wrapper) {
+ *ptr = 20;
+ wrapper->val = 15;
+ }
+
+Null-pointer and other value assignments
+----------------------------------------
+
+The checker supports assigning various values to pointers, making them *null*
+or *non-null*. The checker also supports passing pointers of a pointer to
+external functions.
+
+.. code-block:: c++
+
+ extern int *external();
+ extern void refresh(int **ptr_ptr);
+
+ int f() {
+ int *ptr_null = nullptr;
+ if (ptr_null) { // bugprone: pointer is checked where it cannot be non-null.
+ return *ptr_null;
+ }
+
+ int *ptr = external();
+ if (ptr) { // safe: external() could return either nullable or nonnull pointers.
+ return *ptr;
+ }
+
+ int *ptr2 = external();
+ *ptr2 = 20;
+ refresh(&ptr2);
+ if (ptr2) { // safe: pointer could be changed by refresh().
+ return *ptr2;
+ }
+ return 0;
+ }
+
+Limitations
+~~~~~~~~~~~
+
+The check only supports C++ due to limitations in the data-flow framework.
+
+The annotations ``_nullable`` and ``_nonnull`` are not supported.
+
+.. code-block:: c++
+
+ extern int *_nonnull external_nonnull();
+
+ int annotations() {
+ int *ptr = external_nonnull();
+
+ return ptr ? *ptr : 0; // false-negative: pointer is known to be non-null.
+ }
+
+Function calls taking a pointer value as a reference or a pointer-to-pointer are
+not supported.
+
+.. code-block:: c++
+
+ extern int *external();
+ extern void refresh_ref(int *&ptr);
+ extern void refresh_ptr(int **ptr);
+
+ int extern_ref() {
+ int *ptr = external();
+ *ptr = 20;
+
+ refresh_ref(ptr);
+ refresh_ptr(&ptr);
+
+ return ptr ? *ptr : 0; // false-positive: pointer could be changed by refresh_ref().
+ }
+
+Note tags are currently appended to a single location, even if all paths ensure
+a pointer is not null.
+
+.. code-block:: c++
+
+ int branches(int *p, bool b) {
+ if (b) {
+ *p = 42; // true-positive: note-tag appended here
+ } else {
+ *p = 20; // false-positive: note tag not appended here
+ }
+
+ return ptr ? *ptr : 0;
+ }
+
+Declarations and some other operations are not supported by note tags yet. This
+can sometimes result in erroneous note tags being shown instead of the correct
+one.
+
+.. code-block:: c++
+
+ int note_tags() {
+ int *ptr = nullptr; // false-negative: note tag not shown
+
+ return ptr ? *ptr : 0;
+ }
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
new file mode 100644
index 0000000000000..21e9eff4290f7
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
@@ -0,0 +1,330 @@
+// RUN: %check_clang_tidy %s bugprone-null-check-after-dereference %t
+
+struct S {
+ int a;
+};
+
+int warning_deref(int *p) {
+ *p = 42;
+
+ if (p) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point [bugprone-null-check-after-dereference]
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+ // FIXME: If there's a direct path, make the error message more precise, ie. remove `one of the locations`
+ *p += 20;
+ return *p;
+ } else {
+ return 0;
+ }
+}
+
+int warning_member(S *q) {
+ q->a = 42;
+
+ if (q) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+ q->a += 20;
+ return q->a;
+ } else {
+ return 0;
+ }
+}
+
+int negative_warning(int *p) {
+ *p = 42;
+
+ if (!p) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+ return 0;
+ } else {
+ *p += 20;
+ return *p;
+ }
+}
+
+int no_warning(int *p, bool b) {
+ if (b) {
+ *p = 42;
+ }
+
+ if (p) {
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ *p += 20;
+ return *p;
+ } else {
+ return 0;
+ }
+}
+
+int else_branch_warning(int *p, bool b) {
+ if (b) {
+ *p = 42;
+ } else {
+ *p = 20;
+ }
+
+ if (p) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-7]]:5: note: one of the locations where the pointer's value cannot be null
+ return 0;
+ } else {
+ *p += 20;
+ return *p;
+ }
+}
+
+int two_branches_warning(int *p, bool b) {
+ if (b) {
+ *p = 42;
+ }
+
+ if (!b) {
+ *p = 20;
+ }
+
+ if (p) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-9]]:5: note: one of the locations where the pointer's value cannot be null
+ return 0;
+ } else {
+ *p += 20;
+ return *p;
+ }
+}
+
+int two_branches_reversed(int *p, bool b) {
+ if (!b) {
+ *p = 42;
+ }
+
+ if (b) {
+ *p = 20;
+ }
+
+ if (p) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-9]]:5: note: one of the locations where the pointer's value cannot be null
+ return 0;
+ } else {
+ *p += 20;
+ return *p;
+ }
+}
+
+
+int regular_assignment(int *p, int *q) {
+ *p = 42;
+ q = p;
+
+ if (q) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-5]]:3: note: one of the locations where the pointer's value cannot be null
+ *p += 20;
+ return *p;
+ } else {
+ return 0;
+ }
+}
+
+int nullptr_assignment(int *nullptr_param, bool b) {
+ *nullptr_param = 42;
+ int *nullptr_assigned;
+
+ if (b) {
+ nullptr_assigned = nullptr;
+ } else {
+ nullptr_assigned = nullptr_param;
+ }
+
+ if (nullptr_assigned) {
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ *nullptr_assigned = 20;
+ return *nullptr_assigned;
+ } else {
+ return 0;
+ }
+}
+
+extern int *fncall();
+extern void refresh_ref(int *&ptr);
+extern void refresh_ptr(int **ptr);
+
+int fncall_reassignment(int *fncall_reassigned) {
+ *fncall_reassigned = 42;
+
+ fncall_reassigned = fncall();
+
+ if (fncall_reassigned) {
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ *fncall_reassigned = 42;
+ }
+
+ fncall_reassigned = fncall();
+
+ *fncall_reassigned = 42;
+
+ if (fncall_reassigned) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+ *fncall_reassigned = 42;
+ }
+
+ refresh_ptr(&fncall_reassigned);
+
+ if (fncall_reassigned) {
+ // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ *fncall_reassigned = 42;
+ }
+
+ refresh_ptr(&fncall_reassigned);
+ *fncall_reassigned = 42;
+
+ if (fncall_reassigned) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+ *fncall_reassigned = 42;
+ return *fncall_reassigned;
+ } else {
+ return 0;
+ }
+}
+
+int chained_references(int *a, int *b, int *c, int *d, int *e) {
+ *a = 42;
+
+ if (a) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+ *b = 42;
+ }
+
+ if (b) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-5]]:5: note: one of the locations where the pointer's value cannot be null
+ *c = 42;
+ }
+
+ if (c) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-5]]:5: note: one of the locations where the pointer's value cannot be null
+ *d = 42;
+ }
+
+ if (d) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-5]]:5: note: one of the locations where the pointer's value cannot be null
+ *e = 42;
+ }
+
+ if (e) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-5]]:5: note: one of the locations where the pointer's value cannot be null
+ return *a;
+ } else {
+ return 0;
+ }
+}
+
+int chained_if(int *a) {
+ if (!a) {
+ return 0;
+ }
+
+ // FIXME: Negations are not tracked properly when the previous conditional returns
+ if (a) {
+ // --CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ *a += 20;
+ return *a;
+ } else {
+ return 0;
+ }
+}
+
+int double_if(int *a) {
+ if (a) {
+ if (a) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: pointer value is checked even though it cannot be null at this point
+ // --CHECK-MESSAGES: :[[@LINE-3]]:5: note: one of the locations where the pointer's value cannot be null
+ // FIXME: Add warning for branch statements where pointer is not null afterwards
+ *a += 20;
+ return *a;
+ } else {
+ return 0;
+ }
+ }
+
+ return 0;
+}
+
+int while_loop(int *p, volatile bool *b) {
+ while (true) {
+ if (*b) {
+ *p = 42;
+ break;
+ }
+ }
+
+ if (p) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-7]]:7: note: one of the locations where the pointer's value cannot be null
+ *p = 42;
+ return *p;
+ } else {
+ return 0;
+ }
+}
+
+int ternary_op(int *p, int k) {
+ *p = 42;
+
+ return p ? *p : k;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+}
+
+// In an earlier version, the check would crash on C++17 structured bindings.
+int cxx17_crash(int *p) {
+ *p = 42;
+
+ int arr[2] = {1, 2};
+ auto [a, b] = arr;
+
+ return 0;
+}
+
+void external_by_ref(int *&p);
+void external_by_ptr(int **p);
+
+int external_invalidates() {
+ int *p = nullptr;
+
+ external_by_ref(p);
+
+ if (p) {
+ // FIXME: References of a pointer passed to external functions do not invalidate its value
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked but it can only be null at this point
+ return *p;
+ }
+
+ p = nullptr;
+
+ external_by_ptr(&p);
+
+ if (p) {
+ // FIXME: References of a pointer passed to external functions do not invalidate its value
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked but it can only be null at this point
+ return *p;
+ } else {
+ return 0;
+ }
+}
+
+int note_tags() {
+ // FIXME: Note tags are not appended to declarations
+ int *ptr = nullptr;
+
+ return ptr ? *ptr : 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: pointer value is checked but it can only be null at this point
+}
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
new file mode 100644
index 0000000000000..16b28ecc9ecc9
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
@@ -0,0 +1,112 @@
+//===-- NullPointerAnalysisModel.h ------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines a generic null-pointer analysis model, used for finding
+// pointer null-checks after the pointer has already been dereferenced.
+//
+// Only a limited set of operations are currently recognized. Notably, pointer
+// arithmetic, null-pointer assignments and _nullable/_nonnull attributes are
+// missing as of yet.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_NULLPOINTERANALYSISMODEL_H
+#define CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_NULLPOINTERANALYSISMODEL_H
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/MapLattice.h"
+#include "clang/Analysis/FlowSensitive/NoopLattice.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
+
+namespace clang::dataflow {
+
+class NullPointerAnalysisModel
+ : public DataflowAnalysis<NullPointerAnalysisModel, NoopLattice> {
+public:
+ /// A transparent wrapper around the function arguments of transferBranch().
+ /// Does not outlive the call to transferBranch().
+ struct TransferArgs {
+ bool Branch;
+ Environment &Env;
+ };
+
+private:
+ CFGMatchSwitch<Environment> TransferMatchSwitch;
+ ASTMatchSwitch<Stmt, TransferArgs> BranchTransferMatchSwitch;
+
+public:
+ explicit NullPointerAnalysisModel(ASTContext &Context);
+
+ static NoopLattice initialElement() { return {}; }
+
+ static ast_matchers::StatementMatcher ptrValueMatcher();
+
+ // Used to initialize the storage locations of function arguments.
+ // Required to merge these values within the environment.
+ void initializeFunctionParameters(const ControlFlowContext &CFCtx,
+ Environment &Env);
+
+ void transfer(const CFGElement &E, NoopLattice &State, Environment &Env);
+
+ void transferBranch(bool Branch, const Stmt *E, NoopLattice &State,
+ Environment &Env);
+
+ void join(QualType Type, const Value &Val1, const Environment &Env1,
+ const Value &Val2, const Environment &Env2, Value &MergedVal,
+ Environment &MergedEnv) override;
+
+ ComparisonResult compare(QualType Type, const Value &Val1,
+ const Environment &Env1, const Value &Val2,
+ const Environment &Env2) override;
+
+ Value *widen(QualType Type, Value &Prev, const Environment &PrevEnv,
+ Value &Current, Environment &CurrentEnv) override;
+};
+
+class NullCheckAfterDereferenceDiagnoser {
+public:
+ struct DiagnoseArgs {
+ llvm::DenseMap<const Value *, const Expr *> &ValToDerefLoc;
+ llvm::DenseMap<SourceLocation, const Value *> &WarningLocToVal;
+ const Environment &Env;
+ };
+
+ using ResultType =
+ std::pair<std::vector<SourceLocation>, std::vector<SourceLocation>>;
+
+ // Maps a pointer's Value to a dereference, null-assignment, etc.
+ // This is used later to construct the Note tag.
+ llvm::DenseMap<const Value *, const Expr *> ValToDerefLoc;
+ // Maps Maps a warning's SourceLocation to its relevant Value.
+ llvm::DenseMap<SourceLocation, const Value *> WarningLocToVal;
+
+private:
+ CFGMatchSwitch<DiagnoseArgs, ResultType> DiagnoseMatchSwitch;
+
+public:
+ NullCheckAfterDereferenceDiagnoser();
+
+ ResultType diagnose(ASTContext &Ctx, const CFGElement *Elt,
+ const Environment &Env);
+};
+
+} // namespace clang::dataflow
+
+#endif // CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_NULLPOINTERANALYSISMODEL_H
diff --git a/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt
index 89bbe8791eb2c..6d365dabe6ae5 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt
+++ b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt
@@ -1,5 +1,6 @@
add_clang_library(clangAnalysisFlowSensitiveModels
ChromiumCheckModel.cpp
+ NullPointerAnalysisModel.cpp
UncheckedOptionalAccessModel.cpp
LINK_LIBS
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
new file mode 100644
index 0000000000000..49750406a3f43
--- /dev/null
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -0,0 +1,625 @@
+//===-- NullPointerAnalysisModel.cpp ----------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines a generic null-pointer analysis model, used for finding
+// pointer null-checks after the pointer has already been dereferenced.
+//
+// Only a limited set of operations are currently recognized. Notably, pointer
+// arithmetic, null-pointer assignments and _nullable/_nonnull attributes are
+// missing as of yet.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/MapLattice.h"
+#include "clang/Analysis/FlowSensitive/NoopLattice.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
+
+namespace clang::dataflow {
+
+namespace {
+using namespace ast_matchers;
+
+constexpr char kCond[] = "condition";
+constexpr char kVar[] = "var";
+constexpr char kValue[] = "value";
+constexpr char kIsNonnull[] = "is-nonnull";
+constexpr char kIsNull[] = "is-null";
+
+enum class SatisfiabilityResult {
+ // Returned when the value was not initialized yet.
+ Nullptr,
+ // Special value that signals that the boolean value can be anything.
+ // It signals that the underlying formulas are too complex to be calculated
+ // efficiently.
+ Top,
+ // Equivalent to the literal True in the current environment.
+ True,
+ // Equivalent to the literal False in the current environment.
+ False,
+ // Both True and False values could be produced with an appropriate set of
+ // conditions.
+ Unknown
+};
+
+using SR = SatisfiabilityResult;
+
+// FIXME: These AST matchers should also be exported via the
+// NullPointerAnalysisModel class, for tests
+auto ptrToVar(llvm::StringRef VarName = kVar) {
+ return traverse(TK_IgnoreUnlessSpelledInSource,
+ declRefExpr(hasType(isAnyPointer())).bind(VarName));
+}
+
+auto derefMatcher() {
+ return traverse(
+ TK_IgnoreUnlessSpelledInSource,
+ unaryOperator(hasOperatorName("*"), hasUnaryOperand(ptrToVar())));
+}
+
+auto arrowMatcher() {
+ return traverse(
+ TK_IgnoreUnlessSpelledInSource,
+ memberExpr(allOf(isArrow(), hasObjectExpression(ptrToVar()))));
+}
+
+auto castExprMatcher() {
+ return castExpr(hasCastKind(CK_PointerToBoolean),
+ hasSourceExpression(ptrToVar()))
+ .bind(kCond);
+}
+
+auto nullptrMatcher() {
+ return castExpr(hasCastKind(CK_NullToPointer)).bind(kVar);
+}
+
+auto addressofMatcher() {
+ return unaryOperator(hasOperatorName("&")).bind(kVar);
+}
+
+auto functionCallMatcher() {
+ return callExpr(hasDeclaration(functionDecl(returns(isAnyPointer()))))
+ .bind(kVar);
+}
+
+auto assignMatcher() {
+ return binaryOperation(isAssignmentOperator(), hasLHS(ptrToVar()),
+ hasRHS(expr().bind(kValue)));
+}
+
+auto anyPointerMatcher() { return expr(hasType(isAnyPointer())).bind(kVar); }
+
+// Only computes simple comparisons against the literals True and False in the
+// environment. Faster, but produces many Unknown values.
+SatisfiabilityResult shallowComputeSatisfiability(BoolValue *Val,
+ const Environment &Env) {
+ if (!Val)
+ return SR::Nullptr;
+
+ if (isa<TopBoolValue>(Val))
+ return SR::Top;
+
+ if (Val == &Env.getBoolLiteralValue(true))
+ return SR::True;
+
+ if (Val == &Env.getBoolLiteralValue(false))
+ return SR::False;
+
+ return SR::Unknown;
+}
+
+// Computes satisfiability by using the flow condition. Slower, but more
+// precise.
+SatisfiabilityResult computeSatisfiability(BoolValue *Val,
+ const Environment &Env) {
+ // Early return with the fast compute values.
+ if (SatisfiabilityResult ShallowResult =
+ shallowComputeSatisfiability(Val, Env);
+ ShallowResult != SR::Unknown) {
+ return ShallowResult;
+ }
+
+ if (Env.proves(Val->formula()))
+ return SR::True;
+
+ if (Env.proves(Env.arena().makeNot(Val->formula())))
+ return SR::False;
+
+ return SR::Unknown;
+}
+
+inline BoolValue &getVal(llvm::StringRef Name, Value &RootValue) {
+ return *cast<BoolValue>(RootValue.getProperty(Name));
+}
+
+// Assigns initial pointer null- and nonnull-values to a given Value.
+void initializeRootValue(Value &RootValue, Environment &Env) {
+ Arena &A = Env.arena();
+
+ BoolValue *IsNull = cast_or_null<BoolValue>(RootValue.getProperty(kIsNull));
+ BoolValue *IsNonnull =
+ cast_or_null<BoolValue>(RootValue.getProperty(kIsNonnull));
+
+ if (!IsNull) {
+ IsNull = &A.makeAtomValue();
+ RootValue.setProperty(kIsNull, *IsNull);
+ }
+
+ if (!IsNonnull) {
+ IsNonnull = &A.makeAtomValue();
+ RootValue.setProperty(kIsNonnull, *IsNonnull);
+ }
+
+ // If the pointer cannot have either a null or nonull value, the state is
+ // unreachable.
+ // FIXME: This condition is added in all cases when getValue() is called.
+ // The reason is that on a post-visit step, the initialized Values are used,
+ // but the flow condition does not have this constraint yet.
+ // The framework provides deduplication for constraints, so should not have a
+ // performance impact.
+ Env.assume(A.makeOr(IsNull->formula(), IsNonnull->formula()));
+}
+
+void setGLValue(const Expr &E, Value &Val, Environment &Env) {
+ StorageLocation *SL = Env.getStorageLocation(E);
+ if (!SL) {
+ SL = &Env.createStorageLocation(E);
+ Env.setStorageLocation(E, *SL);
+ }
+
+ Env.setValue(*SL, Val);
+}
+
+void setUnknownValue(const Expr &E, Value &Val, Environment &Env) {
+ if (E.isGLValue())
+ setGLValue(E, Val, Env);
+ else if (E.isPRValue())
+ Env.setValue(E, Val);
+ else
+ llvm_unreachable("all value cases covered");
+}
+
+Value *getValue(const Expr &Var, Environment &Env) {
+ if (auto *EnvVal = Env.getValue(Var)) {
+ // FIXME: The framework usually creates the values for us, but without the
+ // null-properties.
+ initializeRootValue(*EnvVal, Env);
+
+ return EnvVal;
+ }
+
+ auto *RootValue = Env.createValue(Var.getType());
+
+ if (!RootValue)
+ return nullptr;
+
+ initializeRootValue(*RootValue, Env);
+
+ setGLValue(Var, *RootValue, Env);
+
+ return RootValue;
+}
+
+void matchDereferenceExpr(const Stmt *stmt,
+ const MatchFinder::MatchResult &Result,
+ Environment &Env) {
+ const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(Var != nullptr);
+
+ auto *RootValue = getValue(*Var, Env);
+ if (!RootValue) {
+ return;
+ }
+
+ Env.assume(Env.arena().makeNot(getVal(kIsNull, *RootValue).formula()));
+}
+
+void matchCastExpr(const CastExpr *cond, const MatchFinder::MatchResult &Result,
+ NullPointerAnalysisModel::TransferArgs &Data) {
+ auto [IsNonnullBranch, Env] = Data;
+
+ const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(Var != nullptr);
+
+ auto *RootValue = getValue(*Var, Env);
+ if (!RootValue) {
+ return;
+ }
+
+ auto *NewRootValue = Env.createValue(Var->getType());
+ if (!NewRootValue)
+ return;
+
+ setGLValue(*Var, *NewRootValue, Env);
+
+ Arena &A = Env.arena();
+ BoolValue &IsNonnull = getVal(kIsNonnull, *RootValue);
+ BoolValue &IsNull = getVal(kIsNull, *RootValue);
+
+ if (IsNonnullBranch) {
+ Env.assume(A.makeNot(IsNull.formula()));
+ NewRootValue->setProperty(kIsNull, Env.getBoolLiteralValue(false));
+
+ NewRootValue->setProperty(kIsNonnull, IsNonnull);
+ } else {
+ NewRootValue->setProperty(kIsNull, IsNull);
+
+ Env.assume(A.makeNot(IsNonnull.formula()));
+ NewRootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(false));
+ }
+}
+
+void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result,
+ Environment &Env) {
+ const auto *PrVar = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(PrVar != nullptr);
+
+ auto *RootValue = Env.getValue(*PrVar);
+ if (!RootValue) {
+ RootValue = Env.createValue(PrVar->getType());
+
+ if (!RootValue) {
+ return;
+ }
+ }
+
+ RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(true));
+ RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(false));
+ Env.setValue(*PrVar, *RootValue);
+}
+
+void matchAddressofExpr(const Expr *expr,
+ const MatchFinder::MatchResult &Result,
+ Environment &Env) {
+ const auto *PrVar = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(PrVar != nullptr);
+
+ auto *RootValue = Env.createValue(PrVar->getType());
+ if (!RootValue) {
+ return;
+ }
+
+ RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(false));
+ RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(true));
+ Env.setValue(*PrVar, *RootValue);
+}
+
+void matchAnyPointerExpr(const Expr *fncall,
+ const MatchFinder::MatchResult &Result,
+ Environment &Env) {
+ // This is not necessarily a prvalue, since operators such as prefix ++ are
+ // lvalues.
+ const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(Var != nullptr);
+
+ // Initialize to (Unknown, Unknown)
+ if (Env.getValue(*Var))
+ return;
+
+ auto *RootValue = Env.createValue(Var->getType());
+ if (!RootValue)
+ return;
+
+ initializeRootValue(*RootValue, Env);
+ setUnknownValue(*Var, *RootValue, Env);
+}
+
+NullCheckAfterDereferenceDiagnoser::ResultType
+diagnoseDerefLocation(const Expr *Deref, const MatchFinder::MatchResult &Result,
+ NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) {
+ auto [ValToDerefLoc, WarningLocToVal, Env] = Data;
+
+ const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(Var != nullptr);
+
+ auto *RootValue = Env.getValue(*Var);
+ if (!RootValue)
+ return {};
+
+ // Dereferences are always the highest priority when giving a single location
+ // FIXME: Do not replace other dereferences, only other Expr's
+ auto It = ValToDerefLoc.try_emplace(RootValue, nullptr).first;
+
+ It->second = Deref;
+
+ return {};
+}
+
+NullCheckAfterDereferenceDiagnoser::ResultType
+diagnoseAssignLocation(const Expr *Assign,
+ const MatchFinder::MatchResult &Result,
+ NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) {
+ auto [ValToDerefLoc, WarningLocToVal, Env] = Data;
+
+ const auto *RHSVar = Result.Nodes.getNodeAs<Expr>(kValue);
+ assert(RHSVar != nullptr);
+
+ auto *RHSValue = Env.getValue(*RHSVar);
+ if (!RHSValue)
+ return {};
+
+ auto [It, Inserted] = ValToDerefLoc.try_emplace(RHSValue, nullptr);
+
+ if (Inserted)
+ It->second = Assign;
+
+ return {};
+}
+
+NullCheckAfterDereferenceDiagnoser::ResultType
+diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
+ const NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) {
+
+ auto [ValToDerefLoc, WarningLocToVal, Env] = Data;
+
+ const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(Var != nullptr);
+
+ if (auto *RootValue = Env.getValue(*Var)) {
+ // FIXME: The framework usually creates the values for us, but without the
+ // nullability properties.
+ if (RootValue->getProperty(kIsNull) && RootValue->getProperty(kIsNonnull)) {
+ bool IsNull = Env.allows(getVal(kIsNull, *RootValue).formula());
+ bool IsNonnull = Env.allows(getVal(kIsNonnull, *RootValue).formula());
+
+ if (!IsNull && IsNonnull) {
+ bool Inserted =
+ WarningLocToVal.try_emplace(Var->getBeginLoc(), RootValue).second;
+ assert(Inserted && "multiple warnings at the same source location");
+
+ return {{}, {Var->getBeginLoc()}};
+ }
+
+ if (IsNull && !IsNonnull) {
+ bool Inserted =
+ WarningLocToVal.try_emplace(Var->getBeginLoc(), RootValue).second;
+ assert(Inserted && "multiple warnings at the same source location");
+
+ return {{Var->getBeginLoc()}, {}};
+ }
+ }
+
+ // If no matches are found, the cast itself signals a special location
+ auto [It, Inserted] = ValToDerefLoc.try_emplace(RootValue, nullptr);
+
+ if (Inserted)
+ It->second = Stmt;
+ }
+
+ return {};
+}
+
+auto buildTransferMatchSwitch() {
+ return CFGMatchSwitchBuilder<Environment>()
+ .CaseOfCFGStmt<Stmt>(derefMatcher(), matchDereferenceExpr)
+ .CaseOfCFGStmt<Stmt>(arrowMatcher(), matchDereferenceExpr)
+ .CaseOfCFGStmt<Expr>(nullptrMatcher(), matchNullptrExpr)
+ .CaseOfCFGStmt<Expr>(addressofMatcher(), matchAddressofExpr)
+ .CaseOfCFGStmt<Expr>(functionCallMatcher(), matchAnyPointerExpr)
+ .CaseOfCFGStmt<Expr>(anyPointerMatcher(), matchAnyPointerExpr)
+ .Build();
+}
+
+auto buildBranchTransferMatchSwitch() {
+ return ASTMatchSwitchBuilder<Stmt, NullPointerAnalysisModel::TransferArgs>()
+ .CaseOf<CastExpr>(castExprMatcher(), matchCastExpr)
+ .Build();
+}
+
+auto buildDiagnoseMatchSwitch() {
+ return CFGMatchSwitchBuilder<NullCheckAfterDereferenceDiagnoser::DiagnoseArgs,
+ NullCheckAfterDereferenceDiagnoser::ResultType>()
+ .CaseOfCFGStmt<Expr>(derefMatcher(), diagnoseDerefLocation)
+ .CaseOfCFGStmt<Expr>(arrowMatcher(), diagnoseDerefLocation)
+ .CaseOfCFGStmt<Expr>(assignMatcher(), diagnoseAssignLocation)
+ .CaseOfCFGStmt<CastExpr>(castExprMatcher(), diagnoseCastExpr)
+ .Build();
+}
+
+} // namespace
+
+NullPointerAnalysisModel::NullPointerAnalysisModel(ASTContext &Context)
+ : DataflowAnalysis<NullPointerAnalysisModel, NoopLattice>(Context),
+ TransferMatchSwitch(buildTransferMatchSwitch()),
+ BranchTransferMatchSwitch(buildBranchTransferMatchSwitch()) {}
+
+ast_matchers::StatementMatcher NullPointerAnalysisModel::ptrValueMatcher() {
+ return ptrToVar();
+}
+
+void NullPointerAnalysisModel::transfer(const CFGElement &E, NoopLattice &State,
+ Environment &Env) {
+ TransferMatchSwitch(E, getASTContext(), Env);
+}
+
+void NullPointerAnalysisModel::transferBranch(bool Branch, const Stmt *E,
+ NoopLattice &State,
+ Environment &Env) {
+ if (!E)
+ return;
+
+ TransferArgs Args = {Branch, Env};
+ BranchTransferMatchSwitch(*E, getASTContext(), Args);
+}
+
+void NullPointerAnalysisModel::join(QualType Type, const Value &Val1,
+ const Environment &Env1, const Value &Val2,
+ const Environment &Env2, Value &MergedVal,
+ Environment &MergedEnv) {
+ if (!Type->isAnyPointerType())
+ return;
+
+ const auto MergeValues = [&](llvm::StringRef Name) -> BoolValue & {
+ BoolValue *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
+ BoolValue *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
+
+ SatisfiabilityResult LHSResult = computeSatisfiability(LHSVar, Env1);
+ SatisfiabilityResult RHSResult = computeSatisfiability(RHSVar, Env2);
+
+ // Handle special cases.
+ if (LHSResult == SR::Top || RHSResult == SR::Top) {
+ return MergedEnv.makeTopBoolValue();
+ } else if (LHSResult == RHSResult) {
+ switch (LHSResult) {
+ case SR::Nullptr:
+ return MergedEnv.makeAtomicBoolValue();
+ case SR::Top:
+ return *LHSVar;
+ case SR::True:
+ return MergedEnv.getBoolLiteralValue(true);
+ case SR::False:
+ return MergedEnv.getBoolLiteralValue(false);
+ case SR::Unknown:
+ if (MergedEnv.proves(MergedEnv.arena().makeEquals(LHSVar->formula(),
+ RHSVar->formula())))
+ return *LHSVar;
+
+ return MergedEnv.makeTopBoolValue();
+ }
+ }
+
+ return MergedEnv.makeTopBoolValue();
+ };
+
+ BoolValue &NonnullValue = MergeValues(kIsNonnull);
+ BoolValue &NullValue = MergeValues(kIsNull);
+
+ MergedVal.setProperty(kIsNonnull, NonnullValue);
+ MergedVal.setProperty(kIsNull, NullValue);
+
+ MergedEnv.assume(MergedEnv.makeOr(NonnullValue, NullValue).formula());
+}
+
+ComparisonResult NullPointerAnalysisModel::compare(QualType Type,
+ const Value &Val1,
+ const Environment &Env1,
+ const Value &Val2,
+ const Environment &Env2) {
+
+ if (!Type->isAnyPointerType())
+ return ComparisonResult::Unknown;
+
+ // Evaluate values, but different values compare to Unknown.
+ auto CompareValues = [&](llvm::StringRef Name) -> ComparisonResult {
+ BoolValue *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
+ BoolValue *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
+
+ SatisfiabilityResult LHSResult = computeSatisfiability(LHSVar, Env1);
+ SatisfiabilityResult RHSResult = computeSatisfiability(RHSVar, Env2);
+
+ if (LHSResult == SR::Top || RHSResult == SR::Top)
+ return ComparisonResult::Same;
+
+ if (LHSResult == SR::Unknown || RHSResult == SR::Unknown)
+ return ComparisonResult::Unknown;
+
+ if (LHSResult == RHSResult)
+ return ComparisonResult::Same;
+
+ return ComparisonResult::Different;
+ };
+
+ ComparisonResult NullComparison = CompareValues(kIsNull);
+ ComparisonResult NonnullComparison = CompareValues(kIsNonnull);
+
+ if (NullComparison == ComparisonResult::Different ||
+ NonnullComparison == ComparisonResult::Different)
+ return ComparisonResult::Different;
+
+ if (NullComparison == ComparisonResult::Unknown ||
+ NonnullComparison == ComparisonResult::Unknown)
+ return ComparisonResult::Unknown;
+
+ return ComparisonResult::Same;
+}
+
+// Different in that it replaces differing boolean values with Top.
+ComparisonResult compareAndReplace(QualType Type, Value &Val1,
+ const Environment &Env1, Value &Val2,
+ Environment &Env2) {
+
+ if (!Type->isAnyPointerType())
+ return ComparisonResult::Unknown;
+
+ auto FastCompareValues = [&](llvm::StringRef Name) -> ComparisonResult {
+ BoolValue *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
+ BoolValue *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
+
+ SatisfiabilityResult LHSResult = shallowComputeSatisfiability(LHSVar, Env1);
+ SatisfiabilityResult RHSResult = shallowComputeSatisfiability(RHSVar, Env2);
+
+ if (LHSResult == SR::Top || RHSResult == SR::Top) {
+ Val2.setProperty(Name, Env2.makeTopBoolValue());
+ return ComparisonResult::Same;
+ }
+
+ if (LHSResult == SR::Unknown || RHSResult == SR::Unknown)
+ return ComparisonResult::Unknown;
+
+ if (LHSResult == RHSResult)
+ return ComparisonResult::Same;
+
+ Val2.setProperty(Name, Env2.makeTopBoolValue());
+ return ComparisonResult::Different;
+ };
+
+ ComparisonResult NullComparison = FastCompareValues(kIsNull);
+ ComparisonResult NonnullComparison = FastCompareValues(kIsNonnull);
+
+ if (NullComparison == ComparisonResult::Different ||
+ NonnullComparison == ComparisonResult::Different)
+ return ComparisonResult::Different;
+
+ if (NullComparison == ComparisonResult::Unknown ||
+ NonnullComparison == ComparisonResult::Unknown)
+ return ComparisonResult::Unknown;
+
+ return ComparisonResult::Same;
+}
+
+Value *NullPointerAnalysisModel::widen(QualType Type, Value &Prev,
+ const Environment &PrevEnv,
+ Value &Current,
+ Environment &CurrentEnv) {
+ if (!Type->isAnyPointerType())
+ return nullptr;
+
+ switch (compareAndReplace(Type, Prev, PrevEnv, Current, CurrentEnv)) {
+ case ComparisonResult::Same:
+ return &Prev;
+ case ComparisonResult::Unknown:
+ return nullptr;
+ case ComparisonResult::Different:
+ return &Current;
+ }
+}
+
+NullCheckAfterDereferenceDiagnoser::NullCheckAfterDereferenceDiagnoser()
+ : DiagnoseMatchSwitch(buildDiagnoseMatchSwitch()) {}
+
+NullCheckAfterDereferenceDiagnoser::ResultType
+NullCheckAfterDereferenceDiagnoser::diagnose(ASTContext &Ctx,
+ const CFGElement *Elt,
+ const Environment &Env) {
+ DiagnoseArgs Args = {ValToDerefLoc, WarningLocToVal, Env};
+ return DiagnoseMatchSwitch(*Elt, Ctx, Args);
+}
+
+} // namespace clang::dataflow
diff --git a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
index 94160d949637c..2a301a9c48fd2 100644
--- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
@@ -15,6 +15,7 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests
MapLatticeTest.cpp
MatchSwitchTest.cpp
MultiVarConstantPropagationTest.cpp
+ NullPointerAnalysisModelTest.cpp
RecordOpsTest.cpp
SignAnalysisTest.cpp
SimplifyConstraintsTest.cpp
diff --git a/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp
new file mode 100644
index 0000000000000..d63430de2a15e
--- /dev/null
+++ b/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp
@@ -0,0 +1,332 @@
+//===- NullPointerAnalysisModelTest.cpp -------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines a test for pointer nullability, specifically focused on
+// finding invalid dereferences, and unnecessary null-checks.
+// Only a limited set of operations are currently recognized. Notably, pointer
+// arithmetic, null-pointer assignments and _nullable/_nonnull attributes are
+// missing as of yet.
+//
+// FIXME: Port over to the new type of dataflow test infrastructure
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h"
+#include "TestingSupport.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/MapLattice.h"
+#include "clang/Analysis/FlowSensitive/NoopLattice.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/ADT/StringMapEntry.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <cstdint>
+#include <memory>
+#include <ostream>
+#include <string>
+#include <utility>
+
+namespace clang::dataflow {
+namespace {
+using namespace ast_matchers;
+
+constexpr char kVar[] = "var";
+// constexpr char kKnown[] = "is-known";
+constexpr char kIsNonnull[] = "is-nonnull";
+constexpr char kIsNull[] = "is-null";
+
+constexpr char kBoolTrue[] = "true";
+constexpr char kBoolFalse[] = "false";
+constexpr char kBoolInvalid[] = "invalid";
+constexpr char kBoolUnknown[] = "unknown";
+constexpr char kBoolNullptr[] = "is-nullptr";
+
+std::string checkNullabilityState(BoolValue *value, const Environment &Env) {
+ if (value == nullptr) {
+ return std::string(kBoolNullptr);
+ } else {
+ int boolState = 0;
+ if (Env.proves(value->formula())) {
+ boolState |= 1;
+ }
+ if (Env.proves(Env.makeNot(*value).formula())) {
+ boolState |= 2;
+ }
+ switch (boolState) {
+ case 0:
+ return kBoolUnknown;
+ case 1:
+ return kBoolTrue;
+ case 2:
+ return kBoolFalse;
+ // If both the condition and its negation are satisfied, the program point
+ // is proven to be impossible.
+ case 3:
+ return kBoolInvalid;
+ default:
+ llvm_unreachable("all cases covered in switch");
+ }
+ }
+}
+
+// We are binding to the address of the Decl here, as the Expr has a different
+// address than the one stored in the framework.
+auto nameToVar(llvm::StringRef name) {
+ return declRefExpr(hasType(isAnyPointer()),
+ hasDeclaration(namedDecl(hasName(name)).bind(kVar)));
+}
+
+using ::clang::dataflow::test::AnalysisInputs;
+using ::clang::dataflow::test::AnalysisOutputs;
+using ::clang::dataflow::test::checkDataflow;
+using ::llvm::IsStringMapEntry;
+using ::testing::Args;
+using ::testing::Pair;
+using ::testing::UnorderedElementsAre;
+
+MATCHER_P2(HasNullabilityState, null, nonnull,
+ std::string("has nullability state where isNull is ") + null +
+ " and isNonnull is " + nonnull) {
+ return checkNullabilityState(
+ cast_or_null<BoolValue>(arg.first->getProperty(kIsNonnull)),
+ *arg.second) == nonnull &&
+ checkNullabilityState(
+ cast_or_null<BoolValue>(arg.first->getProperty(kIsNull)),
+ *arg.second) == null;
+}
+
+MATCHER_P3(HoldsVariable, name, output, checks,
+ ((negation ? "doesn't hold" : "holds") +
+ llvm::StringRef(" a variable in its environment that ") +
+ ::testing::DescribeMatcher<std::pair<Value *, Environment *>>(
+ checks, negation))
+ .str()) {
+ auto MatchResults = match(functionDecl(hasDescendant(nameToVar(name))),
+ *output.Target, output.ASTCtx);
+ assert(!MatchResults.empty());
+
+ const auto *pointerExpr = MatchResults[0].template getNodeAs<ValueDecl>(kVar);
+ assert(pointerExpr != nullptr);
+
+ const auto *ExprValue = arg.Env.getValue(*pointerExpr);
+
+ if (ExprValue == nullptr) {
+ return false;
+ }
+
+ return ExplainMatchResult(checks, std::pair{ExprValue, &arg.Env},
+ result_listener);
+}
+
+template <typename MatcherFactory>
+void ExpectDataflowResult(llvm::StringRef Code, MatcherFactory Expectations) {
+ ASSERT_THAT_ERROR(
+ checkDataflow<NullPointerAnalysisModel>(
+ AnalysisInputs<NullPointerAnalysisModel>(
+ Code, hasName("fun"),
+ [](ASTContext &C, Environment &Env) {
+ return NullPointerAnalysisModel(C);
+ })
+ .withASTBuildArgs({"-fsyntax-only", "-std=c++17"}),
+ /*VerifyResults=*/
+ [&Expectations](const llvm::StringMap<DataflowAnalysisState<
+ NullPointerAnalysisModel::Lattice>> &Results,
+ const AnalysisOutputs &Output) {
+ EXPECT_THAT(Results, Expectations(Output));
+ }),
+ llvm::Succeeded());
+}
+
+TEST(NullCheckAfterDereferenceTest, DereferenceTypes) {
+ std::string Code = R"(
+ struct S {
+ int a;
+ };
+
+ void fun(int *p, S *q) {
+ *p = 0; // [[p]]
+
+ q->a = 20; // [[q]]
+ }
+ )";
+ ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto {
+ return UnorderedElementsAre(
+ IsStringMapEntry(
+ "p", HoldsVariable("p", Output,
+ HasNullabilityState(kBoolFalse, kBoolTrue))),
+ IsStringMapEntry(
+ "q", HoldsVariable("q", Output,
+ HasNullabilityState(kBoolFalse, kBoolTrue))));
+ });
+}
+
+TEST(NullCheckAfterDereferenceTest, ConditionalTypes) {
+ std::string Code = R"(
+ void fun(int *p) {
+ if (p) {
+ (void)0; // [[p_true]]
+ } else {
+ (void)0; // [[p_false]]
+ }
+
+ // FIXME: Test ternary op
+ }
+ )";
+ ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto {
+ return UnorderedElementsAre(
+ IsStringMapEntry("p_true", HoldsVariable("p", Output,
+ HasNullabilityState(
+ kBoolFalse, kBoolTrue))),
+ IsStringMapEntry("p_false", HoldsVariable("p", Output,
+ HasNullabilityState(
+ kBoolTrue, kBoolFalse))));
+ });
+}
+
+TEST(NullCheckAfterDereferenceTest, UnrelatedCondition) {
+ std::string Code = R"(
+ void fun(int *p, bool b) {
+ if (b) {
+ *p = 42;
+ (void)0; // [[p_b_true]]
+ } else {
+ (void)0; // [[p_b_false]]
+ }
+
+ (void)0; // [[p_merged]]
+
+ if (b) {
+ (void)0; // [[b_true]]
+
+ if (p) {
+ (void)0; // [[b_p_true]]
+ } else {
+ (void)0; // [[b_p_false]]
+ }
+ }
+ }
+ )";
+ ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto {
+ return UnorderedElementsAre(
+ IsStringMapEntry("p_b_true", HoldsVariable("p", Output,
+ HasNullabilityState(
+ kBoolFalse, kBoolTrue))),
+ IsStringMapEntry(
+ "p_b_false",
+ HoldsVariable("p", Output,
+ HasNullabilityState(kBoolUnknown, kBoolUnknown))),
+ IsStringMapEntry(
+ "p_merged",
+ HoldsVariable("p", Output,
+ HasNullabilityState(kBoolUnknown, kBoolUnknown))),
+ IsStringMapEntry("b_true", HoldsVariable("p", Output,
+ HasNullabilityState(
+ kBoolFalse, kBoolTrue))),
+ IsStringMapEntry("b_p_true", HoldsVariable("p", Output,
+ HasNullabilityState(
+ kBoolFalse, kBoolTrue))),
+ // FIXME: Flow condition is false in this last entry,
+ // should test that instead of an invalid state
+ IsStringMapEntry(
+ "b_p_false",
+ HoldsVariable("p", Output,
+ HasNullabilityState(kBoolInvalid, kBoolInvalid))));
+ });
+}
+
+TEST(NullCheckAfterDereferenceTest, AssignmentOfCommonValues) {
+ std::string Code = R"(
+ using size_t = decltype(sizeof(void*));
+ extern void *malloc(size_t);
+ extern int *ext();
+
+ void fun() {
+ int *p = (int*)malloc(sizeof(int));
+ (void)0; // [[p_malloc]]
+
+ if (p) {
+ *p = 42; // [[p_true]]
+ } else {
+ (void)0; // [[p_false]]
+ }
+
+ (void)0; // [[p_merge]]
+
+ p = nullptr; // [[p_nullptr]]
+
+ p = ext(); // [[p_extern]]
+ }
+ )";
+ ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto {
+ return UnorderedElementsAre(
+ // FIXME: Recognize that malloc (and other functions) are nullable
+ IsStringMapEntry(
+ "p_malloc",
+ HoldsVariable("p", Output,
+ HasNullabilityState(kBoolUnknown, kBoolUnknown))),
+ IsStringMapEntry("p_true", HoldsVariable("p", Output,
+ HasNullabilityState(
+ kBoolFalse, kBoolTrue))),
+ IsStringMapEntry("p_false", HoldsVariable("p", Output,
+ HasNullabilityState(
+ kBoolTrue, kBoolFalse))),
+ IsStringMapEntry(
+ "p_merge",
+ HoldsVariable("p", Output,
+ HasNullabilityState(kBoolUnknown, kBoolUnknown))),
+ IsStringMapEntry(
+ "p_nullptr",
+ HoldsVariable("p", Output,
+ HasNullabilityState(kBoolTrue, kBoolFalse))),
+ IsStringMapEntry(
+ "p_extern",
+ HoldsVariable("p", Output,
+ HasNullabilityState(kBoolUnknown, kBoolUnknown))));
+ });
+}
+
+TEST(NullCheckAfterDereferenceTest, MergeValues) {
+ std::string Code = R"(
+ using size_t = decltype(sizeof(void*));
+ extern void *malloc(size_t);
+
+ void fun(int *p, bool b) {
+ if (p) {
+ *p = 10;
+ } else {
+ p = (int*)malloc(sizeof(int));
+ }
+
+ (void)0; // [[p_merge]]
+ }
+ )";
+ ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto {
+ return UnorderedElementsAre(IsStringMapEntry(
+ "p_merge",
+ // Even if a pointer was nonnull on a branch, it is worth keeping the
+ // more complex formula for more precise analysis.
+ HoldsVariable("p", Output,
+ HasNullabilityState(kBoolUnknown, kBoolUnknown))));
+ });
+}
+
+} // namespace
+} // namespace clang::dataflow
>From 6c26363d11bc9546926326e60f655a9f0a50b9e5 Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Mon, 11 Mar 2024 13:52:24 +0000
Subject: [PATCH 02/14] Reverse-null surface-level fixes
---
.../NullCheckAfterDereferenceCheck.cpp | 18 ++--
clang-tools-extra/docs/ReleaseNotes.rst | 6 ++
.../bugprone/null-check-after-dereference.rst | 12 +--
.../bugprone/null-check-after-dereference.cpp | 85 +++++------------
.../Models/NullPointerAnalysisModel.h | 7 +-
.../Models/NullPointerAnalysisModel.cpp | 92 ++++++++-----------
6 files changed, 82 insertions(+), 138 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
index 7ef3169cc6386..68e0d1700bdb8 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
@@ -66,18 +66,18 @@ analyzeFunction(const FunctionDecl &FuncDecl) {
NullCheckAfterDereferenceDiagnoser Diagnoser;
NullCheckAfterDereferenceDiagnoser::ResultType Diagnostics;
- using LatticeState = DataflowAnalysisState<NullPointerAnalysisModel::Lattice>;
- using DetailMaybeLatticeStates = std::vector<std::optional<LatticeState>>;
+ using State = DataflowAnalysisState<NullPointerAnalysisModel::Lattice>;
+ using DetailMaybeStates = std::vector<std::optional<State>>;
auto DiagnoserImpl = [&ASTCtx, &Diagnoser,
&Diagnostics](const CFGElement &Elt,
- const LatticeState &S) mutable -> void {
+ const State &S) mutable -> void {
auto EltDiagnostics = Diagnoser.diagnose(ASTCtx, &Elt, S.Env);
llvm::move(EltDiagnostics.first, std::back_inserter(Diagnostics.first));
llvm::move(EltDiagnostics.second, std::back_inserter(Diagnostics.second));
};
- Expected<DetailMaybeLatticeStates> BlockToOutputState =
+ Expected<DetailMaybeStates> BlockToOutputState =
dataflow::runDataflowAnalysis(*Context, Analysis, Env, DiagnoserImpl);
if (llvm::Error E = BlockToOutputState.takeError()) {
@@ -116,16 +116,16 @@ analyzeFunction(const FunctionDecl &FuncDecl) {
void NullCheckAfterDereferenceCheck::registerMatchers(MatchFinder *Finder) {
using namespace ast_matchers;
- auto hasPointerValue =
+ auto containsPointerValue =
hasDescendant(NullPointerAnalysisModel::ptrValueMatcher());
Finder->addMatcher(
decl(anyOf(functionDecl(unless(isExpansionInSystemHeader()),
// FIXME: Remove the filter below when lambdas are
// well supported by the check.
unless(hasDeclContext(cxxRecordDecl(isLambda()))),
- hasBody(hasPointerValue)),
+ hasBody(containsPointerValue)),
cxxConstructorDecl(hasAnyConstructorInitializer(
- withInitializer(hasPointerValue)))))
+ withInitializer(containsPointerValue)))))
.bind(FuncID),
this);
}
@@ -141,10 +141,10 @@ void NullCheckAfterDereferenceCheck::check(
return;
if (const auto Diagnostics = analyzeFunction(*FuncDecl)) {
- const auto &[CheckWhenNullLocations, CheckAfterDereferenceLocations] =
+ const auto &[CheckWhenNullLocations, CheckWhenNonnullLocations] =
*Diagnostics;
- for (const auto [WarningLoc, DerefLoc] : CheckAfterDereferenceLocations) {
+ for (const auto [WarningLoc, DerefLoc] : CheckWhenNonnullLocations) {
diag(WarningLoc, "pointer value is checked even though "
"it cannot be null at this point");
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 0d2467210fc66..56d44cb8a2e77 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -116,6 +116,12 @@ New checks
Replaces certain conditional statements with equivalent calls to
``std::min`` or ``std::max``.
+- New :doc:`bugprone-null-check-after-dereference
+ <clang-tidy/checks/bugprone/null-check-after-dereference>` check.
+
+ Finds locations where a pointer's nullability is checked after it has already
+ been dereferenced.
+
New check aliases
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
index b4910867c2017..bfcb94d10d98f 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
@@ -7,7 +7,7 @@ bugprone-null-check-after-dereference
This check uses a flow-sensitive static analysis to produce its
results. Therefore, it may be more resource intensive (RAM, CPU) than the
- average clang-tidy check.
+ average Clang-tidy check.
This check identifies redundant pointer null-checks, by finding cases where the
pointer cannot be null at the location of the null-check.
@@ -33,9 +33,9 @@ Supported pointer operations
Pointer null-checks
-------------------
-The checker currently supports null-checks on pointers that use
+The check currently supports null-checks on pointers that use
``operator bool``, such as when being used as the condition
-for an `if` statement.
+for an ``if`` statement.
.. code-block:: c++
@@ -69,8 +69,8 @@ Pointer star- and arrow-dereferences are supported.
Null-pointer and other value assignments
----------------------------------------
-The checker supports assigning various values to pointers, making them *null*
-or *non-null*. The checker also supports passing pointers of a pointer to
+The check supports assigning various values to pointers, making them *null*
+or *non-null*. The check also supports passing pointers of a pointer to
external functions.
.. code-block:: c++
@@ -103,7 +103,7 @@ Limitations
The check only supports C++ due to limitations in the data-flow framework.
-The annotations ``_nullable`` and ``_nonnull`` are not supported.
+The annotations ``_Nullable`` and ``_Nonnull`` are not supported.
.. code-block:: c++
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
index 21e9eff4290f7..3cbe12c5f46ea 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
@@ -70,7 +70,6 @@ int else_branch_warning(int *p, bool b) {
// CHECK-MESSAGES: :[[@LINE-7]]:5: note: one of the locations where the pointer's value cannot be null
return 0;
} else {
- *p += 20;
return *p;
}
}
@@ -89,31 +88,10 @@ int two_branches_warning(int *p, bool b) {
// CHECK-MESSAGES: :[[@LINE-9]]:5: note: one of the locations where the pointer's value cannot be null
return 0;
} else {
- *p += 20;
return *p;
}
}
-int two_branches_reversed(int *p, bool b) {
- if (!b) {
- *p = 42;
- }
-
- if (b) {
- *p = 20;
- }
-
- if (p) {
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
- // CHECK-MESSAGES: :[[@LINE-9]]:5: note: one of the locations where the pointer's value cannot be null
- return 0;
- } else {
- *p += 20;
- return *p;
- }
-}
-
-
int regular_assignment(int *p, int *q) {
*p = 42;
q = p;
@@ -121,7 +99,6 @@ int regular_assignment(int *p, int *q) {
if (q) {
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
// CHECK-MESSAGES: :[[@LINE-5]]:3: note: one of the locations where the pointer's value cannot be null
- *p += 20;
return *p;
} else {
return 0;
@@ -139,29 +116,26 @@ int nullptr_assignment(int *nullptr_param, bool b) {
}
if (nullptr_assigned) {
- // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
- *nullptr_assigned = 20;
return *nullptr_assigned;
} else {
return 0;
}
}
-extern int *fncall();
-extern void refresh_ref(int *&ptr);
-extern void refresh_ptr(int **ptr);
+extern int *external_fn();
+extern void ref_fn(int *&ptr);
+extern void ptr_fn(int **ptr);
int fncall_reassignment(int *fncall_reassigned) {
*fncall_reassigned = 42;
- fncall_reassigned = fncall();
+ fncall_reassigned = external_fn();
if (fncall_reassigned) {
- // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
*fncall_reassigned = 42;
}
- fncall_reassigned = fncall();
+ fncall_reassigned = external_fn();
*fncall_reassigned = 42;
@@ -171,19 +145,33 @@ int fncall_reassignment(int *fncall_reassigned) {
*fncall_reassigned = 42;
}
- refresh_ptr(&fncall_reassigned);
+ ptr_fn(&fncall_reassigned);
if (fncall_reassigned) {
- // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // FIXME: References of a pointer passed to external functions do not invalidate its value
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-8]]:5: note: one of the locations where the pointer's value cannot be null
+ *fncall_reassigned = 42;
+ }
+
+ *fncall_reassigned = 42;
+
+ ref_fn(fncall_reassigned);
+
+ if (fncall_reassigned) {
+ // FIXME: References of a pointer passed to external functions do not invalidate its value
+ // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-19]]:5: note: one of the locations where the pointer's value cannot be null
*fncall_reassigned = 42;
}
- refresh_ptr(&fncall_reassigned);
+ ptr_fn(&fncall_reassigned);
*fncall_reassigned = 42;
if (fncall_reassigned) {
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
- // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+ // FIXME: Better note tag support, preferably after the reassignment/refresh
+ // CHECK-MESSAGES: :[[@LINE-29]]:5: note: one of the locations where the pointer's value cannot be null
*fncall_reassigned = 42;
return *fncall_reassigned;
} else {
@@ -294,33 +282,6 @@ int cxx17_crash(int *p) {
return 0;
}
-void external_by_ref(int *&p);
-void external_by_ptr(int **p);
-
-int external_invalidates() {
- int *p = nullptr;
-
- external_by_ref(p);
-
- if (p) {
- // FIXME: References of a pointer passed to external functions do not invalidate its value
- // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked but it can only be null at this point
- return *p;
- }
-
- p = nullptr;
-
- external_by_ptr(&p);
-
- if (p) {
- // FIXME: References of a pointer passed to external functions do not invalidate its value
- // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked but it can only be null at this point
- return *p;
- } else {
- return 0;
- }
-}
-
int note_tags() {
// FIXME: Note tags are not appended to declarations
int *ptr = nullptr;
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
index 16b28ecc9ecc9..0fbf04703d639 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
@@ -58,11 +58,6 @@ class NullPointerAnalysisModel
static ast_matchers::StatementMatcher ptrValueMatcher();
- // Used to initialize the storage locations of function arguments.
- // Required to merge these values within the environment.
- void initializeFunctionParameters(const ControlFlowContext &CFCtx,
- Environment &Env);
-
void transfer(const CFGElement &E, NoopLattice &State, Environment &Env);
void transferBranch(bool Branch, const Stmt *E, NoopLattice &State,
@@ -88,6 +83,8 @@ class NullCheckAfterDereferenceDiagnoser {
const Environment &Env;
};
+ /// Checked when known to be null, and checked after already dereferenced,
+ /// respectively.
using ResultType =
std::pair<std::vector<SourceLocation>, std::vector<SourceLocation>>;
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index 49750406a3f43..22a1ec3317a33 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -63,26 +63,21 @@ using SR = SatisfiabilityResult;
// FIXME: These AST matchers should also be exported via the
// NullPointerAnalysisModel class, for tests
-auto ptrToVar(llvm::StringRef VarName = kVar) {
- return traverse(TK_IgnoreUnlessSpelledInSource,
- declRefExpr(hasType(isAnyPointer())).bind(VarName));
+auto ptrWithBinding(llvm::StringRef VarName = kVar) {
+ return expr(hasType(isAnyPointer())).bind(VarName);
}
auto derefMatcher() {
- return traverse(
- TK_IgnoreUnlessSpelledInSource,
- unaryOperator(hasOperatorName("*"), hasUnaryOperand(ptrToVar())));
+ return unaryOperator(hasOperatorName("*"), hasUnaryOperand(ptrWithBinding()));
}
auto arrowMatcher() {
- return traverse(
- TK_IgnoreUnlessSpelledInSource,
- memberExpr(allOf(isArrow(), hasObjectExpression(ptrToVar()))));
+ return memberExpr(allOf(isArrow(), hasObjectExpression(ptrWithBinding())));
}
auto castExprMatcher() {
return castExpr(hasCastKind(CK_PointerToBoolean),
- hasSourceExpression(ptrToVar()))
+ hasSourceExpression(ptrWithBinding()))
.bind(kCond);
}
@@ -100,7 +95,7 @@ auto functionCallMatcher() {
}
auto assignMatcher() {
- return binaryOperation(isAssignmentOperator(), hasLHS(ptrToVar()),
+ return binaryOperation(isAssignmentOperator(), hasLHS(ptrWithBinding()),
hasRHS(expr().bind(kValue)));
}
@@ -153,9 +148,8 @@ inline BoolValue &getVal(llvm::StringRef Name, Value &RootValue) {
void initializeRootValue(Value &RootValue, Environment &Env) {
Arena &A = Env.arena();
- BoolValue *IsNull = cast_or_null<BoolValue>(RootValue.getProperty(kIsNull));
- BoolValue *IsNonnull =
- cast_or_null<BoolValue>(RootValue.getProperty(kIsNonnull));
+ auto *IsNull = cast_or_null<BoolValue>(RootValue.getProperty(kIsNull));
+ auto *IsNonnull = cast_or_null<BoolValue>(RootValue.getProperty(kIsNonnull));
if (!IsNull) {
IsNull = &A.makeAtomValue();
@@ -167,7 +161,7 @@ void initializeRootValue(Value &RootValue, Environment &Env) {
RootValue.setProperty(kIsNonnull, *IsNonnull);
}
- // If the pointer cannot have either a null or nonull value, the state is
+ // If the pointer cannot have either a null or nonnull value, the state is
// unreachable.
// FIXME: This condition is added in all cases when getValue() is called.
// The reason is that on a post-visit step, the initialized Values are used,
@@ -190,14 +184,12 @@ void setGLValue(const Expr &E, Value &Val, Environment &Env) {
void setUnknownValue(const Expr &E, Value &Val, Environment &Env) {
if (E.isGLValue())
setGLValue(E, Val, Env);
- else if (E.isPRValue())
- Env.setValue(E, Val);
else
- llvm_unreachable("all value cases covered");
+ Env.setValue(E, Val);
}
Value *getValue(const Expr &Var, Environment &Env) {
- if (auto *EnvVal = Env.getValue(Var)) {
+ if (Value *EnvVal = Env.getValue(Var)) {
// FIXME: The framework usually creates the values for us, but without the
// null-properties.
initializeRootValue(*EnvVal, Env);
@@ -205,10 +197,7 @@ Value *getValue(const Expr &Var, Environment &Env) {
return EnvVal;
}
- auto *RootValue = Env.createValue(Var.getType());
-
- if (!RootValue)
- return nullptr;
+ Value *RootValue = Env.createValue(Var.getType());
initializeRootValue(*RootValue, Env);
@@ -223,10 +212,7 @@ void matchDereferenceExpr(const Stmt *stmt,
const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
assert(Var != nullptr);
- auto *RootValue = getValue(*Var, Env);
- if (!RootValue) {
- return;
- }
+ Value *RootValue = getValue(*Var, Env);
Env.assume(Env.arena().makeNot(getVal(kIsNull, *RootValue).formula()));
}
@@ -238,14 +224,9 @@ void matchCastExpr(const CastExpr *cond, const MatchFinder::MatchResult &Result,
const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
assert(Var != nullptr);
- auto *RootValue = getValue(*Var, Env);
- if (!RootValue) {
- return;
- }
+ Value *RootValue = getValue(*Var, Env);
- auto *NewRootValue = Env.createValue(Var->getType());
- if (!NewRootValue)
- return;
+ Value *NewRootValue = Env.createValue(Var->getType());
setGLValue(*Var, *NewRootValue, Env);
@@ -271,13 +252,9 @@ void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result,
const auto *PrVar = Result.Nodes.getNodeAs<Expr>(kVar);
assert(PrVar != nullptr);
- auto *RootValue = Env.getValue(*PrVar);
+ Value *RootValue = Env.getValue(*PrVar);
if (!RootValue) {
RootValue = Env.createValue(PrVar->getType());
-
- if (!RootValue) {
- return;
- }
}
RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(true));
@@ -291,10 +268,7 @@ void matchAddressofExpr(const Expr *expr,
const auto *PrVar = Result.Nodes.getNodeAs<Expr>(kVar);
assert(PrVar != nullptr);
- auto *RootValue = Env.createValue(PrVar->getType());
- if (!RootValue) {
- return;
- }
+ Value *RootValue = Env.createValue(PrVar->getType());
RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(false));
RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(true));
@@ -313,9 +287,7 @@ void matchAnyPointerExpr(const Expr *fncall,
if (Env.getValue(*Var))
return;
- auto *RootValue = Env.createValue(Var->getType());
- if (!RootValue)
- return;
+ Value *RootValue = Env.createValue(Var->getType());
initializeRootValue(*RootValue, Env);
setUnknownValue(*Var, *RootValue, Env);
@@ -329,7 +301,7 @@ diagnoseDerefLocation(const Expr *Deref, const MatchFinder::MatchResult &Result,
const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
assert(Var != nullptr);
- auto *RootValue = Env.getValue(*Var);
+ Value *RootValue = Env.getValue(*Var);
if (!RootValue)
return {};
@@ -351,7 +323,7 @@ diagnoseAssignLocation(const Expr *Assign,
const auto *RHSVar = Result.Nodes.getNodeAs<Expr>(kValue);
assert(RHSVar != nullptr);
- auto *RHSValue = Env.getValue(*RHSVar);
+ Value *RHSValue = Env.getValue(*RHSVar);
if (!RHSValue)
return {};
@@ -372,7 +344,7 @@ diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
assert(Var != nullptr);
- if (auto *RootValue = Env.getValue(*Var)) {
+ if (Value *RootValue = Env.getValue(*Var)) {
// FIXME: The framework usually creates the values for us, but without the
// nullability properties.
if (RootValue->getProperty(kIsNull) && RootValue->getProperty(kIsNonnull)) {
@@ -383,6 +355,7 @@ diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
bool Inserted =
WarningLocToVal.try_emplace(Var->getBeginLoc(), RootValue).second;
assert(Inserted && "multiple warnings at the same source location");
+ (void)Inserted;
return {{}, {Var->getBeginLoc()}};
}
@@ -391,6 +364,7 @@ diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
bool Inserted =
WarningLocToVal.try_emplace(Var->getBeginLoc(), RootValue).second;
assert(Inserted && "multiple warnings at the same source location");
+ (void)Inserted;
return {{Var->getBeginLoc()}, {}};
}
@@ -441,7 +415,7 @@ NullPointerAnalysisModel::NullPointerAnalysisModel(ASTContext &Context)
BranchTransferMatchSwitch(buildBranchTransferMatchSwitch()) {}
ast_matchers::StatementMatcher NullPointerAnalysisModel::ptrValueMatcher() {
- return ptrToVar();
+ return ptrWithBinding();
}
void NullPointerAnalysisModel::transfer(const CFGElement &E, NoopLattice &State,
@@ -467,8 +441,11 @@ void NullPointerAnalysisModel::join(QualType Type, const Value &Val1,
return;
const auto MergeValues = [&](llvm::StringRef Name) -> BoolValue & {
- BoolValue *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
- BoolValue *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
+ auto *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
+ auto *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
+
+ if (LHSVar == RHSVar)
+ return *LHSVar;
SatisfiabilityResult LHSResult = computeSatisfiability(LHSVar, Env1);
SatisfiabilityResult RHSResult = computeSatisfiability(RHSVar, Env2);
@@ -518,8 +495,11 @@ ComparisonResult NullPointerAnalysisModel::compare(QualType Type,
// Evaluate values, but different values compare to Unknown.
auto CompareValues = [&](llvm::StringRef Name) -> ComparisonResult {
- BoolValue *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
- BoolValue *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
+ auto *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
+ auto *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
+
+ if (LHSVar == RHSVar)
+ return ComparisonResult::Same;
SatisfiabilityResult LHSResult = computeSatisfiability(LHSVar, Env1);
SatisfiabilityResult RHSResult = computeSatisfiability(RHSVar, Env2);
@@ -559,8 +539,8 @@ ComparisonResult compareAndReplace(QualType Type, Value &Val1,
return ComparisonResult::Unknown;
auto FastCompareValues = [&](llvm::StringRef Name) -> ComparisonResult {
- BoolValue *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
- BoolValue *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
+ auto *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
+ auto *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
SatisfiabilityResult LHSResult = shallowComputeSatisfiability(LHSVar, Env1);
SatisfiabilityResult RHSResult = shallowComputeSatisfiability(RHSVar, Env2);
>From b5f09c22d7e4e293c8e7f558767d153db1988953 Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Mon, 11 Mar 2024 14:04:14 +0000
Subject: [PATCH 03/14] Fix crash when analyzing anonymous lambda fns
---
.../NullCheckAfterDereferenceCheck.cpp | 6 ++-
.../bugprone/null-check-after-dereference.cpp | 43 ++++++-------------
2 files changed, 17 insertions(+), 32 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
index 68e0d1700bdb8..9a2938d89af19 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
@@ -124,8 +124,10 @@ void NullCheckAfterDereferenceCheck::registerMatchers(MatchFinder *Finder) {
// well supported by the check.
unless(hasDeclContext(cxxRecordDecl(isLambda()))),
hasBody(containsPointerValue)),
- cxxConstructorDecl(hasAnyConstructorInitializer(
- withInitializer(containsPointerValue)))))
+ cxxConstructorDecl(
+ unless(hasDeclContext(cxxRecordDecl(isLambda()))),
+ hasAnyConstructorInitializer(
+ withInitializer(containsPointerValue)))))
.bind(FuncID),
this);
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
index 3cbe12c5f46ea..380a0a1c679e2 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
@@ -4,7 +4,7 @@ struct S {
int a;
};
-int warning_deref(int *p) {
+void warning_deref(int *p) {
*p = 42;
if (p) {
@@ -12,49 +12,38 @@ int warning_deref(int *p) {
// CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
// FIXME: If there's a direct path, make the error message more precise, ie. remove `one of the locations`
*p += 20;
- return *p;
- } else {
- return 0;
}
}
-int warning_member(S *q) {
+void warning_member(S *q) {
q->a = 42;
if (q) {
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
// CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
q->a += 20;
- return q->a;
- } else {
- return 0;
}
}
-int negative_warning(int *p) {
+void negative_warning(int *p) {
*p = 42;
if (!p) {
// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: pointer value is checked even though it cannot be null at this point
// CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
- return 0;
- } else {
- *p += 20;
- return *p;
+ return;
}
+
+ *p += 20;
}
-int no_warning(int *p, bool b) {
+void no_warning(int *p, bool b) {
if (b) {
*p = 42;
}
if (p) {
- // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
*p += 20;
- return *p;
- } else {
- return 0;
}
}
@@ -195,18 +184,6 @@ int chained_references(int *a, int *b, int *c, int *d, int *e) {
}
if (c) {
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
- // CHECK-MESSAGES: :[[@LINE-5]]:5: note: one of the locations where the pointer's value cannot be null
- *d = 42;
- }
-
- if (d) {
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
- // CHECK-MESSAGES: :[[@LINE-5]]:5: note: one of the locations where the pointer's value cannot be null
- *e = 42;
- }
-
- if (e) {
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
// CHECK-MESSAGES: :[[@LINE-5]]:5: note: one of the locations where the pointer's value cannot be null
return *a;
@@ -282,6 +259,12 @@ int cxx17_crash(int *p) {
return 0;
}
+// In an earlier version, the check would crash when encountering anonymous lambdas.
+void lambda_crash(int *p) {
+ auto f = [p](){ *p = 42; };
+ f();
+}
+
int note_tags() {
// FIXME: Note tags are not appended to declarations
int *ptr = nullptr;
>From f20e87f79017f3bb5cab63b3b5d03595746a6538 Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Mon, 8 Apr 2024 08:29:08 +0000
Subject: [PATCH 04/14] Review 1
* Add handling for `!= nullptr`, `!= ptr` operators
* Re-add TK_IgnoreUnlessSpelledInSource
* Refactor handling for null-check expressions (transfer instead of branchTransfer)
* Fix crash from `&(PrVar)` not always being PrVar
* Fix formatting and docs
---
clang-tools-extra/docs/ReleaseNotes.rst | 12 +-
.../bugprone/null-check-after-dereference.rst | 3 +-
.../bugprone/null-check-after-dereference.cpp | 41 ++++-
.../Models/NullPointerAnalysisModel.h | 4 +-
.../Models/NullPointerAnalysisModel.cpp | 156 ++++++++++++++----
5 files changed, 172 insertions(+), 44 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 56d44cb8a2e77..005b1bd6b5e32 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,12 @@ Improvements to clang-tidy
New checks
^^^^^^^^^^
+- New :doc:`bugprone-null-check-after-dereference
+ <clang-tidy/checks/bugprone/null-check-after-dereference>` check.
+
+ This check identifies redundant pointer null-checks, by finding cases where
+ the pointer cannot be null at the location of the null-check.
+
- New :doc:`modernize-use-designated-initializers
<clang-tidy/checks/modernize/use-designated-initializers>` check.
@@ -116,12 +122,6 @@ New checks
Replaces certain conditional statements with equivalent calls to
``std::min`` or ``std::max``.
-- New :doc:`bugprone-null-check-after-dereference
- <clang-tidy/checks/bugprone/null-check-after-dereference>` check.
-
- Finds locations where a pointer's nullability is checked after it has already
- been dereferenced.
-
New check aliases
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
index bfcb94d10d98f..756d1f5453437 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
@@ -35,7 +35,8 @@ Pointer null-checks
The check currently supports null-checks on pointers that use
``operator bool``, such as when being used as the condition
-for an ``if`` statement.
+for an ``if`` statement. It also supports comparisons such as ``!= nullptr``, and
+``== other_ptr``.
.. code-block:: c++
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
index 380a0a1c679e2..e87ba19b25b75 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
@@ -43,10 +43,44 @@ void no_warning(int *p, bool b) {
}
if (p) {
+ // no-warning
*p += 20;
}
}
+void equals_nullptr(int *p) {
+ *p = 42;
+
+ if (p == nullptr) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+ return;
+ }
+
+ if (p != nullptr) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-10]]:3: note: one of the locations where the pointer's value cannot be null
+ *p += 20;
+ }
+
+ if (nullptr != p) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-16]]:3: note: one of the locations where the pointer's value cannot be null
+ *p += 20;
+ }
+}
+
+void equals_other_ptr(int *p, int *q) {
+ if (q)
+ return;
+
+ if (p == q) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: pointer value is checked but it can only be null at this point
+ // CHECK-MESSAGES: :[[@LINE-5]]:7: note: one of the locations where the pointer's value can only be null
+ return;
+ }
+}
+
int else_branch_warning(int *p, bool b) {
if (b) {
*p = 42;
@@ -105,6 +139,7 @@ int nullptr_assignment(int *nullptr_param, bool b) {
}
if (nullptr_assigned) {
+ // no-warning
return *nullptr_assigned;
} else {
return 0;
@@ -197,9 +232,8 @@ int chained_if(int *a) {
return 0;
}
- // FIXME: Negations are not tracked properly when the previous conditional returns
if (a) {
- // --CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
*a += 20;
return *a;
} else {
@@ -212,8 +246,7 @@ int double_if(int *a) {
if (a) {
// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: pointer value is checked even though it cannot be null at this point
// --CHECK-MESSAGES: :[[@LINE-3]]:5: note: one of the locations where the pointer's value cannot be null
- // FIXME: Add warning for branch statements where pointer is not null afterwards
- *a += 20;
+ // FIXME: Add warning for branch satements where pointer is not null afterwards
return *a;
} else {
return 0;
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
index 0fbf04703d639..2989b554feeed 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
@@ -83,8 +83,8 @@ class NullCheckAfterDereferenceDiagnoser {
const Environment &Env;
};
- /// Checked when known to be null, and checked after already dereferenced,
- /// respectively.
+ /// Returns source locations for pointers that were checked when known to be
+ // null, and checked after already dereferenced, respectively.
using ResultType =
std::pair<std::vector<SourceLocation>, std::vector<SourceLocation>>;
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index 22a1ec3317a33..554beaaead041 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -64,7 +64,8 @@ using SR = SatisfiabilityResult;
// FIXME: These AST matchers should also be exported via the
// NullPointerAnalysisModel class, for tests
auto ptrWithBinding(llvm::StringRef VarName = kVar) {
- return expr(hasType(isAnyPointer())).bind(VarName);
+ return traverse(TK_IgnoreUnlessSpelledInSource,
+ expr(hasType(isAnyPointer())).bind(VarName));
}
auto derefMatcher() {
@@ -81,8 +82,8 @@ auto castExprMatcher() {
.bind(kCond);
}
-auto nullptrMatcher() {
- return castExpr(hasCastKind(CK_NullToPointer)).bind(kVar);
+auto nullptrMatcher(llvm::StringRef VarName = kVar) {
+ return castExpr(hasCastKind(CK_NullToPointer)).bind(VarName);
}
auto addressofMatcher() {
@@ -99,6 +100,19 @@ auto assignMatcher() {
hasRHS(expr().bind(kValue)));
}
+auto nullCheckExprMatcher() {
+ return binaryOperator(hasAnyOperatorName("==", "!="),
+ hasOperands(ptrWithBinding(), nullptrMatcher(kValue)));
+}
+
+// FIXME: When TK_IgnoreUnlessSpelledInSource is removed from ptrWithBinding(),
+// this matcher should be merged with nullCheckExprMatcher().
+auto equalExprMatcher() {
+ return binaryOperator(hasAnyOperatorName("==", "!="),
+ hasOperands(ptrWithBinding(kVar),
+ ptrWithBinding(kValue)));
+}
+
auto anyPointerMatcher() { return expr(hasType(isAnyPointer())).bind(kVar); }
// Only computes simple comparisons against the literals True and False in the
@@ -217,34 +231,72 @@ void matchDereferenceExpr(const Stmt *stmt,
Env.assume(Env.arena().makeNot(getVal(kIsNull, *RootValue).formula()));
}
-void matchCastExpr(const CastExpr *cond, const MatchFinder::MatchResult &Result,
- NullPointerAnalysisModel::TransferArgs &Data) {
- auto [IsNonnullBranch, Env] = Data;
-
+void matchNullCheckExpr(const Expr *NullCheck,
+ const MatchFinder::MatchResult &Result,
+ Environment &Env) {
const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
assert(Var != nullptr);
- Value *RootValue = getValue(*Var, Env);
-
- Value *NewRootValue = Env.createValue(Var->getType());
+ // (bool)p or (p != nullptr)
+ bool IsNonnullOp = true;
+ if (auto *BinOp = dyn_cast<BinaryOperator>(NullCheck);
+ BinOp->getOpcode() == BO_EQ) {
+ IsNonnullOp = false;
+ }
- setGLValue(*Var, *NewRootValue, Env);
+ Value *RootValue = getValue(*Var, Env);
Arena &A = Env.arena();
BoolValue &IsNonnull = getVal(kIsNonnull, *RootValue);
BoolValue &IsNull = getVal(kIsNull, *RootValue);
+ BoolValue *CondValue = cast_or_null<BoolValue>(Env.getValue(*NullCheck));
+ if (!CondValue) {
+ CondValue = &A.makeAtomValue();
+ Env.setValue(*NullCheck, *CondValue);
+ }
+ const Formula &CondFormula = IsNonnullOp ? CondValue->formula()
+ : A.makeNot(CondValue->formula());
+
+ Env.assume(A.makeImplies(CondFormula, A.makeNot(IsNull.formula())));
+ Env.assume(A.makeImplies(A.makeNot(CondFormula),
+ A.makeNot(IsNonnull.formula())));
+}
+
+void matchEqualExpr(const BinaryOperator *EqualExpr,
+ const MatchFinder::MatchResult &Result,
+ Environment &Env) {
+ bool IsNotEqualsOp = EqualExpr->getOpcode() == BO_NE;
+
+ const auto *LHSVar = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(LHSVar != nullptr);
- if (IsNonnullBranch) {
- Env.assume(A.makeNot(IsNull.formula()));
- NewRootValue->setProperty(kIsNull, Env.getBoolLiteralValue(false));
+ const auto *RHSVar = Result.Nodes.getNodeAs<Expr>(kValue);
+ assert(RHSVar != nullptr);
- NewRootValue->setProperty(kIsNonnull, IsNonnull);
- } else {
- NewRootValue->setProperty(kIsNull, IsNull);
+ Arena &A = Env.arena();
+ Value *LHSValue = getValue(*LHSVar, Env);
+ Value *RHSValue = getValue(*RHSVar, Env);
- Env.assume(A.makeNot(IsNonnull.formula()));
- NewRootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(false));
+ BoolValue *CondValue = cast_or_null<BoolValue>(Env.getValue(*EqualExpr));
+ if (!CondValue) {
+ CondValue = &A.makeAtomValue();
+ Env.setValue(*EqualExpr, *CondValue);
}
+
+ const Formula &CondFormula = IsNotEqualsOp ? A.makeNot(CondValue->formula())
+ : CondValue->formula();
+
+ // If the pointers are equal, the nullability properties are the same.
+ Env.assume(A.makeImplies(CondFormula,
+ A.makeAnd(A.makeEquals(getVal(kIsNull, *LHSValue).formula(),
+ getVal(kIsNull, *RHSValue).formula()),
+ A.makeEquals(getVal(kIsNonnull, *LHSValue).formula(),
+ getVal(kIsNonnull, *RHSValue).formula()))));
+
+ // If the pointers are not equal, at most one of the pointers is null.
+ Env.assume(A.makeImplies(A.makeNot(CondFormula),
+ A.makeNot(A.makeAnd(getVal(kIsNull, *LHSValue).formula(),
+ getVal(kIsNull, *RHSValue).formula()))));
}
void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result,
@@ -255,24 +307,31 @@ void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result,
Value *RootValue = Env.getValue(*PrVar);
if (!RootValue) {
RootValue = Env.createValue(PrVar->getType());
+ Env.setValue(*PrVar, *RootValue);
}
RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(true));
RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(false));
- Env.setValue(*PrVar, *RootValue);
}
void matchAddressofExpr(const Expr *expr,
const MatchFinder::MatchResult &Result,
Environment &Env) {
- const auto *PrVar = Result.Nodes.getNodeAs<Expr>(kVar);
- assert(PrVar != nullptr);
+ const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(Var != nullptr);
+
+ Value *RootValue = Env.getValue(*Var);
+ if (!RootValue) {
+ RootValue = Env.createValue(Var->getType());
- Value *RootValue = Env.createValue(PrVar->getType());
+ if (!RootValue)
+ return;
+
+ setUnknownValue(*Var, *RootValue, Env);
+ }
RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(false));
RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(true));
- Env.setValue(*PrVar, *RootValue);
}
void matchAnyPointerExpr(const Expr *fncall,
@@ -336,9 +395,9 @@ diagnoseAssignLocation(const Expr *Assign,
}
NullCheckAfterDereferenceDiagnoser::ResultType
-diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
- const NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) {
-
+diagnoseNullCheckExpr(const Expr *NullCheck,
+ const MatchFinder::MatchResult &Result,
+ const NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) {
auto [ValToDerefLoc, WarningLocToVal, Env] = Data;
const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
@@ -352,6 +411,7 @@ diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
bool IsNonnull = Env.allows(getVal(kIsNonnull, *RootValue).formula());
if (!IsNull && IsNonnull) {
+ // FIXME: Separate function
bool Inserted =
WarningLocToVal.try_emplace(Var->getBeginLoc(), RootValue).second;
assert(Inserted && "multiple warnings at the same source location");
@@ -370,16 +430,44 @@ diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
}
}
- // If no matches are found, the cast itself signals a special location
+ // If no matches are found, the null-check itself signals a special location
auto [It, Inserted] = ValToDerefLoc.try_emplace(RootValue, nullptr);
if (Inserted)
- It->second = Stmt;
+ It->second = NullCheck;
}
return {};
}
+NullCheckAfterDereferenceDiagnoser::ResultType
+diagnoseEqualExpr(const Expr *PtrCheck, const MatchFinder::MatchResult &Result,
+ NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) {
+ auto [ValToDerefLoc, WarningLocToVal, Env] = Data;
+
+ const auto *LHSVar = Result.Nodes.getNodeAs<Expr>(kVar);
+ assert(LHSVar != nullptr);
+ const auto *RHSVar = Result.Nodes.getNodeAs<Expr>(kValue);
+ assert(RHSVar != nullptr);
+
+ Arena &A = Env.arena();
+ std::vector<SourceLocation> NullVarLocations;
+
+ if (Value *LHSValue = Env.getValue(*LHSVar);
+ Env.proves(A.makeNot(getVal(kIsNonnull, *LHSValue).formula()))) {
+ WarningLocToVal.try_emplace(LHSVar->getBeginLoc(), LHSValue);
+ NullVarLocations.push_back(LHSVar->getBeginLoc());
+ }
+
+ if (Value *RHSValue = Env.getValue(*RHSVar);
+ Env.proves(A.makeNot(getVal(kIsNonnull, *RHSValue).formula()))) {
+ WarningLocToVal.try_emplace(RHSVar->getBeginLoc(), RHSValue);
+ NullVarLocations.push_back(RHSVar->getBeginLoc());
+ }
+
+ return {NullVarLocations, {}};
+}
+
auto buildTransferMatchSwitch() {
return CFGMatchSwitchBuilder<Environment>()
.CaseOfCFGStmt<Stmt>(derefMatcher(), matchDereferenceExpr)
@@ -388,12 +476,16 @@ auto buildTransferMatchSwitch() {
.CaseOfCFGStmt<Expr>(addressofMatcher(), matchAddressofExpr)
.CaseOfCFGStmt<Expr>(functionCallMatcher(), matchAnyPointerExpr)
.CaseOfCFGStmt<Expr>(anyPointerMatcher(), matchAnyPointerExpr)
+ .CaseOfCFGStmt<Expr>(castExprMatcher(), matchNullCheckExpr)
+ .CaseOfCFGStmt<Expr>(nullCheckExprMatcher(), matchNullCheckExpr)
+ .CaseOfCFGStmt<BinaryOperator>(equalExprMatcher(), matchEqualExpr)
.Build();
}
auto buildBranchTransferMatchSwitch() {
return ASTMatchSwitchBuilder<Stmt, NullPointerAnalysisModel::TransferArgs>()
- .CaseOf<CastExpr>(castExprMatcher(), matchCastExpr)
+ // .CaseOf<CastExpr>(castExprMatcher(), matchNullCheckExpr)
+ // .CaseOf<BinaryOperator>(equalExprMatcher(), matchEqualExpr)
.Build();
}
@@ -403,7 +495,9 @@ auto buildDiagnoseMatchSwitch() {
.CaseOfCFGStmt<Expr>(derefMatcher(), diagnoseDerefLocation)
.CaseOfCFGStmt<Expr>(arrowMatcher(), diagnoseDerefLocation)
.CaseOfCFGStmt<Expr>(assignMatcher(), diagnoseAssignLocation)
- .CaseOfCFGStmt<CastExpr>(castExprMatcher(), diagnoseCastExpr)
+ .CaseOfCFGStmt<Expr>(castExprMatcher(), diagnoseNullCheckExpr)
+ .CaseOfCFGStmt<Expr>(nullCheckExprMatcher(), diagnoseNullCheckExpr)
+ .CaseOfCFGStmt<Expr>(equalExprMatcher(), diagnoseEqualExpr)
.Build();
}
>From a673ac26a3b4d39e59a2187a87470697403210ad Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Mon, 8 Apr 2024 08:51:30 +0000
Subject: [PATCH 05/14] Fix upstream fn signature change
---
.../FlowSensitive/Models/NullPointerAnalysisModel.h | 2 +-
.../FlowSensitive/Models/NullPointerAnalysisModel.cpp | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
index 2989b554feeed..bc2d87d1acde5 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
@@ -71,7 +71,7 @@ class NullPointerAnalysisModel
const Environment &Env1, const Value &Val2,
const Environment &Env2) override;
- Value *widen(QualType Type, Value &Prev, const Environment &PrevEnv,
+ std::optional<WidenResult> widen(QualType Type, Value &Prev, const Environment &PrevEnv,
Value &Current, Environment &CurrentEnv) override;
};
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index 554beaaead041..b785a7755009b 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -668,20 +668,20 @@ ComparisonResult compareAndReplace(QualType Type, Value &Val1,
return ComparisonResult::Same;
}
-Value *NullPointerAnalysisModel::widen(QualType Type, Value &Prev,
+std::optional<WidenResult> NullPointerAnalysisModel::widen(QualType Type, Value &Prev,
const Environment &PrevEnv,
Value &Current,
Environment &CurrentEnv) {
if (!Type->isAnyPointerType())
- return nullptr;
+ return std::nullopt;
switch (compareAndReplace(Type, Prev, PrevEnv, Current, CurrentEnv)) {
case ComparisonResult::Same:
- return &Prev;
+ return WidenResult{&Prev, LatticeEffect::Unchanged};
case ComparisonResult::Unknown:
- return nullptr;
+ return std::nullopt;
case ComparisonResult::Different:
- return &Current;
+ return WidenResult{&Current, LatticeEffect::Changed};
}
}
>From 3b0b514be208c5ca022091e68b61794c4478fc6e Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Wed, 10 Apr 2024 06:47:38 +0000
Subject: [PATCH 06/14] Formatting
---
.../Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
index bc2d87d1acde5..64e58780f2d40 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
@@ -71,8 +71,9 @@ class NullPointerAnalysisModel
const Environment &Env1, const Value &Val2,
const Environment &Env2) override;
- std::optional<WidenResult> widen(QualType Type, Value &Prev, const Environment &PrevEnv,
- Value &Current, Environment &CurrentEnv) override;
+ std::optional<WidenResult> widen(QualType Type, Value &Prev,
+ const Environment &PrevEnv, Value &Current,
+ Environment &CurrentEnv) override;
};
class NullCheckAfterDereferenceDiagnoser {
>From 7c9c6e29906e972269af3219cabfd4b14238a5f6 Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Wed, 10 Apr 2024 08:04:35 +0000
Subject: [PATCH 07/14] Aggressive Top skipping, first pass
---
.../Models/NullPointerAnalysisModel.cpp | 98 +++++++++++++------
1 file changed, 66 insertions(+), 32 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index b785a7755009b..344fc5352b7d2 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -59,7 +59,15 @@ enum class SatisfiabilityResult {
Unknown
};
+enum class CompareResult {
+ Same,
+ Different,
+ Top,
+ Unknown
+};
+
using SR = SatisfiabilityResult;
+using CR = CompareResult;
// FIXME: These AST matchers should also be exported via the
// NullPointerAnalysisModel class, for tests
@@ -228,7 +236,12 @@ void matchDereferenceExpr(const Stmt *stmt,
Value *RootValue = getValue(*Var, Env);
- Env.assume(Env.arena().makeNot(getVal(kIsNull, *RootValue).formula()));
+ BoolValue &IsNull = getVal(kIsNull, *RootValue);
+
+ if (&IsNull == &Env.makeTopBoolValue())
+ return;
+
+ Env.assume(Env.arena().makeNot(IsNull.formula()));
}
void matchNullCheckExpr(const Expr *NullCheck,
@@ -249,6 +262,11 @@ void matchNullCheckExpr(const Expr *NullCheck,
Arena &A = Env.arena();
BoolValue &IsNonnull = getVal(kIsNonnull, *RootValue);
BoolValue &IsNull = getVal(kIsNull, *RootValue);
+
+ if (&IsNonnull == &Env.makeTopBoolValue() ||
+ &IsNull == &Env.makeTopBoolValue())
+ return;
+
BoolValue *CondValue = cast_or_null<BoolValue>(Env.getValue(*NullCheck));
if (!CondValue) {
CondValue = &A.makeAtomValue();
@@ -277,6 +295,17 @@ void matchEqualExpr(const BinaryOperator *EqualExpr,
Value *LHSValue = getValue(*LHSVar, Env);
Value *RHSValue = getValue(*RHSVar, Env);
+ BoolValue &LHSNonnull = getVal(kIsNonnull, *LHSValue);
+ BoolValue &LHSNull = getVal(kIsNull, *LHSValue);
+ BoolValue &RHSNonnull = getVal(kIsNonnull, *RHSValue);
+ BoolValue &RHSNull = getVal(kIsNull, *RHSValue);
+
+ if (&LHSNonnull == &Env.makeTopBoolValue() ||
+ &RHSNonnull == &Env.makeTopBoolValue() ||
+ &LHSNull == &Env.makeTopBoolValue() ||
+ &RHSNull == &Env.makeTopBoolValue())
+ return;
+
BoolValue *CondValue = cast_or_null<BoolValue>(Env.getValue(*EqualExpr));
if (!CondValue) {
CondValue = &A.makeAtomValue();
@@ -286,17 +315,15 @@ void matchEqualExpr(const BinaryOperator *EqualExpr,
const Formula &CondFormula = IsNotEqualsOp ? A.makeNot(CondValue->formula())
: CondValue->formula();
+ // FIXME: Simplify formulas
// If the pointers are equal, the nullability properties are the same.
Env.assume(A.makeImplies(CondFormula,
- A.makeAnd(A.makeEquals(getVal(kIsNull, *LHSValue).formula(),
- getVal(kIsNull, *RHSValue).formula()),
- A.makeEquals(getVal(kIsNonnull, *LHSValue).formula(),
- getVal(kIsNonnull, *RHSValue).formula()))));
+ A.makeAnd(A.makeEquals(LHSNull.formula(), RHSNull.formula()),
+ A.makeEquals(LHSNonnull.formula(), RHSNonnull.formula()))));
// If the pointers are not equal, at most one of the pointers is null.
Env.assume(A.makeImplies(A.makeNot(CondFormula),
- A.makeNot(A.makeAnd(getVal(kIsNull, *LHSValue).formula(),
- getVal(kIsNull, *RHSValue).formula()))));
+ A.makeNot(A.makeAnd(LHSNull.formula(), RHSNull.formula()))));
}
void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result,
@@ -307,6 +334,7 @@ void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result,
Value *RootValue = Env.getValue(*PrVar);
if (!RootValue) {
RootValue = Env.createValue(PrVar->getType());
+ assert(RootValue && "Failed to create nullptr value");
Env.setValue(*PrVar, *RootValue);
}
@@ -348,7 +376,7 @@ void matchAnyPointerExpr(const Expr *fncall,
Value *RootValue = Env.createValue(Var->getType());
- initializeRootValue(*RootValue, Env);
+ // initializeRootValue(*RootValue, Env);
setUnknownValue(*Var, *RootValue, Env);
}
@@ -539,7 +567,7 @@ void NullPointerAnalysisModel::join(QualType Type, const Value &Val1,
auto *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
if (LHSVar == RHSVar)
- return *LHSVar;
+ return LHSVar ? *LHSVar : MergedEnv.makeAtomicBoolValue();
SatisfiabilityResult LHSResult = computeSatisfiability(LHSVar, Env1);
SatisfiabilityResult RHSResult = computeSatisfiability(RHSVar, Env2);
@@ -588,37 +616,40 @@ ComparisonResult NullPointerAnalysisModel::compare(QualType Type,
return ComparisonResult::Unknown;
// Evaluate values, but different values compare to Unknown.
- auto CompareValues = [&](llvm::StringRef Name) -> ComparisonResult {
+ auto CompareValues = [&](llvm::StringRef Name) -> CR {
auto *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
auto *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
if (LHSVar == RHSVar)
- return ComparisonResult::Same;
+ return (LHSVar == &Env1.makeTopBoolValue()) ? CR::Top : CR::Same;
SatisfiabilityResult LHSResult = computeSatisfiability(LHSVar, Env1);
SatisfiabilityResult RHSResult = computeSatisfiability(RHSVar, Env2);
if (LHSResult == SR::Top || RHSResult == SR::Top)
- return ComparisonResult::Same;
+ return CR::Top;
if (LHSResult == SR::Unknown || RHSResult == SR::Unknown)
- return ComparisonResult::Unknown;
+ return CR::Unknown;
if (LHSResult == RHSResult)
- return ComparisonResult::Same;
+ return CR::Same;
- return ComparisonResult::Different;
+ return CR::Different;
};
- ComparisonResult NullComparison = CompareValues(kIsNull);
- ComparisonResult NonnullComparison = CompareValues(kIsNonnull);
+ CR NullComparison = CompareValues(kIsNull);
+ CR NonnullComparison = CompareValues(kIsNonnull);
+
+ if (NullComparison == CR::Top || NonnullComparison == CR::Top)
+ return ComparisonResult::Same;
- if (NullComparison == ComparisonResult::Different ||
- NonnullComparison == ComparisonResult::Different)
+ if (NullComparison == CR::Different ||
+ NonnullComparison == CR::Different)
return ComparisonResult::Different;
- if (NullComparison == ComparisonResult::Unknown ||
- NonnullComparison == ComparisonResult::Unknown)
+ if (NullComparison == CR::Unknown ||
+ NonnullComparison == CR::Unknown)
return ComparisonResult::Unknown;
return ComparisonResult::Same;
@@ -632,7 +663,7 @@ ComparisonResult compareAndReplace(QualType Type, Value &Val1,
if (!Type->isAnyPointerType())
return ComparisonResult::Unknown;
- auto FastCompareValues = [&](llvm::StringRef Name) -> ComparisonResult {
+ auto FastCompareValues = [&](llvm::StringRef Name) -> CR {
auto *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
auto *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
@@ -641,28 +672,31 @@ ComparisonResult compareAndReplace(QualType Type, Value &Val1,
if (LHSResult == SR::Top || RHSResult == SR::Top) {
Val2.setProperty(Name, Env2.makeTopBoolValue());
- return ComparisonResult::Same;
+ return CR::Top;
}
if (LHSResult == SR::Unknown || RHSResult == SR::Unknown)
- return ComparisonResult::Unknown;
+ return CR::Unknown;
if (LHSResult == RHSResult)
- return ComparisonResult::Same;
+ return CR::Same;
Val2.setProperty(Name, Env2.makeTopBoolValue());
- return ComparisonResult::Different;
+ return CR::Different;
};
- ComparisonResult NullComparison = FastCompareValues(kIsNull);
- ComparisonResult NonnullComparison = FastCompareValues(kIsNonnull);
+ CR NullComparison = FastCompareValues(kIsNull);
+ CR NonnullComparison = FastCompareValues(kIsNonnull);
+
+ if (NullComparison == CR::Top || NonnullComparison == CR::Top)
+ return ComparisonResult::Same;
- if (NullComparison == ComparisonResult::Different ||
- NonnullComparison == ComparisonResult::Different)
+ if (NullComparison == CR::Different ||
+ NonnullComparison == CR::Different)
return ComparisonResult::Different;
- if (NullComparison == ComparisonResult::Unknown ||
- NonnullComparison == ComparisonResult::Unknown)
+ if (NullComparison == CR::Unknown ||
+ NonnullComparison == CR::Unknown)
return ComparisonResult::Unknown;
return ComparisonResult::Same;
>From 0469746036b7b15c7599f36606db14de2fb942a5 Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Mon, 22 Apr 2024 08:33:49 +0000
Subject: [PATCH 08/14] Add support for function calls that take a
pointer-of-pointer argument
---
.../bugprone/null-check-after-dereference.cpp | 16 ++--
.../Models/NullPointerAnalysisModel.cpp | 76 ++++++++++++++++---
2 files changed, 70 insertions(+), 22 deletions(-)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
index e87ba19b25b75..8f3d33370460a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
@@ -168,24 +168,21 @@ int fncall_reassignment(int *fncall_reassigned) {
// CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
*fncall_reassigned = 42;
}
-
- ptr_fn(&fncall_reassigned);
+
+ ref_fn(fncall_reassigned);
if (fncall_reassigned) {
// FIXME: References of a pointer passed to external functions do not invalidate its value
// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked even though it cannot be null at this point
- // CHECK-MESSAGES: :[[@LINE-8]]:5: note: one of the locations where the pointer's value cannot be null
*fncall_reassigned = 42;
}
*fncall_reassigned = 42;
-
- ref_fn(fncall_reassigned);
+
+ ptr_fn(&fncall_reassigned);
if (fncall_reassigned) {
- // FIXME: References of a pointer passed to external functions do not invalidate its value
- // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked even though it cannot be null at this point
- // CHECK-MESSAGES: :[[@LINE-19]]:5: note: one of the locations where the pointer's value cannot be null
+ // no-warning
*fncall_reassigned = 42;
}
@@ -194,8 +191,7 @@ int fncall_reassignment(int *fncall_reassigned) {
if (fncall_reassigned) {
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
- // FIXME: Better note tag support, preferably after the reassignment/refresh
- // CHECK-MESSAGES: :[[@LINE-29]]:5: note: one of the locations where the pointer's value cannot be null
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
*fncall_reassigned = 42;
return *fncall_reassigned;
} else {
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index 344fc5352b7d2..e2fb8890ab097 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -99,8 +99,9 @@ auto addressofMatcher() {
}
auto functionCallMatcher() {
- return callExpr(hasDeclaration(functionDecl(returns(isAnyPointer()))))
- .bind(kVar);
+ return callExpr(
+ callee(functionDecl(hasAnyParameter(anyOf(hasType(pointerType()),
+ hasType(referenceType()))))));
}
auto assignMatcher() {
@@ -362,6 +363,49 @@ void matchAddressofExpr(const Expr *expr,
RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(true));
}
+void matchPtrArgFunctionExpr(const CallExpr *fncall,
+ const MatchFinder::MatchResult &Result,
+ Environment &Env) {
+ // Inner storageloc, inner type
+ fncall->dump();
+
+ for (const auto *Arg : fncall->arguments()) {
+ // WrappedArg->dump();
+ // const auto *Arg = WrappedArg->IgnoreCasts();
+ Arg->dump();
+
+ // FIXME: Add handling for reference types as arguments
+ if (Arg->getType()->isPointerType()) {
+ llvm::errs() << (int)Env.getValue(*Arg)->getKind();
+ PointerValue *OuterValue = cast_or_null<PointerValue>(
+ Env.getValue(*Arg));
+
+ if (!OuterValue)
+ continue;
+
+ QualType InnerType = Arg->getType()->getPointeeType();
+ if (!InnerType->isPointerType())
+ continue;
+
+ StorageLocation &InnerLoc = OuterValue->getPointeeLoc();
+
+ PointerValue *InnerValue =
+ cast_or_null<PointerValue>(Env.getValue(InnerLoc));
+
+ if (!InnerValue)
+ continue;
+
+ Value *NewValue = Env.createValue(InnerType);
+ assert(NewValue && "Failed to re-initialize a pointer's value");
+
+ Env.setValue(InnerLoc, *NewValue);
+
+ // FIXME: Recursively invalidate all member pointers of eg. a struct
+ // Should be part of the framework, most likely.
+ }
+ }
+}
+
void matchAnyPointerExpr(const Expr *fncall,
const MatchFinder::MatchResult &Result,
Environment &Env) {
@@ -502,7 +546,7 @@ auto buildTransferMatchSwitch() {
.CaseOfCFGStmt<Stmt>(arrowMatcher(), matchDereferenceExpr)
.CaseOfCFGStmt<Expr>(nullptrMatcher(), matchNullptrExpr)
.CaseOfCFGStmt<Expr>(addressofMatcher(), matchAddressofExpr)
- .CaseOfCFGStmt<Expr>(functionCallMatcher(), matchAnyPointerExpr)
+ .CaseOfCFGStmt<CallExpr>(functionCallMatcher(), matchPtrArgFunctionExpr)
.CaseOfCFGStmt<Expr>(anyPointerMatcher(), matchAnyPointerExpr)
.CaseOfCFGStmt<Expr>(castExprMatcher(), matchNullCheckExpr)
.CaseOfCFGStmt<Expr>(nullCheckExprMatcher(), matchNullCheckExpr)
@@ -586,24 +630,32 @@ void NullPointerAnalysisModel::join(QualType Type, const Value &Val1,
case SR::False:
return MergedEnv.getBoolLiteralValue(false);
case SR::Unknown:
- if (MergedEnv.proves(MergedEnv.arena().makeEquals(LHSVar->formula(),
- RHSVar->formula())))
- return *LHSVar;
-
- return MergedEnv.makeTopBoolValue();
+ break;
}
}
+ if (LHSVar && RHSVar &&
+ MergedEnv.proves(MergedEnv.arena().makeEquals(LHSVar->formula(),
+ RHSVar->formula()))) {
+ return *LHSVar;
+ }
+
return MergedEnv.makeTopBoolValue();
};
BoolValue &NonnullValue = MergeValues(kIsNonnull);
BoolValue &NullValue = MergeValues(kIsNull);
- MergedVal.setProperty(kIsNonnull, NonnullValue);
- MergedVal.setProperty(kIsNull, NullValue);
-
- MergedEnv.assume(MergedEnv.makeOr(NonnullValue, NullValue).formula());
+ if (&NonnullValue == &MergedEnv.makeTopBoolValue() ||
+ &NullValue == &MergedEnv.makeTopBoolValue()) {
+ MergedVal.setProperty(kIsNonnull, MergedEnv.makeTopBoolValue());
+ MergedVal.setProperty(kIsNull, MergedEnv.makeTopBoolValue());
+ } else {
+ MergedVal.setProperty(kIsNonnull, NonnullValue);
+ MergedVal.setProperty(kIsNull, NullValue);
+
+ MergedEnv.assume(MergedEnv.makeOr(NonnullValue, NullValue).formula());
+ }
}
ComparisonResult NullPointerAnalysisModel::compare(QualType Type,
>From 16e2dbaa3253d8873c27f096fba5c6d4a3ae681d Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Mon, 22 Apr 2024 08:47:23 +0000
Subject: [PATCH 09/14] Remove leftover logs
---
.../FlowSensitive/Models/NullPointerAnalysisModel.cpp | 8 --------
1 file changed, 8 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index e2fb8890ab097..559e3125003fd 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -366,17 +366,9 @@ void matchAddressofExpr(const Expr *expr,
void matchPtrArgFunctionExpr(const CallExpr *fncall,
const MatchFinder::MatchResult &Result,
Environment &Env) {
- // Inner storageloc, inner type
- fncall->dump();
-
for (const auto *Arg : fncall->arguments()) {
- // WrappedArg->dump();
- // const auto *Arg = WrappedArg->IgnoreCasts();
- Arg->dump();
-
// FIXME: Add handling for reference types as arguments
if (Arg->getType()->isPointerType()) {
- llvm::errs() << (int)Env.getValue(*Arg)->getKind();
PointerValue *OuterValue = cast_or_null<PointerValue>(
Env.getValue(*Arg));
>From 7968420c6e12711a4f4b145a826beaf8e14be848 Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Tue, 23 Apr 2024 07:45:16 +0000
Subject: [PATCH 10/14] Fix crashes related to setting gl/prvalues mismatch
---
.../FlowSensitive/Models/NullPointerAnalysisModel.cpp | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index 559e3125003fd..af4e098a85a23 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -224,7 +224,7 @@ Value *getValue(const Expr &Var, Environment &Env) {
initializeRootValue(*RootValue, Env);
- setGLValue(Var, *RootValue, Env);
+ setUnknownValue(Var, *RootValue, Env);
return RootValue;
}
@@ -396,6 +396,15 @@ void matchPtrArgFunctionExpr(const CallExpr *fncall,
// Should be part of the framework, most likely.
}
}
+
+ if (fncall->getCallReturnType(*Result.Context)->isPointerType()) {
+ Value *RootValue = Env.createValue(
+ fncall->getCallReturnType(*Result.Context));
+ if (!RootValue)
+ return;
+
+ setUnknownValue(*fncall, *RootValue, Env);
+ }
}
void matchAnyPointerExpr(const Expr *fncall,
>From 236502564ba76e562eb5b4c70241feed34a97ce9 Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Thu, 2 May 2024 09:23:19 +0000
Subject: [PATCH 11/14] Remove reference to ControlFlowContext
---
.../bugprone/NullCheckAfterDereferenceCheck.cpp | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
index 9a2938d89af19..263ff100ee65f 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
@@ -13,7 +13,6 @@
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Analysis/CFG.h"
-#include "clang/Analysis/FlowSensitive/ControlFlowContext.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
@@ -23,6 +22,7 @@
#include "llvm/ADT/Any.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/Error.h"
+#include <clang/Analysis/FlowSensitive/AdornedCFG.h>
#include <memory>
#include <vector>
@@ -44,7 +44,7 @@ using ExpandedResultType =
static std::optional<ExpandedResultType>
analyzeFunction(const FunctionDecl &FuncDecl) {
- using dataflow::ControlFlowContext;
+ using dataflow::AdornedCFG;
using dataflow::DataflowAnalysisState;
using llvm::Expected;
@@ -54,8 +54,8 @@ analyzeFunction(const FunctionDecl &FuncDecl) {
return std::nullopt;
}
- Expected<ControlFlowContext> Context =
- ControlFlowContext::build(FuncDecl, *FuncDecl.getBody(), ASTCtx);
+ Expected<AdornedCFG> Context =
+ AdornedCFG::build(FuncDecl, *FuncDecl.getBody(), ASTCtx);
if (!Context)
return std::nullopt;
@@ -74,7 +74,7 @@ analyzeFunction(const FunctionDecl &FuncDecl) {
const State &S) mutable -> void {
auto EltDiagnostics = Diagnoser.diagnose(ASTCtx, &Elt, S.Env);
llvm::move(EltDiagnostics.first, std::back_inserter(Diagnostics.first));
- llvm::move(EltDiagnostics.second, std::back_inserter(Diagnostics.second));
+ llvm::move(EltDiagnostics.second, std::back_inserter(Diagnostics.second));
};
Expected<DetailMaybeStates> BlockToOutputState =
>From 8a8808caf507c31ab5d74d5b9a33c27feaef103e Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Mon, 6 May 2024 06:37:31 +0000
Subject: [PATCH 12/14] Fix formatting
---
.../FlowSensitive/Models/NullPointerAnalysisModel.cpp | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index af4e098a85a23..3f4bbd80bc3a8 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -755,10 +755,10 @@ ComparisonResult compareAndReplace(QualType Type, Value &Val1,
return ComparisonResult::Same;
}
-std::optional<WidenResult> NullPointerAnalysisModel::widen(QualType Type, Value &Prev,
- const Environment &PrevEnv,
- Value &Current,
- Environment &CurrentEnv) {
+std::optional<WidenResult>
+NullPointerAnalysisModel::widen(QualType Type, Value &Prev,
+ const Environment &PrevEnv, Value &Current,
+ Environment &CurrentEnv) {
if (!Type->isAnyPointerType())
return std::nullopt;
>From b5a09ac610f2f386b20d30859a7af483de28e8a1 Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Tue, 11 Jun 2024 08:45:18 +0000
Subject: [PATCH 13/14] Better top-bool- and generic-value handling
---
.../bugprone/null-check-after-dereference.cpp | 1 +
.../Models/NullPointerAnalysisModel.cpp | 60 +++++++++++--------
2 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
index 8f3d33370460a..78709b9210f69 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
@@ -156,6 +156,7 @@ int fncall_reassignment(int *fncall_reassigned) {
fncall_reassigned = external_fn();
if (fncall_reassigned) {
+ // no-warning
*fncall_reassigned = 42;
}
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index 3f4bbd80bc3a8..bcf6301b3253e 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -211,6 +211,8 @@ void setUnknownValue(const Expr &E, Value &Val, Environment &Env) {
Env.setValue(E, Val);
}
+// Gets a pointer's value, and initializes it to (Unknown, Unknown) if it hasn't
+// been initialized already.
Value *getValue(const Expr &Var, Environment &Env) {
if (Value *EnvVal = Env.getValue(Var)) {
// FIXME: The framework usually creates the values for us, but without the
@@ -220,13 +222,20 @@ Value *getValue(const Expr &Var, Environment &Env) {
return EnvVal;
}
- Value *RootValue = Env.createValue(Var.getType());
+ return nullptr;
- initializeRootValue(*RootValue, Env);
-
- setUnknownValue(Var, *RootValue, Env);
+ // Value *RootValue = Env.createValue(Var.getType());
+//
+ // initializeRootValue(*RootValue, Env);
+//
+ // setUnknownValue(Var, *RootValue, Env);
+//
+ // return RootValue;
+}
- return RootValue;
+bool hasTopOrNullValue(const Value *Val, const Environment &Env) {
+ return !Val || isa_and_present<TopBoolValue>(Val->getProperty(kIsNull)) ||
+ isa_and_present<TopBoolValue>(Val->getProperty(kIsNonnull));
}
void matchDereferenceExpr(const Stmt *stmt,
@@ -236,12 +245,11 @@ void matchDereferenceExpr(const Stmt *stmt,
assert(Var != nullptr);
Value *RootValue = getValue(*Var, Env);
+ if (hasTopOrNullValue(RootValue, Env))
+ return;
BoolValue &IsNull = getVal(kIsNull, *RootValue);
- if (&IsNull == &Env.makeTopBoolValue())
- return;
-
Env.assume(Env.arena().makeNot(IsNull.formula()));
}
@@ -259,15 +267,13 @@ void matchNullCheckExpr(const Expr *NullCheck,
}
Value *RootValue = getValue(*Var, Env);
+ if (hasTopOrNullValue(RootValue, Env))
+ return;
Arena &A = Env.arena();
BoolValue &IsNonnull = getVal(kIsNonnull, *RootValue);
BoolValue &IsNull = getVal(kIsNull, *RootValue);
- if (&IsNonnull == &Env.makeTopBoolValue() ||
- &IsNull == &Env.makeTopBoolValue())
- return;
-
BoolValue *CondValue = cast_or_null<BoolValue>(Env.getValue(*NullCheck));
if (!CondValue) {
CondValue = &A.makeAtomValue();
@@ -296,17 +302,14 @@ void matchEqualExpr(const BinaryOperator *EqualExpr,
Value *LHSValue = getValue(*LHSVar, Env);
Value *RHSValue = getValue(*RHSVar, Env);
+ if (hasTopOrNullValue(LHSValue, Env) || hasTopOrNullValue(RHSValue, Env))
+ return;
+
BoolValue &LHSNonnull = getVal(kIsNonnull, *LHSValue);
BoolValue &LHSNull = getVal(kIsNull, *LHSValue);
BoolValue &RHSNonnull = getVal(kIsNonnull, *RHSValue);
BoolValue &RHSNull = getVal(kIsNull, *RHSValue);
- if (&LHSNonnull == &Env.makeTopBoolValue() ||
- &RHSNonnull == &Env.makeTopBoolValue() ||
- &LHSNull == &Env.makeTopBoolValue() ||
- &RHSNull == &Env.makeTopBoolValue())
- return;
-
BoolValue *CondValue = cast_or_null<BoolValue>(Env.getValue(*EqualExpr));
if (!CondValue) {
CondValue = &A.makeAtomValue();
@@ -349,6 +352,7 @@ void matchAddressofExpr(const Expr *expr,
const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
assert(Var != nullptr);
+ // FIXME: Use atoms or export to separate function
Value *RootValue = Env.getValue(*Var);
if (!RootValue) {
RootValue = Env.createValue(Var->getType());
@@ -397,7 +401,8 @@ void matchPtrArgFunctionExpr(const CallExpr *fncall,
}
}
- if (fncall->getCallReturnType(*Result.Context)->isPointerType()) {
+ if (fncall->getCallReturnType(*Result.Context)->isPointerType() &&
+ !Env.getValue(*fncall)) {
Value *RootValue = Env.createValue(
fncall->getCallReturnType(*Result.Context));
if (!RootValue)
@@ -415,13 +420,15 @@ void matchAnyPointerExpr(const Expr *fncall,
const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
assert(Var != nullptr);
- // Initialize to (Unknown, Unknown)
+ // In some cases, prvalues are not automatically initialized
+ // Initialize these values, but don't set null-ness values for performance
if (Env.getValue(*Var))
return;
Value *RootValue = Env.createValue(Var->getType());
+ if (!RootValue)
+ return;
- // initializeRootValue(*RootValue, Env);
setUnknownValue(*Var, *RootValue, Env);
}
@@ -527,12 +534,14 @@ diagnoseEqualExpr(const Expr *PtrCheck, const MatchFinder::MatchResult &Result,
std::vector<SourceLocation> NullVarLocations;
if (Value *LHSValue = Env.getValue(*LHSVar);
+ LHSValue->getProperty(kIsNonnull) &&
Env.proves(A.makeNot(getVal(kIsNonnull, *LHSValue).formula()))) {
WarningLocToVal.try_emplace(LHSVar->getBeginLoc(), LHSValue);
NullVarLocations.push_back(LHSVar->getBeginLoc());
}
if (Value *RHSValue = Env.getValue(*RHSVar);
+ RHSValue->getProperty(kIsNonnull) &&
Env.proves(A.makeNot(getVal(kIsNonnull, *RHSValue).formula()))) {
WarningLocToVal.try_emplace(RHSVar->getBeginLoc(), RHSValue);
NullVarLocations.push_back(RHSVar->getBeginLoc());
@@ -647,8 +656,7 @@ void NullPointerAnalysisModel::join(QualType Type, const Value &Val1,
BoolValue &NonnullValue = MergeValues(kIsNonnull);
BoolValue &NullValue = MergeValues(kIsNull);
- if (&NonnullValue == &MergedEnv.makeTopBoolValue() ||
- &NullValue == &MergedEnv.makeTopBoolValue()) {
+ if (isa<TopBoolValue>(NonnullValue) || isa<TopBoolValue>(NullValue)) {
MergedVal.setProperty(kIsNonnull, MergedEnv.makeTopBoolValue());
MergedVal.setProperty(kIsNull, MergedEnv.makeTopBoolValue());
} else {
@@ -673,8 +681,12 @@ ComparisonResult NullPointerAnalysisModel::compare(QualType Type,
auto *LHSVar = cast_or_null<BoolValue>(Val1.getProperty(Name));
auto *RHSVar = cast_or_null<BoolValue>(Val2.getProperty(Name));
+ if (isa_and_present<TopBoolValue>(LHSVar) ||
+ isa_and_present<TopBoolValue>(RHSVar))
+ return CR::Top;
+
if (LHSVar == RHSVar)
- return (LHSVar == &Env1.makeTopBoolValue()) ? CR::Top : CR::Same;
+ return CR::Same;
SatisfiabilityResult LHSResult = computeSatisfiability(LHSVar, Env1);
SatisfiabilityResult RHSResult = computeSatisfiability(RHSVar, Env2);
>From 4058a89c48bb1b42ea7e2a4d899cf6331753096c Mon Sep 17 00:00:00 2001
From: Viktor <viktor.cseh at ericsson.com>
Date: Thu, 4 Jul 2024 12:43:02 +0000
Subject: [PATCH 14/14] Remove TK_IgnoreUnlessSpelledInSource
---
.../Models/NullPointerAnalysisModel.h | 5 ---
.../Models/NullPointerAnalysisModel.cpp | 31 ++-----------------
2 files changed, 2 insertions(+), 34 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
index 64e58780f2d40..754ce316f0697 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
@@ -49,8 +49,6 @@ class NullPointerAnalysisModel
private:
CFGMatchSwitch<Environment> TransferMatchSwitch;
- ASTMatchSwitch<Stmt, TransferArgs> BranchTransferMatchSwitch;
-
public:
explicit NullPointerAnalysisModel(ASTContext &Context);
@@ -60,9 +58,6 @@ class NullPointerAnalysisModel
void transfer(const CFGElement &E, NoopLattice &State, Environment &Env);
- void transferBranch(bool Branch, const Stmt *E, NoopLattice &State,
- Environment &Env);
-
void join(QualType Type, const Value &Val1, const Environment &Env1,
const Value &Val2, const Environment &Env2, Value &MergedVal,
Environment &MergedEnv) override;
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
index bcf6301b3253e..e09b3edefb8b8 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
@@ -72,8 +72,7 @@ using CR = CompareResult;
// FIXME: These AST matchers should also be exported via the
// NullPointerAnalysisModel class, for tests
auto ptrWithBinding(llvm::StringRef VarName = kVar) {
- return traverse(TK_IgnoreUnlessSpelledInSource,
- expr(hasType(isAnyPointer())).bind(VarName));
+ return expr(hasType(isAnyPointer())).bind(VarName);
}
auto derefMatcher() {
@@ -223,14 +222,6 @@ Value *getValue(const Expr &Var, Environment &Env) {
}
return nullptr;
-
- // Value *RootValue = Env.createValue(Var.getType());
-//
- // initializeRootValue(*RootValue, Env);
-//
- // setUnknownValue(Var, *RootValue, Env);
-//
- // return RootValue;
}
bool hasTopOrNullValue(const Value *Val, const Environment &Env) {
@@ -564,13 +555,6 @@ auto buildTransferMatchSwitch() {
.Build();
}
-auto buildBranchTransferMatchSwitch() {
- return ASTMatchSwitchBuilder<Stmt, NullPointerAnalysisModel::TransferArgs>()
- // .CaseOf<CastExpr>(castExprMatcher(), matchNullCheckExpr)
- // .CaseOf<BinaryOperator>(equalExprMatcher(), matchEqualExpr)
- .Build();
-}
-
auto buildDiagnoseMatchSwitch() {
return CFGMatchSwitchBuilder<NullCheckAfterDereferenceDiagnoser::DiagnoseArgs,
NullCheckAfterDereferenceDiagnoser::ResultType>()
@@ -587,8 +571,7 @@ auto buildDiagnoseMatchSwitch() {
NullPointerAnalysisModel::NullPointerAnalysisModel(ASTContext &Context)
: DataflowAnalysis<NullPointerAnalysisModel, NoopLattice>(Context),
- TransferMatchSwitch(buildTransferMatchSwitch()),
- BranchTransferMatchSwitch(buildBranchTransferMatchSwitch()) {}
+ TransferMatchSwitch(buildTransferMatchSwitch()) {}
ast_matchers::StatementMatcher NullPointerAnalysisModel::ptrValueMatcher() {
return ptrWithBinding();
@@ -599,16 +582,6 @@ void NullPointerAnalysisModel::transfer(const CFGElement &E, NoopLattice &State,
TransferMatchSwitch(E, getASTContext(), Env);
}
-void NullPointerAnalysisModel::transferBranch(bool Branch, const Stmt *E,
- NoopLattice &State,
- Environment &Env) {
- if (!E)
- return;
-
- TransferArgs Args = {Branch, Env};
- BranchTransferMatchSwitch(*E, getASTContext(), Args);
-}
-
void NullPointerAnalysisModel::join(QualType Type, const Value &Val1,
const Environment &Env1, const Value &Val2,
const Environment &Env2, Value &MergedVal,
More information about the cfe-commits
mailing list