From 4a8e0df6e073e0f159778925e079aea0621c30b1 Mon Sep 17 00:00:00 2001 From: Viktor Date: Wed, 6 Mar 2024 14:10:44 +0000 Subject: [PATCH 01/19] [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 | 5 +- .../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, 1778 insertions(+), 2 deletions(-) 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 33ac65e715ce8..5111bc294efcd 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -51,6 +51,7 @@ #include "NonZeroEnumToBoolConversionCheck.h" #include "NondeterministicPointerIterationOrderCheck.h" #include "NotNullTerminatedResultCheck.h" +#include "NullCheckAfterDereferenceCheck.h" #include "OptionalValueConversionCheck.h" #include "ParentVirtualCallCheck.h" #include "PointerArithmeticOnPolymorphicObjectCheck.h" @@ -195,6 +196,8 @@ class BugproneModule : public ClangTidyModule { CheckFactories.registerCheck("bugprone-posix-return"); CheckFactories.registerCheck( "bugprone-reserved-identifier"); + CheckFactories.registerCheck( + "bugprone-null-check-after-dereference"); CheckFactories.registerCheck( "bugprone-shared-ptr-array-mismatch"); CheckFactories.registerCheck("bugprone-signal-handler"); diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 13adad7c3dadb..d2d7ac9aa8e9b 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -46,6 +46,7 @@ add_clang_library(clangTidyBugproneModule STATIC NonZeroEnumToBoolConversionCheck.cpp NondeterministicPointerIterationOrderCheck.cpp NotNullTerminatedResultCheck.cpp + NullCheckAfterDereferenceCheck.cpp OptionalValueConversionCheck.cpp ParentVirtualCallCheck.cpp PointerArithmeticOnPolymorphicObjectCheck.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 +#include + +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 DerefLoc; +}; + +using ExpandedResultType = + std::pair, std::vector>; + +static std::optional +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 Context = + ControlFlowContext::build(FuncDecl, *FuncDecl.getBody(), ASTCtx); + if (!Context) + return std::nullopt; + + dataflow::DataflowAnalysisContext AnalysisContext( + std::make_unique()); + dataflow::Environment Env(AnalysisContext, FuncDecl); + NullPointerAnalysisModel Analysis(ASTCtx); + NullCheckAfterDereferenceDiagnoser Diagnoser; + NullCheckAfterDereferenceDiagnoser::ResultType Diagnostics; + + using LatticeState = DataflowAnalysisState; + using DetailMaybeLatticeStates = std::vector>; + + 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 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(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 2ac123246a4cb..97ad00f795181 100644 --- a/clang-tools-extra/clangd/TidyProvider.cpp +++ b/clang-tools-extra/clangd/TidyProvider.cpp @@ -219,9 +219,10 @@ TidyProvider disableUnusableChecks(llvm::ArrayRef 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-unchecked-optional-access", + "-bugprone-null-check-after-dereference"); size_t Size = BadChecks.size(); for (const std::string &Str : ExtraBadChecks) { 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 { +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 TransferMatchSwitch; + ASTMatchSwitch 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 &ValToDerefLoc; + llvm::DenseMap &WarningLocToVal; + const Environment &Env; + }; + + using ResultType = + std::pair, std::vector>; + + // Maps a pointer's Value to a dereference, null-assignment, etc. + // This is used later to construct the Note tag. + llvm::DenseMap ValToDerefLoc; + // Maps Maps a warning's SourceLocation to its relevant Value. + llvm::DenseMap WarningLocToVal; + +private: + CFGMatchSwitch 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(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(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(RootValue.getProperty(kIsNull)); + BoolValue *IsNonnull = + cast_or_null(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(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(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(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(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(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(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(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(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() + .CaseOfCFGStmt(derefMatcher(), matchDereferenceExpr) + .CaseOfCFGStmt(arrowMatcher(), matchDereferenceExpr) + .CaseOfCFGStmt(nullptrMatcher(), matchNullptrExpr) + .CaseOfCFGStmt(addressofMatcher(), matchAddressofExpr) + .CaseOfCFGStmt(functionCallMatcher(), matchAnyPointerExpr) + .CaseOfCFGStmt(anyPointerMatcher(), matchAnyPointerExpr) + .Build(); +} + +auto buildBranchTransferMatchSwitch() { + return ASTMatchSwitchBuilder() + .CaseOf(castExprMatcher(), matchCastExpr) + .Build(); +} + +auto buildDiagnoseMatchSwitch() { + return CFGMatchSwitchBuilder() + .CaseOfCFGStmt(derefMatcher(), diagnoseDerefLocation) + .CaseOfCFGStmt(arrowMatcher(), diagnoseDerefLocation) + .CaseOfCFGStmt(assignMatcher(), diagnoseAssignLocation) + .CaseOfCFGStmt(castExprMatcher(), diagnoseCastExpr) + .Build(); +} + +} // namespace + +NullPointerAnalysisModel::NullPointerAnalysisModel(ASTContext &Context) + : DataflowAnalysis(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(Val1.getProperty(Name)); + BoolValue *RHSVar = cast_or_null(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(Val1.getProperty(Name)); + BoolValue *RHSVar = cast_or_null(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(Val1.getProperty(Name)); + BoolValue *RHSVar = cast_or_null(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 4e1819bfa166a..9278c61eaa9a5 100644 --- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt +++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt @@ -17,6 +17,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 +#include +#include +#include +#include + +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(arg.first->getProperty(kIsNonnull)), + *arg.second) == nonnull && + checkNullabilityState( + cast_or_null(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>( + checks, negation)) + .str()) { + auto MatchResults = match(functionDecl(hasDescendant(nameToVar(name))), + *output.Target, output.ASTCtx); + assert(!MatchResults.empty()); + + const auto *pointerExpr = MatchResults[0].template getNodeAs(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 +void ExpectDataflowResult(llvm::StringRef Code, MatcherFactory Expectations) { + ASSERT_THAT_ERROR( + checkDataflow( + AnalysisInputs( + Code, hasName("fun"), + [](ASTContext &C, Environment &Env) { + return NullPointerAnalysisModel(C); + }) + .withASTBuildArgs({"-fsyntax-only", "-std=c++17"}), + /*VerifyResults=*/ + [&Expectations](const llvm::StringMap> &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 ba2f02d6fc94e4a28c45fefb09dde77a354e53bc Mon Sep 17 00:00:00 2001 From: Viktor Date: Mon, 11 Mar 2024 13:52:24 +0000 Subject: [PATCH 02/19] 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; - using DetailMaybeLatticeStates = std::vector>; + using State = DataflowAnalysisState; + using DetailMaybeStates = std::vector>; 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 BlockToOutputState = + Expected 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 6803842106791..ffdb4cf045a70 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -148,6 +148,12 @@ New checks Finds cases when an uninstantiated virtual member function in a template class causes cross-compiler incompatibility. +- New :doc:`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>; 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(RootValue.getProperty(kIsNull)); - BoolValue *IsNonnull = - cast_or_null(RootValue.getProperty(kIsNonnull)); + auto *IsNull = cast_or_null(RootValue.getProperty(kIsNull)); + auto *IsNonnull = cast_or_null(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(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(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(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(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(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(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(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(Val1.getProperty(Name)); - BoolValue *RHSVar = cast_or_null(Val2.getProperty(Name)); + auto *LHSVar = cast_or_null(Val1.getProperty(Name)); + auto *RHSVar = cast_or_null(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(Val1.getProperty(Name)); - BoolValue *RHSVar = cast_or_null(Val2.getProperty(Name)); + auto *LHSVar = cast_or_null(Val1.getProperty(Name)); + auto *RHSVar = cast_or_null(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(Val1.getProperty(Name)); - BoolValue *RHSVar = cast_or_null(Val2.getProperty(Name)); + auto *LHSVar = cast_or_null(Val1.getProperty(Name)); + auto *RHSVar = cast_or_null(Val2.getProperty(Name)); SatisfiabilityResult LHSResult = shallowComputeSatisfiability(LHSVar, Env1); SatisfiabilityResult RHSResult = shallowComputeSatisfiability(RHSVar, Env2); From 68a903e6fbef3f696e78a58afa0d9d03d7a9cb17 Mon Sep 17 00:00:00 2001 From: Viktor Date: Mon, 11 Mar 2024 14:04:14 +0000 Subject: [PATCH 03/19] 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 142c79cdab838281f27e729421f482803dfbc63b Mon Sep 17 00:00:00 2001 From: Viktor Date: Mon, 8 Apr 2024 08:29:08 +0000 Subject: [PATCH 04/19] 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 ffdb4cf045a70..21bf754da23ae 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -130,6 +130,12 @@ New checks Finds nondeterministic usages of pointers in unordered containers. +- New :doc:`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:`bugprone-tagged-union-member-count ` check. @@ -148,12 +154,6 @@ New checks Finds cases when an uninstantiated virtual member function in a template class causes cross-compiler incompatibility. -- New :doc:`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>; 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(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(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(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(kVar); + assert(LHSVar != nullptr); - if (IsNonnullBranch) { - Env.assume(A.makeNot(IsNull.formula())); - NewRootValue->setProperty(kIsNull, Env.getBoolLiteralValue(false)); + const auto *RHSVar = Result.Nodes.getNodeAs(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(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(kVar); - assert(PrVar != nullptr); + const auto *Var = Result.Nodes.getNodeAs(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(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(kVar); + assert(LHSVar != nullptr); + const auto *RHSVar = Result.Nodes.getNodeAs(kValue); + assert(RHSVar != nullptr); + + Arena &A = Env.arena(); + std::vector 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() .CaseOfCFGStmt(derefMatcher(), matchDereferenceExpr) @@ -388,12 +476,16 @@ auto buildTransferMatchSwitch() { .CaseOfCFGStmt(addressofMatcher(), matchAddressofExpr) .CaseOfCFGStmt(functionCallMatcher(), matchAnyPointerExpr) .CaseOfCFGStmt(anyPointerMatcher(), matchAnyPointerExpr) + .CaseOfCFGStmt(castExprMatcher(), matchNullCheckExpr) + .CaseOfCFGStmt(nullCheckExprMatcher(), matchNullCheckExpr) + .CaseOfCFGStmt(equalExprMatcher(), matchEqualExpr) .Build(); } auto buildBranchTransferMatchSwitch() { return ASTMatchSwitchBuilder() - .CaseOf(castExprMatcher(), matchCastExpr) + // .CaseOf(castExprMatcher(), matchNullCheckExpr) + // .CaseOf(equalExprMatcher(), matchEqualExpr) .Build(); } @@ -403,7 +495,9 @@ auto buildDiagnoseMatchSwitch() { .CaseOfCFGStmt(derefMatcher(), diagnoseDerefLocation) .CaseOfCFGStmt(arrowMatcher(), diagnoseDerefLocation) .CaseOfCFGStmt(assignMatcher(), diagnoseAssignLocation) - .CaseOfCFGStmt(castExprMatcher(), diagnoseCastExpr) + .CaseOfCFGStmt(castExprMatcher(), diagnoseNullCheckExpr) + .CaseOfCFGStmt(nullCheckExprMatcher(), diagnoseNullCheckExpr) + .CaseOfCFGStmt(equalExprMatcher(), diagnoseEqualExpr) .Build(); } From af54b316f6242e98992f9cc3467e5fd0dea452b1 Mon Sep 17 00:00:00 2001 From: Viktor Date: Mon, 8 Apr 2024 08:51:30 +0000 Subject: [PATCH 05/19] 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 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 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 672223e043427b264cdd77c261de67f811bd6d0f Mon Sep 17 00:00:00 2001 From: Viktor Date: Wed, 10 Apr 2024 06:47:38 +0000 Subject: [PATCH 06/19] 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 widen(QualType Type, Value &Prev, const Environment &PrevEnv, - Value &Current, Environment &CurrentEnv) override; + std::optional widen(QualType Type, Value &Prev, + const Environment &PrevEnv, Value &Current, + Environment &CurrentEnv) override; }; class NullCheckAfterDereferenceDiagnoser { From 268e913dc7ad975583265882512b03e8ffefa9de Mon Sep 17 00:00:00 2001 From: Viktor Date: Wed, 10 Apr 2024 08:04:35 +0000 Subject: [PATCH 07/19] 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(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(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(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(Val1.getProperty(Name)); auto *RHSVar = cast_or_null(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(Val1.getProperty(Name)); auto *RHSVar = cast_or_null(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 1c93b73b4851d55ffe345a13b93a4052df2974a3 Mon Sep 17 00:00:00 2001 From: Viktor Date: Mon, 22 Apr 2024 08:33:49 +0000 Subject: [PATCH 08/19] 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( + Env.getValue(*Arg)); + + if (!OuterValue) + continue; + + QualType InnerType = Arg->getType()->getPointeeType(); + if (!InnerType->isPointerType()) + continue; + + StorageLocation &InnerLoc = OuterValue->getPointeeLoc(); + + PointerValue *InnerValue = + cast_or_null(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(arrowMatcher(), matchDereferenceExpr) .CaseOfCFGStmt(nullptrMatcher(), matchNullptrExpr) .CaseOfCFGStmt(addressofMatcher(), matchAddressofExpr) - .CaseOfCFGStmt(functionCallMatcher(), matchAnyPointerExpr) + .CaseOfCFGStmt(functionCallMatcher(), matchPtrArgFunctionExpr) .CaseOfCFGStmt(anyPointerMatcher(), matchAnyPointerExpr) .CaseOfCFGStmt(castExprMatcher(), matchNullCheckExpr) .CaseOfCFGStmt(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 dd150a994ce6c34918f36ac111e68fb38f957c7f Mon Sep 17 00:00:00 2001 From: Viktor Date: Mon, 22 Apr 2024 08:47:23 +0000 Subject: [PATCH 09/19] 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( Env.getValue(*Arg)); From 32cfd3179792b4bad24e2b9c07b5a8edef84cf75 Mon Sep 17 00:00:00 2001 From: Viktor Date: Tue, 23 Apr 2024 07:45:16 +0000 Subject: [PATCH 10/19] 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 a5bb2d4222d622d2a760c215c3a5a4069595edd1 Mon Sep 17 00:00:00 2001 From: Viktor Date: Thu, 2 May 2024 09:23:19 +0000 Subject: [PATCH 11/19] 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 #include #include @@ -44,7 +44,7 @@ using ExpandedResultType = static std::optional 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 Context = - ControlFlowContext::build(FuncDecl, *FuncDecl.getBody(), ASTCtx); + Expected 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 BlockToOutputState = From 0bdaa4039074f7206cece0fd7dca2d63178dd911 Mon Sep 17 00:00:00 2001 From: Viktor Date: Mon, 6 May 2024 06:37:31 +0000 Subject: [PATCH 12/19] 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 NullPointerAnalysisModel::widen(QualType Type, Value &Prev, - const Environment &PrevEnv, - Value &Current, - Environment &CurrentEnv) { +std::optional +NullPointerAnalysisModel::widen(QualType Type, Value &Prev, + const Environment &PrevEnv, Value &Current, + Environment &CurrentEnv) { if (!Type->isAnyPointerType()) return std::nullopt; From ca00002b85042278245af16eb39e6530011d223a Mon Sep 17 00:00:00 2001 From: Viktor Date: Tue, 11 Jun 2024 08:45:18 +0000 Subject: [PATCH 13/19] 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(Val->getProperty(kIsNull)) || + isa_and_present(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(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(Env.getValue(*EqualExpr)); if (!CondValue) { CondValue = &A.makeAtomValue(); @@ -349,6 +352,7 @@ void matchAddressofExpr(const Expr *expr, const auto *Var = Result.Nodes.getNodeAs(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(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 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(NonnullValue) || isa(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(Val1.getProperty(Name)); auto *RHSVar = cast_or_null(Val2.getProperty(Name)); + if (isa_and_present(LHSVar) || + isa_and_present(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 bf9f79ac52cd555f13e1cf1367962b9c609c4c5a Mon Sep 17 00:00:00 2001 From: Viktor Date: Thu, 4 Jul 2024 12:43:02 +0000 Subject: [PATCH 14/19] 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 TransferMatchSwitch; - ASTMatchSwitch 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() - // .CaseOf(castExprMatcher(), matchNullCheckExpr) - // .CaseOf(equalExprMatcher(), matchEqualExpr) - .Build(); -} - auto buildDiagnoseMatchSwitch() { return CFGMatchSwitchBuilder() @@ -587,8 +571,7 @@ auto buildDiagnoseMatchSwitch() { NullPointerAnalysisModel::NullPointerAnalysisModel(ASTContext &Context) : DataflowAnalysis(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, From 32e516d236c27a40b8aaa43e543b331c9ec0f730 Mon Sep 17 00:00:00 2001 From: Viktor Date: Tue, 17 Sep 2024 04:15:33 +0000 Subject: [PATCH 15/19] Use new caller interface for runDataflowAnalysis --- .../NullCheckAfterDereferenceCheck.cpp | 97 ++++++++----------- .../Models/NullPointerAnalysisModel.h | 13 ++- .../Models/NullPointerAnalysisModel.cpp | 41 ++++---- 3 files changed, 68 insertions(+), 83 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp index 263ff100ee65f..41f14b44bff38 100644 --- a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp @@ -31,16 +31,16 @@ namespace clang::tidy::bugprone { using ast_matchers::MatchFinder; using dataflow::NullCheckAfterDereferenceDiagnoser; using dataflow::NullPointerAnalysisModel; +using Diagnoser = NullCheckAfterDereferenceDiagnoser; static constexpr llvm::StringLiteral FuncID("fun"); struct ExpandedResult { - SourceLocation WarningLoc; + Diagnoser::DiagnosticEntry Entry; std::optional DerefLoc; }; -using ExpandedResultType = - std::pair, std::vector>; +using ExpandedResultType = llvm::SmallVector; static std::optional analyzeFunction(const FunctionDecl &FuncDecl) { @@ -63,24 +63,14 @@ analyzeFunction(const FunctionDecl &FuncDecl) { std::make_unique()); dataflow::Environment Env(AnalysisContext, FuncDecl); NullPointerAnalysisModel Analysis(ASTCtx); - NullCheckAfterDereferenceDiagnoser Diagnoser; - NullCheckAfterDereferenceDiagnoser::ResultType Diagnostics; + Diagnoser Diagnoser; - using State = DataflowAnalysisState; - using DetailMaybeStates = std::vector>; + Expected Diagnostics = + dataflow::diagnoseFunction( + FuncDecl, ASTCtx, Diagnoser); - auto DiagnoserImpl = [&ASTCtx, &Diagnoser, - &Diagnostics](const CFGElement &Elt, - 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 BlockToOutputState = - dataflow::runDataflowAnalysis(*Context, Analysis, Env, DiagnoserImpl); - - if (llvm::Error E = BlockToOutputState.takeError()) { + + if (llvm::Error E = Diagnostics.takeError()) { llvm::dbgs() << "Dataflow analysis failed: " << llvm::toString(std::move(E)) << ".\n"; return std::nullopt; @@ -88,26 +78,15 @@ analyzeFunction(const FunctionDecl &FuncDecl) { 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]; + llvm::transform(*Diagnostics, + std::back_inserter(ExpandedDiagnostics), + [&](Diagnoser::DiagnosticEntry Entry) -> ExpandedResult { + if (auto Val = Diagnoser.WarningLocToVal[Entry.Location]; auto DerefExpr = Diagnoser.ValToDerefLoc[Val]) { - return {WarningLoc, DerefExpr->getBeginLoc()}; + return {Entry, DerefExpr->getBeginLoc()}; } - return {WarningLoc, std::nullopt}; + return {Entry, std::nullopt}; }); return ExpandedDiagnostics; @@ -143,28 +122,30 @@ void NullCheckAfterDereferenceCheck::check( return; if (const auto Diagnostics = analyzeFunction(*FuncDecl)) { - const auto &[CheckWhenNullLocations, CheckWhenNonnullLocations] = - *Diagnostics; - - for (const auto [WarningLoc, DerefLoc] : CheckWhenNonnullLocations) { - 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); + for (const auto [Entry, DerefLoc] : *Diagnostics) { + const auto [WarningLoc, Type] = Entry; + + switch (Type) { + case Diagnoser::DiagnosticType::CheckAfterDeref: + 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); + } + break; + case Diagnoser::DiagnosticType::CheckWhenNull: + 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); + } + break; } } } diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h index 754ce316f0697..d52a5062f60b2 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h @@ -81,8 +81,13 @@ class NullCheckAfterDereferenceDiagnoser { /// 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>; + enum class DiagnosticType { CheckWhenNull, CheckAfterDeref }; + + struct DiagnosticEntry { + SourceLocation Location; + DiagnosticType Type; + }; + using ResultType = llvm::SmallVector; // Maps a pointer's Value to a dereference, null-assignment, etc. // This is used later to construct the Note tag. @@ -96,8 +101,8 @@ class NullCheckAfterDereferenceDiagnoser { public: NullCheckAfterDereferenceDiagnoser(); - ResultType diagnose(ASTContext &Ctx, const CFGElement *Elt, - const Environment &Env); + ResultType operator()(const CFGElement &Elt, ASTContext &Ctx, + const TransferStateForDiagnostics &State); }; } // namespace clang::dataflow diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp index e09b3edefb8b8..52a048c104ac6 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp @@ -36,6 +36,7 @@ namespace clang::dataflow { namespace { using namespace ast_matchers; +using Diagnoser = NullCheckAfterDereferenceDiagnoser; constexpr char kCond[] = "condition"; constexpr char kVar[] = "var"; @@ -423,9 +424,9 @@ void matchAnyPointerExpr(const Expr *fncall, setUnknownValue(*Var, *RootValue, Env); } -NullCheckAfterDereferenceDiagnoser::ResultType +Diagnoser::ResultType diagnoseDerefLocation(const Expr *Deref, const MatchFinder::MatchResult &Result, - NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) { + Diagnoser::DiagnoseArgs &Data) { auto [ValToDerefLoc, WarningLocToVal, Env] = Data; const auto *Var = Result.Nodes.getNodeAs(kVar); @@ -444,10 +445,9 @@ diagnoseDerefLocation(const Expr *Deref, const MatchFinder::MatchResult &Result, return {}; } -NullCheckAfterDereferenceDiagnoser::ResultType -diagnoseAssignLocation(const Expr *Assign, - const MatchFinder::MatchResult &Result, - NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) { +Diagnoser::ResultType diagnoseAssignLocation(const Expr *Assign, + const MatchFinder::MatchResult &Result, + Diagnoser::DiagnoseArgs &Data) { auto [ValToDerefLoc, WarningLocToVal, Env] = Data; const auto *RHSVar = Result.Nodes.getNodeAs(kValue); @@ -468,7 +468,7 @@ diagnoseAssignLocation(const Expr *Assign, NullCheckAfterDereferenceDiagnoser::ResultType diagnoseNullCheckExpr(const Expr *NullCheck, const MatchFinder::MatchResult &Result, - const NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) { + const Diagnoser::DiagnoseArgs &Data) { auto [ValToDerefLoc, WarningLocToVal, Env] = Data; const auto *Var = Result.Nodes.getNodeAs(kVar); @@ -488,7 +488,7 @@ diagnoseNullCheckExpr(const Expr *NullCheck, assert(Inserted && "multiple warnings at the same source location"); (void)Inserted; - return {{}, {Var->getBeginLoc()}}; + return {{Var->getBeginLoc(), Diagnoser::DiagnosticType::CheckAfterDeref}}; } if (IsNull && !IsNonnull) { @@ -497,7 +497,7 @@ diagnoseNullCheckExpr(const Expr *NullCheck, assert(Inserted && "multiple warnings at the same source location"); (void)Inserted; - return {{Var->getBeginLoc()}, {}}; + return {{Var->getBeginLoc(), Diagnoser::DiagnosticType::CheckWhenNull}}; } } @@ -513,7 +513,7 @@ diagnoseNullCheckExpr(const Expr *NullCheck, NullCheckAfterDereferenceDiagnoser::ResultType diagnoseEqualExpr(const Expr *PtrCheck, const MatchFinder::MatchResult &Result, - NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) { + Diagnoser::DiagnoseArgs &Data) { auto [ValToDerefLoc, WarningLocToVal, Env] = Data; const auto *LHSVar = Result.Nodes.getNodeAs(kVar); @@ -522,23 +522,23 @@ diagnoseEqualExpr(const Expr *PtrCheck, const MatchFinder::MatchResult &Result, assert(RHSVar != nullptr); Arena &A = Env.arena(); - std::vector NullVarLocations; + llvm::SmallVector 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()); + NullVarLocations.push_back({LHSVar->getBeginLoc(), Diagnoser::DiagnosticType::CheckWhenNull}); } 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()); + NullVarLocations.push_back({RHSVar->getBeginLoc(), Diagnoser::DiagnosticType::CheckWhenNull}); } - return {NullVarLocations, {}}; + return NullVarLocations; } auto buildTransferMatchSwitch() { @@ -556,8 +556,7 @@ auto buildTransferMatchSwitch() { } auto buildDiagnoseMatchSwitch() { - return CFGMatchSwitchBuilder() + return CFGMatchSwitchBuilder() .CaseOfCFGStmt(derefMatcher(), diagnoseDerefLocation) .CaseOfCFGStmt(arrowMatcher(), diagnoseDerefLocation) .CaseOfCFGStmt(assignMatcher(), diagnoseAssignLocation) @@ -761,11 +760,11 @@ 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); +NullCheckAfterDereferenceDiagnoser::operator()( + const CFGElement &Elt, ASTContext &Ctx, + const TransferStateForDiagnostics &State) { + DiagnoseArgs Args = {ValToDerefLoc, WarningLocToVal, State.Env}; + return DiagnoseMatchSwitch(Elt, Ctx, Args); } } // namespace clang::dataflow From 79075e06a26d62e769d8f0a530cabc80f970c742 Mon Sep 17 00:00:00 2001 From: Viktor Date: Wed, 2 Apr 2025 13:23:55 +0000 Subject: [PATCH 16/19] Simplify join logic, add some proper unit tests --- .../Models/NullPointerAnalysisModel.cpp | 62 +++-- .../NullPointerAnalysisModelTest.cpp | 248 ++++++++---------- 2 files changed, 143 insertions(+), 167 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp index 52a048c104ac6..200a9e5ddddc7 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp @@ -592,37 +592,55 @@ void NullPointerAnalysisModel::join(QualType Type, const Value &Val1, auto *LHSVar = cast_or_null(Val1.getProperty(Name)); auto *RHSVar = cast_or_null(Val2.getProperty(Name)); - if (LHSVar == RHSVar) - return LHSVar ? *LHSVar : MergedEnv.makeAtomicBoolValue(); - - 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) { + const auto SimplifyVar = [&](BoolValue *VarToSimplify, + const Environment &Env) -> BoolValue * { + SatisfiabilityResult SatResult = + computeSatisfiability(VarToSimplify, Env); + switch (SatResult) { case SR::Nullptr: - return MergedEnv.makeAtomicBoolValue(); + return nullptr; case SR::Top: - return *LHSVar; + return &MergedEnv.makeTopBoolValue(); case SR::True: - return MergedEnv.getBoolLiteralValue(true); + return &MergedEnv.getBoolLiteralValue(true); case SR::False: - return MergedEnv.getBoolLiteralValue(false); + return &MergedEnv.getBoolLiteralValue(false); case SR::Unknown: - break; + return VarToSimplify; } - } + }; + + LHSVar = SimplifyVar(LHSVar, Env1); + LHSVar = SimplifyVar(RHSVar, Env2); - if (LHSVar && RHSVar && - MergedEnv.proves(MergedEnv.arena().makeEquals(LHSVar->formula(), - RHSVar->formula()))) { + // Handle special cases. + if (LHSVar == RHSVar) + return LHSVar ? *LHSVar : MergedEnv.makeAtomicBoolValue(); + else if (isa(LHSVar) || isa(RHSVar)) + return MergedEnv.makeTopBoolValue(); + + if (!LHSVar) + LHSVar = &MergedEnv.makeAtomicBoolValue(); + + if (!RHSVar) + RHSVar = &MergedEnv.makeAtomicBoolValue(); + + assert(LHSVar != nullptr && RHSVar != nullptr); + + if (MergedEnv.proves( + MergedEnv.arena().makeEquals(LHSVar->formula(), RHSVar->formula()))) return *LHSVar; - } - return MergedEnv.makeTopBoolValue(); + BoolValue &ReturnVar = MergedEnv.makeAtomicBoolValue(); + Arena &A = MergedEnv.arena(); + + MergedEnv.assume(A.makeOr( + A.makeAnd(A.makeAtomRef(Env1.getFlowConditionToken()), + A.makeEquals(ReturnVar.formula(), LHSVar->formula())), + A.makeAnd(A.makeAtomRef(Env2.getFlowConditionToken()), + A.makeEquals(ReturnVar.formula(), RHSVar->formula())))); + + return ReturnVar; }; BoolValue &NonnullValue = MergeValues(kIsNonnull); diff --git a/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp index d63430de2a15e..7480c5b5f4292 100644 --- a/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/NullPointerAnalysisModelTest.cpp @@ -87,18 +87,12 @@ std::string checkNullabilityState(BoolValue *value, const Environment &Env) { } } -// 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 namespace test; using ::llvm::IsStringMapEntry; +using ::testing::AllOf; using ::testing::Args; +using ::testing::IsSupersetOf; +using ::testing::NotNull; using ::testing::Pair; using ::testing::UnorderedElementsAre; @@ -119,8 +113,9 @@ MATCHER_P3(HoldsVariable, name, output, checks, ::testing::DescribeMatcher>( checks, negation)) .str()) { - auto MatchResults = match(functionDecl(hasDescendant(nameToVar(name))), - *output.Target, output.ASTCtx); + auto MatchResults = + match(functionDecl(hasDescendant(namedDecl(hasName(name)).bind(kVar))), + *output.Target, output.ASTCtx); assert(!MatchResults.empty()); const auto *pointerExpr = MatchResults[0].template getNodeAs(kVar); @@ -136,49 +131,57 @@ MATCHER_P3(HoldsVariable, name, output, checks, result_listener); } +void RunDataflowAnalysis( + llvm::StringRef Code, + std::function> &, + const AnalysisOutputs &)> + VerifyResults) { + ASSERT_THAT_ERROR(checkDataflow( + AnalysisInputs( + Code, hasName("fun"), + [](ASTContext &C, Environment &Env) { + return NullPointerAnalysisModel(C); + }) + .withASTBuildArgs({"-fsyntax-only", "-std=c++17"}), + VerifyResults), + llvm::Succeeded()); +} + template void ExpectDataflowResult(llvm::StringRef Code, MatcherFactory Expectations) { - ASSERT_THAT_ERROR( - checkDataflow( - AnalysisInputs( - Code, hasName("fun"), - [](ASTContext &C, Environment &Env) { - return NullPointerAnalysisModel(C); - }) - .withASTBuildArgs({"-fsyntax-only", "-std=c++17"}), - /*VerifyResults=*/ - [&Expectations](const llvm::StringMap> &Results, - const AnalysisOutputs &Output) { - EXPECT_THAT(Results, Expectations(Output)); - }), - llvm::Succeeded()); + RunDataflowAnalysis( + Code, + /*VerifyResults=*/ + [&Expectations](const llvm::StringMap> &Results, + const AnalysisOutputs &Output) { + EXPECT_THAT(Results, Expectations(Output)); + }); } -TEST(NullCheckAfterDereferenceTest, DereferenceTypes) { +TEST(NullCheckAfterDereferenceTest, Operations) { std::string Code = R"( struct S { int a; }; - void fun(int *p, S *q) { - *p = 0; // [[p]] - - q->a = 20; // [[q]] + void fun(int *Deref, S *Arrow) { + *Deref = 0; + Arrow->a = 20; + // [[p]] } )"; ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto { - return UnorderedElementsAre( - IsStringMapEntry( - "p", HoldsVariable("p", Output, - HasNullabilityState(kBoolFalse, kBoolTrue))), - IsStringMapEntry( - "q", HoldsVariable("q", Output, - HasNullabilityState(kBoolFalse, kBoolTrue)))); + return UnorderedElementsAre(IsStringMapEntry( + "p", AllOf(HoldsVariable("Deref", Output, + HasNullabilityState(kBoolFalse, kBoolTrue)), + HoldsVariable("Arrow", Output, + HasNullabilityState(kBoolFalse, kBoolTrue))))); }); } -TEST(NullCheckAfterDereferenceTest, ConditionalTypes) { +TEST(NullCheckAfterDereferenceTest, Conditional) { std::string Code = R"( void fun(int *p) { if (p) { @@ -187,7 +190,7 @@ TEST(NullCheckAfterDereferenceTest, ConditionalTypes) { (void)0; // [[p_false]] } - // FIXME: Test ternary op + (void)0; // [[p_merge]] } )"; ExpectDataflowResult(Code, [](const AnalysisOutputs &Output) -> auto { @@ -197,134 +200,89 @@ TEST(NullCheckAfterDereferenceTest, ConditionalTypes) { 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 + kBoolTrue, kBoolFalse))), IsStringMapEntry( - "b_p_false", + "p_merge", HoldsVariable("p", Output, - HasNullabilityState(kBoolInvalid, kBoolInvalid)))); + HasNullabilityState(kBoolUnknown, kBoolUnknown)))); }); } -TEST(NullCheckAfterDereferenceTest, AssignmentOfCommonValues) { +TEST(NullCheckAfterDereferenceTest, ValueAssignment) { 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]] + void fun(int arg) { + int *Addressof = &arg; + int *Nullptr = nullptr; + int *Nullptr2 = 0; - if (p) { - *p = 42; // [[p_true]] - } else { - (void)0; // [[p_false]] - } + int *MallocExpr = (int *)malloc(sizeof(int)); + int *NewExpr = new int(3); - (void)0; // [[p_merge]] - - p = nullptr; // [[p_nullptr]] + int *ExternalFn = ext(); - p = ext(); // [[p_extern]] + (void)0; // [[p]] } )"; 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)))); + return UnorderedElementsAre(IsStringMapEntry( + "p", + AllOf(HoldsVariable("Addressof", Output, + HasNullabilityState(kBoolFalse, kBoolTrue)), + HoldsVariable("Nullptr", Output, + HasNullabilityState(kBoolTrue, kBoolFalse)), + HoldsVariable("Nullptr", Output, + HasNullabilityState(kBoolTrue, kBoolFalse)), + HoldsVariable("MallocExpr", Output, + HasNullabilityState(kBoolNullptr, kBoolNullptr)), + // HoldsVariable("NewExpr", Output, + // HasNullabilityState(kBoolFalse, kBoolTrue)), + HoldsVariable("ExternalFn", Output, + HasNullabilityState(kBoolNullptr, kBoolNullptr))))); }); } -TEST(NullCheckAfterDereferenceTest, MergeValues) { +TEST(NullCheckAfterDereferenceTest, BooleanDependence) { 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; + void fun(int *ptr, bool b) { + if (b) { + *ptr = 10; } else { - p = (int*)malloc(sizeof(int)); + ptr = nullptr; } - - (void)0; // [[p_merge]] + + (void)0; // [[p]] } )"; - 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)))); + RunDataflowAnalysis(Code, [](const llvm::StringMap> &Results, + const AnalysisOutputs &Outputs) { + ASSERT_THAT(Results.keys(), UnorderedElementsAre("p")); + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + auto &BoolVal = getValueForDecl(Outputs.ASTCtx, Env, "b"); + auto &PtrVal = getValueForDecl(Outputs.ASTCtx, Env, "ptr"); + + auto *IsNull = cast_or_null(PtrVal.getProperty(kIsNull)); + auto *IsNonnull = cast_or_null(PtrVal.getProperty(kIsNonnull)); + ASSERT_THAT(IsNull, NotNull()); + ASSERT_THAT(IsNonnull, NotNull()); + + ASSERT_EQ(checkNullabilityState( + &Env.makeImplication(BoolVal, Env.makeNot(*IsNull)), Env), + kBoolTrue); + ASSERT_EQ(checkNullabilityState( + &Env.makeImplication(Env.makeNot(BoolVal), *IsNull), Env), + kBoolTrue); + ASSERT_EQ( + checkNullabilityState(&Env.makeImplication(BoolVal, *IsNonnull), Env), + kBoolTrue); + ASSERT_EQ( + checkNullabilityState(&Env.makeImplication(*IsNonnull, BoolVal), Env), + kBoolTrue); }); } From b2427cdf3e4adb0167b5c208affbb260afb60953 Mon Sep 17 00:00:00 2001 From: Viktor Date: Tue, 8 Apr 2025 08:31:49 +0000 Subject: [PATCH 17/19] Minor fixes --- .../NullCheckAfterDereferenceCheck.cpp | 10 +- clang-tools-extra/docs/ReleaseNotes.rst | 12 +-- .../bugprone/null-check-after-dereference.rst | 4 +- .../Models/NullPointerAnalysisModel.h | 8 -- .../Models/NullPointerAnalysisModel.cpp | 92 +++++++++---------- 5 files changed, 58 insertions(+), 68 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp index 41f14b44bff38..6ea8e174b7149 100644 --- a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp @@ -65,12 +65,10 @@ analyzeFunction(const FunctionDecl &FuncDecl) { NullPointerAnalysisModel Analysis(ASTCtx); Diagnoser Diagnoser; - Expected Diagnostics = - dataflow::diagnoseFunction( - FuncDecl, ASTCtx, Diagnoser); - + Diagnoser::ResultType Diagnostics; - if (llvm::Error E = Diagnostics.takeError()) { + if (llvm::Error E = dataflow::diagnoseFunction( + FuncDecl, ASTCtx, Diagnoser).moveInto(Diagnostics)) { llvm::dbgs() << "Dataflow analysis failed: " << llvm::toString(std::move(E)) << ".\n"; return std::nullopt; @@ -78,7 +76,7 @@ analyzeFunction(const FunctionDecl &FuncDecl) { ExpandedResultType ExpandedDiagnostics; - llvm::transform(*Diagnostics, + llvm::transform(Diagnostics, std::back_inserter(ExpandedDiagnostics), [&](Diagnoser::DiagnosticEntry Entry) -> ExpandedResult { if (auto Val = Diagnoser.WarningLocToVal[Entry.Location]; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 0d1069c30b3a3..303498803b363 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -103,18 +103,18 @@ New checks Finds lambda captures that capture the ``this`` pointer and store it as class members without handle the copy and move constructors and the assignments. +- New :doc:`bugprone-null-check-after-dereference + ` check. + + Identifies redundant pointer null-checks, by finding cases where + the pointer cannot be null at the location of the null-check. + - New :doc:`bugprone-unintended-char-ostream-output ` check. Finds unintended character output from ``unsigned char`` and ``signed char`` to an ``ostream``. -- New :doc:`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:`readability-ambiguous-smartptr-reset-call ` check. 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 756d1f5453437..70b8311af947a 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 @@ -9,8 +9,8 @@ bugprone-null-check-after-dereference 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. +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 diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h index d52a5062f60b2..af5145aaef190 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h @@ -39,14 +39,6 @@ namespace clang::dataflow { class NullPointerAnalysisModel : public DataflowAnalysis { -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 TransferMatchSwitch; public: diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp index 200a9e5ddddc7..36907c8fdbbe2 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp @@ -163,25 +163,25 @@ SatisfiabilityResult computeSatisfiability(BoolValue *Val, return SR::Unknown; } -inline BoolValue &getVal(llvm::StringRef Name, Value &RootValue) { - return *cast(RootValue.getProperty(Name)); +inline BoolValue &getVal(llvm::StringRef Name, Value &PtrValue) { + return *cast(PtrValue.getProperty(Name)); } // Assigns initial pointer null- and nonnull-values to a given Value. -void initializeRootValue(Value &RootValue, Environment &Env) { +void initializeNullnessProperties(Value &PtrValue, Environment &Env) { Arena &A = Env.arena(); - auto *IsNull = cast_or_null(RootValue.getProperty(kIsNull)); - auto *IsNonnull = cast_or_null(RootValue.getProperty(kIsNonnull)); + auto *IsNull = cast_or_null(PtrValue.getProperty(kIsNull)); + auto *IsNonnull = cast_or_null(PtrValue.getProperty(kIsNonnull)); if (!IsNull) { IsNull = &A.makeAtomValue(); - RootValue.setProperty(kIsNull, *IsNull); + PtrValue.setProperty(kIsNull, *IsNull); } if (!IsNonnull) { IsNonnull = &A.makeAtomValue(); - RootValue.setProperty(kIsNonnull, *IsNonnull); + PtrValue.setProperty(kIsNonnull, *IsNonnull); } // If the pointer cannot have either a null or nonnull value, the state is @@ -217,7 +217,7 @@ 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 // null-properties. - initializeRootValue(*EnvVal, Env); + initializeNullnessProperties(*EnvVal, Env); return EnvVal; } @@ -236,11 +236,11 @@ void matchDereferenceExpr(const Stmt *stmt, const auto *Var = Result.Nodes.getNodeAs(kVar); assert(Var != nullptr); - Value *RootValue = getValue(*Var, Env); - if (hasTopOrNullValue(RootValue, Env)) + Value *PtrValue = getValue(*Var, Env); + if (hasTopOrNullValue(PtrValue, Env)) return; - BoolValue &IsNull = getVal(kIsNull, *RootValue); + BoolValue &IsNull = getVal(kIsNull, *PtrValue); Env.assume(Env.arena().makeNot(IsNull.formula())); } @@ -253,18 +253,18 @@ void matchNullCheckExpr(const Expr *NullCheck, // (bool)p or (p != nullptr) bool IsNonnullOp = true; - if (auto *BinOp = dyn_cast(NullCheck); + if (const auto *BinOp = dyn_cast(NullCheck); BinOp->getOpcode() == BO_EQ) { IsNonnullOp = false; } - Value *RootValue = getValue(*Var, Env); - if (hasTopOrNullValue(RootValue, Env)) + Value *PtrValue = getValue(*Var, Env); + if (hasTopOrNullValue(PtrValue, Env)) return; Arena &A = Env.arena(); - BoolValue &IsNonnull = getVal(kIsNonnull, *RootValue); - BoolValue &IsNull = getVal(kIsNull, *RootValue); + BoolValue &IsNonnull = getVal(kIsNonnull, *PtrValue); + BoolValue &IsNull = getVal(kIsNull, *PtrValue); BoolValue *CondValue = cast_or_null(Env.getValue(*NullCheck)); if (!CondValue) { @@ -327,15 +327,15 @@ void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result, const auto *PrVar = Result.Nodes.getNodeAs(kVar); assert(PrVar != nullptr); - Value *RootValue = Env.getValue(*PrVar); - if (!RootValue) { - RootValue = Env.createValue(PrVar->getType()); - assert(RootValue && "Failed to create nullptr value"); - Env.setValue(*PrVar, *RootValue); + Value *PtrValue = Env.getValue(*PrVar); + if (!PtrValue) { + PtrValue = Env.createValue(PrVar->getType()); + assert(PtrValue && "Failed to create nullptr value"); + Env.setValue(*PrVar, *PtrValue); } - RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(true)); - RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(false)); + PtrValue->setProperty(kIsNull, Env.getBoolLiteralValue(true)); + PtrValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(false)); } void matchAddressofExpr(const Expr *expr, @@ -345,18 +345,18 @@ void matchAddressofExpr(const Expr *expr, assert(Var != nullptr); // FIXME: Use atoms or export to separate function - Value *RootValue = Env.getValue(*Var); - if (!RootValue) { - RootValue = Env.createValue(Var->getType()); + Value *PtrValue = Env.getValue(*Var); + if (!PtrValue) { + PtrValue = Env.createValue(Var->getType()); - if (!RootValue) + if (!PtrValue) return; - setUnknownValue(*Var, *RootValue, Env); + setUnknownValue(*Var, *PtrValue, Env); } - RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(false)); - RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(true)); + PtrValue->setProperty(kIsNull, Env.getBoolLiteralValue(false)); + PtrValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(true)); } void matchPtrArgFunctionExpr(const CallExpr *fncall, @@ -395,12 +395,12 @@ void matchPtrArgFunctionExpr(const CallExpr *fncall, if (fncall->getCallReturnType(*Result.Context)->isPointerType() && !Env.getValue(*fncall)) { - Value *RootValue = Env.createValue( + Value *PtrValue = Env.createValue( fncall->getCallReturnType(*Result.Context)); - if (!RootValue) + if (!PtrValue) return; - setUnknownValue(*fncall, *RootValue, Env); + setUnknownValue(*fncall, *PtrValue, Env); } } @@ -417,11 +417,11 @@ void matchAnyPointerExpr(const Expr *fncall, if (Env.getValue(*Var)) return; - Value *RootValue = Env.createValue(Var->getType()); - if (!RootValue) + Value *PtrValue = Env.createValue(Var->getType()); + if (!PtrValue) return; - setUnknownValue(*Var, *RootValue, Env); + setUnknownValue(*Var, *PtrValue, Env); } Diagnoser::ResultType @@ -432,13 +432,13 @@ diagnoseDerefLocation(const Expr *Deref, const MatchFinder::MatchResult &Result, const auto *Var = Result.Nodes.getNodeAs(kVar); assert(Var != nullptr); - Value *RootValue = Env.getValue(*Var); - if (!RootValue) + Value *PtrValue = Env.getValue(*Var); + if (!PtrValue) 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; + auto It = ValToDerefLoc.try_emplace(PtrValue, nullptr).first; It->second = Deref; @@ -474,17 +474,17 @@ diagnoseNullCheckExpr(const Expr *NullCheck, const auto *Var = Result.Nodes.getNodeAs(kVar); assert(Var != nullptr); - if (Value *RootValue = Env.getValue(*Var)) { + if (Value *PtrValue = 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 (PtrValue->getProperty(kIsNull) && PtrValue->getProperty(kIsNonnull)) { + bool IsNull = Env.allows(getVal(kIsNull, *PtrValue).formula()); + bool IsNonnull = Env.allows(getVal(kIsNonnull, *PtrValue).formula()); if (!IsNull && IsNonnull) { // FIXME: Separate function bool Inserted = - WarningLocToVal.try_emplace(Var->getBeginLoc(), RootValue).second; + WarningLocToVal.try_emplace(Var->getBeginLoc(), PtrValue).second; assert(Inserted && "multiple warnings at the same source location"); (void)Inserted; @@ -493,7 +493,7 @@ diagnoseNullCheckExpr(const Expr *NullCheck, if (IsNull && !IsNonnull) { bool Inserted = - WarningLocToVal.try_emplace(Var->getBeginLoc(), RootValue).second; + WarningLocToVal.try_emplace(Var->getBeginLoc(), PtrValue).second; assert(Inserted && "multiple warnings at the same source location"); (void)Inserted; @@ -502,7 +502,7 @@ diagnoseNullCheckExpr(const Expr *NullCheck, } // If no matches are found, the null-check itself signals a special location - auto [It, Inserted] = ValToDerefLoc.try_emplace(RootValue, nullptr); + auto [It, Inserted] = ValToDerefLoc.try_emplace(PtrValue, nullptr); if (Inserted) It->second = NullCheck; From e1ef074f850c8e2f66a030ecea0e49b6a4deb359 Mon Sep 17 00:00:00 2001 From: Viktor Date: Thu, 26 Jun 2025 11:22:20 +0000 Subject: [PATCH 18/19] Fix docs --- clang-tools-extra/docs/ReleaseNotes.rst | 4 ++-- .../checks/bugprone/null-check-after-dereference.rst | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 303498803b363..df5223bbb1fab 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -106,8 +106,8 @@ New checks - New :doc:`bugprone-null-check-after-dereference ` check. - Identifies redundant pointer null-checks, by finding cases where - the pointer cannot be null at the location of the null-check. + Identifies redundant pointer null-checks, by finding cases where the pointer + cannot be null at the location of the null-check. - New :doc:`bugprone-unintended-char-ostream-output ` check. 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 70b8311af947a..b451ceceea099 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 @@ -140,11 +140,11 @@ a pointer is not null. .. code-block:: c++ - int branches(int *p, bool b) { + int branches(int *ptr, bool b) { if (b) { - *p = 42; // true-positive: note-tag appended here + *ptr = 42; // true-positive: note-tag appended here } else { - *p = 20; // false-positive: note tag not appended here + *ptr = 20; // false-positive: note tag not appended here } return ptr ? *ptr : 0; From 0185bbc6758cc890269af1d8a7c3c0a86ee99905 Mon Sep 17 00:00:00 2001 From: Viktor Date: Tue, 1 Jul 2025 13:00:28 +0000 Subject: [PATCH 19/19] Simplify logic for compare and widen operations --- .../Models/NullPointerAnalysisModel.cpp | 79 ++++++++++++------- 1 file changed, 50 insertions(+), 29 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp index 36907c8fdbbe2..593f5463c00b8 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp @@ -611,12 +611,12 @@ void NullPointerAnalysisModel::join(QualType Type, const Value &Val1, }; LHSVar = SimplifyVar(LHSVar, Env1); - LHSVar = SimplifyVar(RHSVar, Env2); + RHSVar = SimplifyVar(RHSVar, Env2); // Handle special cases. if (LHSVar == RHSVar) return LHSVar ? *LHSVar : MergedEnv.makeAtomicBoolValue(); - else if (isa(LHSVar) || isa(RHSVar)) + else if (isa_and_nonnull(LHSVar) || isa_and_nonnull(RHSVar)) return MergedEnv.makeTopBoolValue(); if (!LHSVar) @@ -671,24 +671,32 @@ ComparisonResult NullPointerAnalysisModel::compare(QualType Type, auto *LHSVar = cast_or_null(Val1.getProperty(Name)); auto *RHSVar = cast_or_null(Val2.getProperty(Name)); - if (isa_and_present(LHSVar) || - isa_and_present(RHSVar)) - return CR::Top; - - if (LHSVar == RHSVar) - return CR::Same; + const auto SimplifyVar = [&](BoolValue *VarToSimplify, + const Environment &Env) -> BoolValue * { + SatisfiabilityResult SatResult = + computeSatisfiability(VarToSimplify, Env); + switch (SatResult) { + case SR::Nullptr: + return nullptr; + case SR::Top: + return &MergedEnv.makeTopBoolValue(); + case SR::True: + return &MergedEnv.getBoolLiteralValue(true); + case SR::False: + return &MergedEnv.getBoolLiteralValue(false); + case SR::Unknown: + return VarToSimplify; + } + }; - SatisfiabilityResult LHSResult = computeSatisfiability(LHSVar, Env1); - SatisfiabilityResult RHSResult = computeSatisfiability(RHSVar, Env2); + LHSVar = SimplifyVar(LHSVar, Env1); + RHSVar = SimplifyVar(RHSVar, Env2); - if (LHSResult == SR::Top || RHSResult == SR::Top) + // Handle special cases. + if (isa_and_nonnull(LHSVar) || isa_and_nonnull(RHSVar)) return CR::Top; - - if (LHSResult == SR::Unknown || RHSResult == SR::Unknown) - return CR::Unknown; - - if (LHSResult == RHSResult) - return CR::Same; + else if (LHSVar == RHSVar) + return LHSVar ? CR::Same : CR::Unknown; return CR::Different; }; @@ -718,28 +726,41 @@ ComparisonResult compareAndReplace(QualType Type, Value &Val1, if (!Type->isAnyPointerType()) return ComparisonResult::Unknown; + // Evaluate values, but different values compare to Unknown. auto FastCompareValues = [&](llvm::StringRef Name) -> CR { auto *LHSVar = cast_or_null(Val1.getProperty(Name)); auto *RHSVar = cast_or_null(Val2.getProperty(Name)); - SatisfiabilityResult LHSResult = shallowComputeSatisfiability(LHSVar, Env1); - SatisfiabilityResult RHSResult = shallowComputeSatisfiability(RHSVar, Env2); + const auto SimplifyVar = [&](BoolValue *VarToSimplify, + const Environment &Env) -> BoolValue * { + SatisfiabilityResult SatResult = + shallowComputeSatisfiability(VarToSimplify, Env); + switch (SatResult) { + case SR::Nullptr: + return nullptr; + case SR::Top: + return &MergedEnv.makeTopBoolValue(); + case SR::True: + return &MergedEnv.getBoolLiteralValue(true); + case SR::False: + return &MergedEnv.getBoolLiteralValue(false); + case SR::Unknown: + return VarToSimplify; + } + }; + + LHSVar = SimplifyVar(LHSVar, Env1); + RHSVar = SimplifyVar(RHSVar, Env2); - if (LHSResult == SR::Top || RHSResult == SR::Top) { + // Handle special cases. + if (isa_and_nonnull(LHSVar) || isa_and_nonnull(RHSVar)) { Val2.setProperty(Name, Env2.makeTopBoolValue()); return CR::Top; - } + } else if (LHSVar == RHSVar) + return LHSVar ? CR::Same : CR::Unknown; - if (LHSResult == SR::Unknown || RHSResult == SR::Unknown) - return CR::Unknown; - - if (LHSResult == RHSResult) - return CR::Same; - - Val2.setProperty(Name, Env2.makeTopBoolValue()); return CR::Different; }; - CR NullComparison = FastCompareValues(kIsNull); CR NonnullComparison = FastCompareValues(kIsNonnull);