[clang-tools-extra] added Conflicting Global Accesses checker (PR #130421)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Mar 9 13:19:11 PDT 2025
https://github.com/ConcreteCactus updated https://github.com/llvm/llvm-project/pull/130421
>From 290d0db1ddad65a4162bca367dd0807c039d8a42 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81ron=20H=C3=A1rn=C3=A1si?= <aron.harnasi at gmail.com>
Date: Fri, 22 Nov 2024 21:43:04 +0100
Subject: [PATCH] Added Conflicting Global Accesses checker
This checker attempts to detect unsequenced accesses to global
variables. It recurses into function calls in the same translation unit,
and can handle fields on global structs/unions.
---
.../bugprone/BugproneTidyModule.cpp | 3 +
.../clang-tidy/bugprone/CMakeLists.txt | 1 +
.../bugprone/ConflictingGlobalAccesses.cpp | 728 ++++++++++++++++++
.../bugprone/ConflictingGlobalAccesses.h | 61 ++
.../bugprone/conflicting-global-accesses.cpp | 267 +++++++
5 files changed, 1060 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
create mode 100644 clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.h
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/conflicting-global-accesses.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 0a3376949b6e5..5418e718a404e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -19,6 +19,7 @@
#include "CastingThroughVoidCheck.h"
#include "ChainedComparisonCheck.h"
#include "ComparePointerToMemberVirtualFunctionCheck.h"
+#include "ConflictingGlobalAccesses.h"
#include "CopyConstructorInitCheck.h"
#include "CrtpConstructorAccessibilityCheck.h"
#include "DanglingHandleCheck.h"
@@ -124,6 +125,8 @@ class BugproneModule : public ClangTidyModule {
"bugprone-chained-comparison");
CheckFactories.registerCheck<ComparePointerToMemberVirtualFunctionCheck>(
"bugprone-compare-pointer-to-member-virtual-function");
+ CheckFactories.registerCheck<ConflictingGlobalAccesses>(
+ "bugprone-conflicting-global-accesses");
CheckFactories.registerCheck<CopyConstructorInitCheck>(
"bugprone-copy-constructor-init");
CheckFactories.registerCheck<DanglingHandleCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index dab139b77c770..b66ef8ea3634b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -15,6 +15,7 @@ add_clang_library(clangTidyBugproneModule STATIC
CastingThroughVoidCheck.cpp
ChainedComparisonCheck.cpp
ComparePointerToMemberVirtualFunctionCheck.cpp
+ ConflictingGlobalAccesses.cpp
CopyConstructorInitCheck.cpp
CrtpConstructorAccessibilityCheck.cpp
DanglingHandleCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp b/clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
new file mode 100644
index 0000000000000..c6eb42ca57519
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
@@ -0,0 +1,728 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 "ConflictingGlobalAccesses.h"
+
+#include "clang/AST/RecursiveASTVisitor.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+// An AccesKind represents one access to a global variable.
+//
+// The unchecked versions represent reads/writes that are not handled by
+// -Wunsequenced. (e.g. accesses inside functions).
+using AccessKind = uint8_t;
+static constexpr AccessKind AkRead = 0;
+static constexpr AccessKind AkWrite = 1;
+static constexpr AccessKind AkUncheckedRead = 2;
+static constexpr AccessKind AkUncheckedWrite = 3;
+
+static constexpr uint8_t AkCount = 4;
+
+// The TraversalResultKind represents a set of accesses.
+// Bits are corresponding to the AccessKind enum values.
+using TraversalResultKind = uint8_t;
+static constexpr TraversalResultKind TrInvalid = 0;
+static constexpr TraversalResultKind TrRead = 1 << AkRead;
+static constexpr TraversalResultKind TrWrite = 1 << AkWrite;
+static constexpr TraversalResultKind TrUncheckedWrite = 1 << AkUncheckedWrite;
+
+// To represent fields in structs or unions we use numbered FieldIndices. The
+// FieldIndexArray represents one field inside a global struct/union system.
+// The FieldIndexArray can be thought of as a path inside a tree.
+using FieldIndex = uint16_t;
+static constexpr FieldIndex FiUnion = 0x8000;
+
+// Note: This bit signals whether the field is a *field of* a struct or a
+// union, not whether the type of the field itself is a struct or a union.
+using FieldIndexArray = SmallVector<FieldIndex>;
+
+/// One traversal recurses into one side of a binary expression or one
+/// parameter of a function call. At least two of these traversals are used to
+/// find conflicting accesses.
+///
+/// A TraversalResult represents one traversal.
+struct TraversalResult {
+ int IndexCreated; // We use indices to keep track of which
+ // traversal we are in currently. The current
+ // index is stored in GlobalRWVisitor with the
+ // name TraversalIndex.
+ SourceLocation Loc[AkCount];
+ TraversalResultKind Kind;
+
+ TraversalResult();
+ TraversalResult(int Index, SourceLocation Loc, AccessKind Access);
+ void addNewAccess(SourceLocation Loc, AccessKind Access);
+};
+
+/// The result of a number of traversals.
+class TraversalAggregation {
+ DeclarationName DeclName; // The name of the global variable being checked.
+
+ // We only store the result of two traversals as two conflicting accesses
+ // are enough to detect undefined behavior. The two stored TraversalResults
+ // have different traversal indices.
+ //
+ // Note: Sometimes multiple traversals are merged into one
+ // TraversalResult.
+ TraversalResult MainPart, OtherPart;
+ // Pairings that are not reportable: Read-Read, Read-Write,
+ // Read-UncheckedRead, Write-Write, UncheckedRead-UncheckedRead.
+
+public:
+ TraversalAggregation();
+ TraversalAggregation(DeclarationName Name, SourceLocation Loc,
+ AccessKind Access, int Index);
+ void addGlobalRW(SourceLocation Loc, AccessKind Access, int Index);
+ DeclarationName getDeclName() const;
+
+ bool isValid() const;
+
+ // If there is a conflict and that conflict isn't reported by -Wunsequenced
+ // then we report the conflict.
+ bool shouldBeReported() const;
+ bool hasConflictingOperations() const;
+
+private:
+ bool hasTwoAccesses() const;
+ bool isReportedByWunsequenced() const;
+};
+
+/// The ObjectAccessTree stores the TraversalAggregations of one global
+/// struct/union. Because each field can be handled as a single variable, the
+/// tree stores one TraversalAggregation for every field.
+///
+/// Note: structs, classes, and unions are called objects in the code.
+struct ObjectAccessTree {
+ using FieldMap = std::map<int, std::unique_ptr<ObjectAccessTree>>;
+ TraversalAggregation OwnAccesses;
+
+ // In a union, new fields should inherit from UnionTemporalAccesses
+ // instead of OwnAccesses. That's because an access to a field of a union is
+ // also an access to every other field of the same union.
+ TraversalAggregation UnionTemporalAccesses;
+
+ // We try to be lazy and only store fields that are actually accessed.
+ FieldMap Fields;
+ bool IsUnion;
+
+ ObjectAccessTree(TraversalAggregation Own);
+ ObjectAccessTree(ObjectAccessTree& Other) = delete;
+ ObjectAccessTree(const ObjectAccessTree& Other) = delete;
+ ObjectAccessTree(ObjectAccessTree&& Other) = default;
+
+ void addFieldToAll(SourceLocation Loc, AccessKind Access, int Index);
+ void addFieldToAllExcept(uint16_t ExceptIndex, SourceLocation Loc,
+ AccessKind Access, int Index);
+};
+
+/// This object is the root of all ObjectAccessTrees.
+class ObjectTraversalAggregation {
+ DeclarationName DeclName; // The name of the global struct/union.
+ ObjectAccessTree AccessTree;
+
+public:
+ ObjectTraversalAggregation(DeclarationName Name, SourceLocation Loc,
+ FieldIndexArray FieldIndices, AccessKind Access,
+ int Index);
+ void addFieldRW(SourceLocation Loc, FieldIndexArray FieldIndices,
+ AccessKind Access, int Index);
+ DeclarationName getDeclName() const;
+ bool shouldBeReported() const;
+
+private:
+ bool shouldBeReportedRec(const ObjectAccessTree *Node) const;
+};
+
+/// GlobalRWVisitor (global read write visitor) does all the traversals.
+class GlobalRWVisitor : public RecursiveASTVisitor<GlobalRWVisitor> {
+ friend RecursiveASTVisitor<GlobalRWVisitor>;
+
+public:
+ GlobalRWVisitor();
+
+ // startTraversal is called to start a new traversal. It increments the
+ // TraversalIndex, which in turn will generate new TraversalResults.
+ void startTraversal(Expr *E);
+
+ const std::vector<TraversalAggregation> &getGlobalsFound() const;
+
+ const std::vector<ObjectTraversalAggregation> &getObjectGlobalsFound() const;
+
+protected:
+ // RecursiveASTVisitor overrides
+ bool VisitDeclRefExpr(DeclRefExpr *S);
+ bool VisitUnaryOperator(UnaryOperator *Op);
+ bool VisitBinaryOperator(BinaryOperator *Op);
+ bool VisitCallExpr(CallExpr *CE);
+ bool VisitMemberExpr(MemberExpr *ME);
+
+private:
+ void visitCallExprArgs(CallExpr *CE);
+ void traverseFunctionsToBeChecked();
+
+ std::vector<TraversalAggregation> GlobalsFound;
+ std::vector<ObjectTraversalAggregation> ObjectGlobalsFound;
+
+ // We check inside functions only if the functions hasn't been checked in
+ // the current traversal. We use this array to check if the function is
+ // already registered to be checked.
+ std::vector<std::pair<DeclarationName, Stmt *>> FunctionsToBeChecked;
+
+ // The TraversalIndex is used to differentiate between two sides of a binary
+ // expression or the parameters of a function. Every traversal represents
+ // one such expression and the TraversalIndex is incremented between them.
+ int TraversalIndex;
+
+ // Accesses that are inside functions are not checked by -Wunsequenced,
+ // therefore we keep track of whether we are inside of a function or not.
+ bool IsInFunction;
+
+ void addGlobal(DeclarationName Name, SourceLocation Loc, bool IsWrite,
+ bool IsUnchecked);
+ void addGlobal(const DeclRefExpr *DR, bool IsWrite, bool IsUnchecked);
+ void addField(DeclarationName Name, FieldIndexArray FieldIndices,
+ SourceLocation Loc, bool IsWrite, bool IsUnchecked);
+ bool handleModified(const Expr *Modified, bool IsUnchecked);
+ bool handleModifiedVariable(const DeclRefExpr *DE, bool IsUnchecked);
+ bool handleAccessedObject(const Expr *E, bool IsWrite, bool IsUnchecked);
+ bool isVariable(const Expr *E);
+};
+} // namespace
+
+static bool isGlobalDecl(const VarDecl *VD) {
+ return VD && VD->hasGlobalStorage() && VD->getLocation().isValid() &&
+ !VD->getType().isConstQualified();
+}
+
+AST_MATCHER_P(Expr, twoGlobalWritesBetweenSequencePoints, const LangStandard *,
+ LangStd) {
+ assert(LangStd);
+
+ const Expr *E = &Node;
+
+ if (const BinaryOperator *Op = dyn_cast<BinaryOperator>(E)) {
+ const BinaryOperator::Opcode Code = Op->getOpcode();
+ if (Code == BO_LAnd || Code == BO_LOr || Code == BO_Comma) {
+ return false;
+ }
+
+ if (Op->isAssignmentOp() && isa<DeclRefExpr>(Op->getLHS())) {
+ return false;
+ }
+
+ if (LangStd->isCPlusPlus17() &&
+ (Code == BO_Shl || Code == BO_Shr || Code == BO_PtrMemD ||
+ Code == BO_PtrMemI || Op->isAssignmentOp())) {
+ return false;
+ }
+
+ return true;
+ }
+
+ if (isa<CallExpr>(E)) {
+ return true;
+ }
+
+ return false;
+}
+
+ConflictingGlobalAccesses::ConflictingGlobalAccesses(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+
+void ConflictingGlobalAccesses::registerMatchers(MatchFinder *Finder) {
+
+ const LangStandard *LangStd =
+ &LangStandard::getLangStandardForKind(getLangOpts().LangStd);
+
+ ast_matchers::internal::Matcher<Expr> GlobalAccessMatcher =
+ twoGlobalWritesBetweenSequencePoints(LangStd);
+
+ Finder->addMatcher(
+ stmt(traverse(TK_AsIs, expr(GlobalAccessMatcher).bind("gw"))), this);
+}
+
+void ConflictingGlobalAccesses::check(const MatchFinder::MatchResult &Result) {
+ const Expr *E = Result.Nodes.getNodeAs<Expr>("gw");
+ assert(E);
+
+ GlobalRWVisitor Visitor;
+ if (const auto *Op = dyn_cast<BinaryOperator>(E)) {
+ Visitor.startTraversal(Op->getLHS());
+ Visitor.startTraversal(Op->getRHS());
+
+ } else if (const auto *CE = dyn_cast<CallExpr>(E)) {
+ for (uint32_t I = 0; I < CE->getNumArgs(); I++) {
+ Visitor.startTraversal(const_cast<Expr *>(CE->getArg(I)));
+ }
+ }
+
+ const std::vector<TraversalAggregation> &Globals = Visitor.getGlobalsFound();
+
+ for (uint32_t I = 0; I < Globals.size(); I++) {
+ if (Globals[I].shouldBeReported()) {
+ diag(E->getBeginLoc(), "read/write conflict on global variable " +
+ Globals[I].getDeclName().getAsString());
+ }
+ }
+ const std::vector<ObjectTraversalAggregation> &ObjectGlobals =
+ Visitor.getObjectGlobalsFound();
+ for (uint32_t I = 0; I < ObjectGlobals.size(); I++) {
+ if (ObjectGlobals[I].shouldBeReported()) {
+ diag(E->getBeginLoc(), "read/write conflict on the field of the global "
+ "object " +
+ ObjectGlobals[I].getDeclName().getAsString());
+ }
+ }
+}
+
+GlobalRWVisitor::GlobalRWVisitor() : TraversalIndex(0), IsInFunction(false) {}
+
+void GlobalRWVisitor::startTraversal(Expr *E) {
+ TraversalIndex++;
+ FunctionsToBeChecked.clear();
+ IsInFunction = false;
+ TraverseStmt(E);
+
+ // We keep a list of functions to be checked during traversal so that they are
+ // not checked multiple times.
+ traverseFunctionsToBeChecked();
+}
+
+void GlobalRWVisitor::traverseFunctionsToBeChecked() {
+ IsInFunction = true;
+
+ // We could find more functions to be checked while checking functions.
+ // Because a simple iterator could get invalidated, we index into the array.
+ for (size_t I = 0; I < FunctionsToBeChecked.size(); ++I) {
+ TraverseStmt(FunctionsToBeChecked[I].second);
+ }
+}
+
+bool GlobalRWVisitor::isVariable(const Expr *E) {
+ const Type *T = E->getType().getTypePtrOrNull();
+ return isa<DeclRefExpr>(E) && (!T->isRecordType() || T->isUnionType());
+}
+
+bool GlobalRWVisitor::VisitDeclRefExpr(DeclRefExpr *DR) {
+ if (!isa<VarDecl>(DR->getDecl())) {
+ return true;
+ }
+ const auto *VD = dyn_cast<VarDecl>(DR->getDecl());
+ if (!isVariable(DR)) {
+ return handleAccessedObject(DR, /*IsWrite*/ false, /*IsUnchecked*/ false);
+ }
+ if (isGlobalDecl(VD)) {
+ addGlobal(VD->getDeclName(), VD->getBeginLoc(), /*IsWrite*/ false,
+ /*IsUnchecked*/ false);
+ return true;
+ }
+ return true;
+}
+
+bool GlobalRWVisitor::VisitMemberExpr(MemberExpr *ME) {
+ return handleAccessedObject(ME, /*IsWrite*/ false, /*IsUnchecked*/ false);
+}
+
+bool GlobalRWVisitor::handleModifiedVariable(const DeclRefExpr *DR,
+ bool IsUnchecked) {
+ if (!isa<VarDecl>(DR->getDecl())) {
+ return true;
+ }
+ const auto *VD = dyn_cast<VarDecl>(DR->getDecl());
+
+ if (isGlobalDecl(VD)) {
+ addGlobal(VD->getDeclName(), VD->getBeginLoc(), /*IsWrite*/ true,
+ IsUnchecked);
+ return false;
+ }
+ return true;
+}
+
+bool GlobalRWVisitor::handleAccessedObject(const Expr *E, bool IsWrite,
+ bool IsUnchecked) {
+ const Expr *CurrentNode = E;
+ int NodeCount = 0;
+ while (isa<MemberExpr>(CurrentNode)) {
+ const MemberExpr *CurrentField = dyn_cast<MemberExpr>(CurrentNode);
+ if (CurrentField->isArrow()) {
+ return true;
+ }
+
+ const ValueDecl *Decl = CurrentField->getMemberDecl();
+ if (!isa<FieldDecl>(Decl)) {
+ return true;
+ }
+
+ CurrentNode = CurrentField->getBase();
+ NodeCount++;
+ }
+
+ if (!isa<DeclRefExpr>(CurrentNode)) {
+ return true;
+ }
+ const DeclRefExpr *Base = dyn_cast<DeclRefExpr>(CurrentNode);
+
+ if (!isa<VarDecl>(Base->getDecl())) {
+ return true;
+ }
+ const VarDecl *BaseDecl = dyn_cast<VarDecl>(Base->getDecl());
+
+ if (!isGlobalDecl(BaseDecl)) {
+ return true;
+ }
+
+ FieldIndexArray FieldIndices(NodeCount);
+ CurrentNode = E;
+ while (isa<MemberExpr>(CurrentNode)) {
+ const MemberExpr *CurrentField = dyn_cast<MemberExpr>(CurrentNode);
+ const FieldDecl *Decl = dyn_cast<FieldDecl>(CurrentField->getMemberDecl());
+
+ FieldIndices[NodeCount - 1] = Decl->getFieldIndex();
+ const RecordDecl *Record = Decl->getParent();
+ if (Record && Record->isUnion()) {
+ // Not sure what to do if Record isn't a value.
+ FieldIndices[NodeCount - 1] |= FiUnion;
+ }
+
+ CurrentNode = CurrentField->getBase();
+ NodeCount--;
+ }
+
+ addField(BaseDecl->getDeclName(), FieldIndices, Base->getBeginLoc(), IsWrite,
+ IsUnchecked);
+ return false;
+}
+
+bool GlobalRWVisitor::handleModified(const Expr *Modified, bool IsUnchecked) {
+ if (!Modified) {
+ return true;
+ }
+
+ if (isVariable(Modified)) {
+ return handleModifiedVariable(dyn_cast<DeclRefExpr>(Modified), IsUnchecked);
+ }
+
+ return handleAccessedObject(Modified, /*IsWrite*/ true, IsUnchecked);
+}
+
+bool GlobalRWVisitor::VisitUnaryOperator(UnaryOperator *Op) {
+ UnaryOperator::Opcode Code = Op->getOpcode();
+ if (Code == UO_PostInc || Code == UO_PostDec || Code == UO_PreInc ||
+ Code == UO_PreDec) {
+ return handleModified(Op->getSubExpr(), /*IsUnchecked*/ false);
+ }
+ return true;
+}
+
+bool GlobalRWVisitor::VisitBinaryOperator(BinaryOperator *Op) {
+ if (Op->isAssignmentOp()) {
+ return handleModified(Op->getLHS(), /*IsUnchecked*/ false);
+ }
+
+ return true;
+}
+
+void GlobalRWVisitor::visitCallExprArgs(CallExpr *CE) {
+ const Type *CT = CE->getCallee()->getType().getTypePtrOrNull();
+ if (const auto *PT = dyn_cast_if_present<PointerType>(CT)) {
+ CT = PT->getPointeeType().getTypePtrOrNull();
+ }
+ const auto *ProtoType = dyn_cast_if_present<FunctionProtoType>(CT);
+
+ for (uint32_t I = 0; I < CE->getNumArgs(); I++) {
+ Expr *Arg = CE->getArg(I);
+
+ if (!ProtoType || I >= ProtoType->getNumParams()) {
+ continue;
+ }
+
+ if (const auto *Op = dyn_cast<UnaryOperator>(Arg)) {
+ if (Op->getOpcode() != UO_AddrOf) {
+ continue;
+ }
+
+ if (const auto *PtrType = dyn_cast_if_present<PointerType>(
+ ProtoType->getParamType(I).getTypePtrOrNull())) {
+ if (PtrType->getPointeeType().isConstQualified()) {
+ continue;
+ }
+
+ if (handleModified(Op->getSubExpr(), /*IsUnchecked*/ true)) {
+ continue;
+ }
+ }
+ }
+
+ if (const auto *RefType = dyn_cast_if_present<ReferenceType>(
+ ProtoType->getParamType(I).getTypePtrOrNull())) {
+ if (RefType->getPointeeType().isConstQualified()) {
+ continue;
+ }
+
+ if (handleModified(Arg, /*IsUnchecked*/ true)) {
+ continue;
+ }
+ }
+ }
+}
+
+bool GlobalRWVisitor::VisitCallExpr(CallExpr *CE) {
+ visitCallExprArgs(CE);
+
+ if (!isa_and_nonnull<FunctionDecl>(CE->getCalleeDecl())) {
+ return true;
+ }
+ const auto *FD = dyn_cast<FunctionDecl>(CE->getCalleeDecl());
+
+ if (!FD->hasBody()) {
+ return true;
+ }
+
+ for (const std::pair<DeclarationName, Stmt *> &Fun : FunctionsToBeChecked) {
+ if (Fun.first == FD->getDeclName()) {
+ return true;
+ }
+ }
+ FunctionsToBeChecked.emplace_back(FD->getDeclName(), FD->getBody());
+
+ return true;
+}
+
+const std::vector<TraversalAggregation> &
+GlobalRWVisitor::getGlobalsFound() const {
+ return GlobalsFound;
+}
+
+const std::vector<ObjectTraversalAggregation> &
+GlobalRWVisitor::getObjectGlobalsFound() const {
+ return ObjectGlobalsFound;
+}
+
+void GlobalRWVisitor::addGlobal(DeclarationName Name, SourceLocation Loc,
+ bool IsWrite, bool IsUnchecked) {
+ AccessKind Access = (IsInFunction || IsUnchecked)
+ ? (IsWrite ? AkUncheckedWrite : AkUncheckedRead)
+ : (IsWrite ? AkWrite : AkRead);
+ for (uint32_t I = 0; I < GlobalsFound.size(); I++) {
+ if (GlobalsFound[I].getDeclName() == Name) {
+ GlobalsFound[I].addGlobalRW(Loc, Access, TraversalIndex);
+ return;
+ }
+ }
+
+ GlobalsFound.emplace_back(Name, Loc, Access, TraversalIndex);
+}
+
+void GlobalRWVisitor::addField(DeclarationName Name,
+ FieldIndexArray FieldIndices, SourceLocation Loc,
+ bool IsWrite, bool IsUnchecked) {
+ AccessKind Access = (IsInFunction || IsUnchecked)
+ ? (IsWrite ? AkUncheckedWrite : AkUncheckedRead)
+ : (IsWrite ? AkWrite : AkRead);
+ for (uint32_t I = 0; I < ObjectGlobalsFound.size(); I++) {
+ if (ObjectGlobalsFound[I].getDeclName() == Name) {
+ ObjectGlobalsFound[I].addFieldRW(Loc, FieldIndices, Access,
+ TraversalIndex);
+ return;
+ }
+ }
+ ObjectGlobalsFound.emplace_back(Name, Loc, FieldIndices, Access,
+ TraversalIndex);
+}
+
+static TraversalResultKind akToTr(AccessKind Ak) { return 1 << Ak; }
+
+TraversalAggregation::TraversalAggregation() {}
+
+TraversalAggregation::TraversalAggregation(DeclarationName Name,
+ SourceLocation Loc,
+ AccessKind Access, int Index)
+ : DeclName(Name), MainPart(Index, Loc, Access) {}
+
+void TraversalAggregation::addGlobalRW(SourceLocation Loc, AccessKind Access,
+ int Index) {
+ if (!isValid()) {
+ MainPart = TraversalResult(Index, Loc, Access);
+ return;
+ }
+ if (isReportedByWunsequenced()) {
+ return;
+ }
+ if (MainPart.IndexCreated == Index) {
+ MainPart.addNewAccess(Loc, Access);
+ return;
+ }
+ if (!hasTwoAccesses()) {
+ OtherPart = TraversalResult(Index, Loc, Access);
+ return;
+ }
+ if (OtherPart.IndexCreated == Index) {
+ OtherPart.addNewAccess(Loc, Access);
+ return;
+ }
+ // We know that the current state is not reported by -Wunsequenced, so
+ // either one of the parts has only unchecked accesses, or both parts have
+ // only reads.
+ switch (Access) {
+ case AkWrite:
+ case AkUncheckedWrite: {
+ if (OtherPart.Kind & (TrRead | TrWrite)) {
+ MainPart = OtherPart;
+ }
+ OtherPart = TraversalResult(Index, Loc, Access);
+ break;
+ }
+ case AkRead: {
+ if (!(MainPart.Kind & TrWrite) &&
+ (OtherPart.Kind & (TrWrite | TrUncheckedWrite))) {
+ MainPart = OtherPart;
+ }
+ OtherPart = TraversalResult(Index, Loc, Access);
+ break;
+ }
+ case AkUncheckedRead:
+ default: {
+ break;
+ }
+ }
+}
+
+bool TraversalAggregation::isValid() const {
+ return MainPart.Kind != TrInvalid;
+}
+
+DeclarationName TraversalAggregation::getDeclName() const { return DeclName; }
+
+bool TraversalAggregation::hasTwoAccesses() const {
+ return OtherPart.Kind != TrInvalid;
+}
+
+bool TraversalAggregation::hasConflictingOperations() const {
+ return hasTwoAccesses() &&
+ (MainPart.Kind | OtherPart.Kind) & (TrWrite | TrUncheckedWrite);
+}
+
+bool TraversalAggregation::isReportedByWunsequenced() const {
+ return ((MainPart.Kind | OtherPart.Kind) & TrWrite) &&
+ (MainPart.Kind & (TrWrite | TrRead)) &&
+ (OtherPart.Kind & (TrWrite | TrRead));
+}
+
+bool TraversalAggregation::shouldBeReported() const {
+ return hasConflictingOperations() && !isReportedByWunsequenced();
+}
+
+TraversalResult::TraversalResult() : IndexCreated(-1), Kind(TrInvalid) {}
+
+TraversalResult::TraversalResult(int Index, SourceLocation Location,
+ AccessKind Access)
+ : IndexCreated(Index), Kind(akToTr(Access)) {
+ Loc[Access] = Location;
+}
+
+void TraversalResult::addNewAccess(SourceLocation NewLoc, AccessKind Access) {
+ Kind |= 1 << Access;
+ Loc[Access] = NewLoc;
+}
+
+ObjectTraversalAggregation::ObjectTraversalAggregation(
+ DeclarationName Name, SourceLocation Loc, FieldIndexArray FieldIndices,
+ AccessKind Access, int Index)
+ : DeclName(Name), AccessTree(TraversalAggregation()) {
+ addFieldRW(Loc, FieldIndices, Access, Index);
+}
+
+void ObjectTraversalAggregation::addFieldRW(SourceLocation Loc,
+ FieldIndexArray FieldIndices,
+ AccessKind Access, int Index) {
+ ObjectAccessTree *CurrentNode = &AccessTree;
+ for (uint16_t I = 0; I < FieldIndices.size(); I++) {
+ bool IsUnion = (FieldIndices[I] & FiUnion) != 0;
+ uint16_t FieldKey = FieldIndices[I] & ~FiUnion;
+
+ ObjectAccessTree *PrevNode = CurrentNode;
+ ObjectAccessTree::FieldMap::iterator It =
+ CurrentNode->Fields.find(FieldKey);
+
+ if (It == CurrentNode->Fields.end()) {
+ CurrentNode =
+ new ObjectAccessTree(IsUnion ? CurrentNode->UnionTemporalAccesses
+ : CurrentNode->OwnAccesses);
+ PrevNode->Fields[FieldKey] =
+ std::unique_ptr<ObjectAccessTree>(CurrentNode);
+ } else {
+ CurrentNode = It->second.get();
+ }
+
+ if (IsUnion) {
+ if (!PrevNode->IsUnion) {
+ PrevNode->IsUnion = IsUnion; // Setting the parent of the
+ // field instead of the field
+ // itself.
+ PrevNode->UnionTemporalAccesses = PrevNode->OwnAccesses;
+ }
+ PrevNode->addFieldToAllExcept(FieldKey, Loc, Access, Index);
+ }
+ }
+ CurrentNode->addFieldToAll(Loc, Access, Index);
+}
+
+bool ObjectTraversalAggregation::shouldBeReported() const {
+ return shouldBeReportedRec(&AccessTree);
+}
+
+bool ObjectTraversalAggregation::shouldBeReportedRec(
+ const ObjectAccessTree *Node) const {
+ if (Node->OwnAccesses.hasConflictingOperations()) {
+ return true;
+ }
+ ObjectAccessTree::FieldMap::const_iterator FieldIt = Node->Fields.begin();
+ for (; FieldIt != Node->Fields.end(); FieldIt++) {
+ if (shouldBeReportedRec(FieldIt->second.get())) {
+ return true;
+ }
+ }
+ return false;
+}
+
+DeclarationName ObjectTraversalAggregation::getDeclName() const {
+ return DeclName;
+}
+
+ObjectAccessTree::ObjectAccessTree(TraversalAggregation Own)
+ : OwnAccesses(Own), IsUnion(false) {}
+
+void ObjectAccessTree::addFieldToAll(SourceLocation Loc, AccessKind Access,
+ int Index) {
+ OwnAccesses.addGlobalRW(Loc, Access, Index);
+ UnionTemporalAccesses.addGlobalRW(Loc, Access, Index);
+ FieldMap::iterator FieldIt = Fields.begin();
+ for (; FieldIt != Fields.end(); FieldIt++) {
+ FieldIt->second->addFieldToAll(Loc, Access, Index);
+ }
+}
+
+void ObjectAccessTree::addFieldToAllExcept(uint16_t ExceptIndex,
+ SourceLocation Loc,
+ AccessKind Access, int Index) {
+
+ UnionTemporalAccesses.addGlobalRW(Loc, Access, Index);
+ FieldMap::const_iterator FieldIt = Fields.begin();
+ for (; FieldIt != Fields.end(); FieldIt++) {
+ if (FieldIt->first != ExceptIndex) {
+ FieldIt->second->addFieldToAll(Loc, Access, Index);
+ }
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.h b/clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.h
new file mode 100644
index 0000000000000..ce8b66a70f8bf
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.h
@@ -0,0 +1,61 @@
+//===--- ConflictingGlobalAccesses.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_SIDEEFFECTBETWEENSEQUENCE\
+POINTSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIDEEFFECTBETWEENSEQUENCE\
+POINTSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds conflicting accesses on global variables.
+///
+/// Modifying twice or reading and modifying a memory location without a
+/// defined sequence of the operations is undefined behavior. This checker is
+/// similar to the -Wunsequenced clang warning, however it only looks at global
+/// variables and can find unsequenced operations inside functions as well.
+///
+/// For example: \code
+///
+/// int a = 0;
+/// int b = (a++) - a; // This is flagged by -Wunsequenced.
+///
+/// \endcode
+///
+/// However global variables allow for more complex scenarios that
+/// -Wunsequenced doesn't detect. E.g. \code
+///
+/// int globalVar = 0;
+///
+/// int incFun() {
+/// globalVar++;
+/// return globalVar;
+/// }
+///
+/// int main() {
+/// return globalVar + incFun(); // This is not detected by -Wunsequenced.
+/// }
+///
+/// \endcode
+///
+/// This checker attempts to detect such undefined behavior. It recurses into
+/// functions that are inside the same translation unit. It also attempts not to
+/// flag cases that are already covered by -Wunsequenced. Global unions and
+/// structs are also handled.
+class ConflictingGlobalAccesses : public ClangTidyCheck {
+public:
+ ConflictingGlobalAccesses(StringRef Name, ClangTidyContext *Context);
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/conflicting-global-accesses.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/conflicting-global-accesses.cpp
new file mode 100644
index 0000000000000..5fab3f5997e38
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/conflicting-global-accesses.cpp
@@ -0,0 +1,267 @@
+// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CPP11 %s bugprone-conflicting-global-accesses %t
+// RUN: %check_clang_tidy -std=c++17 %s bugprone-conflicting-global-accesses %t
+
+int GlobalVarA;
+
+int incGlobalVarA(void) {
+ GlobalVarA++;
+ return 0;
+}
+
+int getGlobalVarA(void) {
+ return GlobalVarA;
+}
+
+int undefinedFunc1(int);
+
+int testFunc1(void) {
+
+ int B = getGlobalVarA() + incGlobalVarA();
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: read/write conflict on global variable GlobalVarA
+ (void)B;
+
+ return GlobalVarA + incGlobalVarA();
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: read/write conflict on global variable GlobalVarA
+
+ return GlobalVarA + undefinedFunc1(incGlobalVarA());
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: read/write conflict on global variable GlobalVarA
+
+}
+
+int addAll(int A, int B, int C, int D) {
+ return A + B + C + D;
+}
+
+int testFunc2(void) {
+ int B;
+ (void)B;
+ // Make sure the order does not affect the outcome
+
+ B = getGlobalVarA() + (GlobalVarA++);
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: read/write conflict on global variable GlobalVarA
+
+ B = (GlobalVarA++) + getGlobalVarA();
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: read/write conflict on global variable GlobalVarA
+
+ B = incGlobalVarA() + GlobalVarA;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: read/write conflict on global variable GlobalVarA
+
+ B = addAll(GlobalVarA++, getGlobalVarA(), 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: read/write conflict on global variable GlobalVarA
+
+ B = addAll(getGlobalVarA(), GlobalVarA++, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: read/write conflict on global variable GlobalVarA
+
+ // This is already checked by the unsequenced clang warning, so we don't
+ // want to warn about this.
+ return GlobalVarA + (++GlobalVarA);
+}
+
+int testFunc3(void) {
+
+ // Make sure double reads are not flagged
+ int B = GlobalVarA + GlobalVarA; (void)B;
+ B = GlobalVarA + getGlobalVarA();
+
+ return GlobalVarA - GlobalVarA;
+}
+
+bool testFunc4(void) {
+
+ // Make sure || and && operators are not flagged
+ bool B = GlobalVarA || (GlobalVarA++);
+ if(GlobalVarA && (GlobalVarA--)) {
+
+ B = GlobalVarA || (GlobalVarA++) + getGlobalVarA();
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: read/write conflict on global variable GlobalVarA
+
+ return (++GlobalVarA) || B || getGlobalVarA();
+ }
+
+ int C = GlobalVarA << incGlobalVarA(); (void)C;
+ // CHECK-MESSAGES-CPP11: :[[@LINE-1]]:13: warning: read/write conflict on global variable GlobalVarA
+
+ return false;
+}
+
+int incArg(int& P) {
+ P++;
+ return 0;
+}
+
+int incArgPtr(int* P) {
+ (*P)++;
+ return 0;
+}
+
+int testFunc5() {
+
+ // Also check if statements
+
+ if(GlobalVarA > incGlobalVarA()) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: read/write conflict on global variable GlobalVarA
+
+ return 1;
+ }
+
+ if(addAll(GlobalVarA, incArg(GlobalVarA), 0, 0)) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: read/write conflict on global variable GlobalVarA
+ return 1;
+ }
+
+ if(addAll(GlobalVarA, incArgPtr(&GlobalVarA), 0, 0)) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: read/write conflict on global variable GlobalVarA
+ return 2;
+ }
+
+ if(addAll(GlobalVarA, 0, incGlobalVarA(), 0)) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: read/write conflict on global variable GlobalVarA
+ return 2;
+ }
+
+ return 0;
+}
+
+int testFunc6() {
+
+ // Shouldn't warn here as the write takes place after the expression is
+ // evaluated.
+ GlobalVarA = GlobalVarA + 1;
+ GlobalVarA = incGlobalVarA();
+
+ // Also check the assignment expression, array element assignment, and
+ // pointer dereference lvalues.
+ int A = (GlobalVarA = 1) + incGlobalVarA(); (void)A;
+ // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: read/write conflict on global variable GlobalVarA
+
+ int Array[] = {1, 2, 3};
+ Array[GlobalVarA] = incGlobalVarA();
+ // CHECK-MESSAGES-CPP11: :[[@LINE-1]]:5: warning: read/write conflict on global variable GlobalVarA
+
+ *(Array + GlobalVarA) = incGlobalVarA();
+ // CHECK-MESSAGES-CPP11: :[[@LINE-1]]:5: warning: read/write conflict on global variable GlobalVarA
+
+ // Shouldn't warn here as the clang warning takes care of it.
+ return addAll(GlobalVarA, getGlobalVarA(), GlobalVarA++, 0);
+}
+
+class TestClass1 {
+public:
+ static int StaticVar1;
+
+ int incStaticVar1() {
+ StaticVar1++;
+ return 0;
+ }
+
+ int getStaticVar1() {
+ return StaticVar1;
+ }
+
+ int testClass1MemberFunc1() {
+
+ return incStaticVar1() + getStaticVar1();
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: read/write conflict on global variable StaticVar1
+
+ }
+
+ TestClass1 operator++() {
+ incStaticVar1();
+ return *this;
+ }
+
+ operator int() {
+ return StaticVar1;
+ }
+};
+
+void testFunc7() {
+ TestClass1 Obj;
+ addAll(TestClass1::StaticVar1, Obj.incStaticVar1(), 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on global variable StaticVar1
+ addAll(TestClass1::StaticVar1, (Obj.incStaticVar1()), 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on global variable StaticVar1
+ addAll(TestClass1::StaticVar1, (Obj.incStaticVar1(), 0), 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on global variable StaticVar1
+ addAll(TestClass1::StaticVar1, ++Obj, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on global variable StaticVar1
+}
+
+struct {
+ int VarA;
+ int VarB;
+ struct {
+ int VarC;
+ int VarD;
+ } StructA;
+} GlobalStruct;
+
+struct {
+ int VarA;
+ union {
+ struct {
+ int VarB;
+ int VarC;
+ } StructA;
+ int VarD;
+ } UnionA;
+ int VarE;
+} ComplexGlobalStruct;
+
+struct QuiteComplexStruct {
+ int VarA;
+ union {
+ union {
+ int VarB;
+ int VarC;
+ struct QuiteComplexStruct* PtrA;
+ } UnionB;
+ int VarD;
+ } UnionA;
+ int VarE;
+} QuiteComplexGlobalStruct;
+
+
+void testFunc8() {
+
+ // Check if unions and structs are handled properly
+
+ addAll(GlobalStruct.VarA, GlobalStruct.VarB++, 0, 0);
+ addAll(GlobalStruct.StructA.VarD, GlobalStruct.VarA++, 0, 0);
+ addAll(GlobalStruct.StructA.VarC, GlobalStruct.StructA.VarD++, GlobalStruct.VarB++, GlobalStruct.VarA++);
+ addAll(GlobalStruct.VarA, (GlobalStruct.StructA = {}, 0), 0, 0);
+
+ addAll(GlobalStruct.StructA.VarD, (GlobalStruct.StructA = {}, 0), 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object GlobalStruct
+ addAll(GlobalStruct.VarA, (GlobalStruct.VarA++, 0), 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object GlobalStruct
+ addAll(GlobalStruct.StructA.VarC, (GlobalStruct = {}, 0), 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object GlobalStruct
+ addAll((GlobalStruct.StructA = {}, 1), (GlobalStruct = {}, 0), 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object GlobalStruct
+ addAll((GlobalStruct.StructA = {}, 1), (GlobalStruct.VarA++, 0), GlobalStruct.StructA.VarD, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object GlobalStruct
+
+ addAll(ComplexGlobalStruct.UnionA.VarD, ComplexGlobalStruct.UnionA.StructA.VarC++, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object ComplexGlobalStruct
+ addAll(ComplexGlobalStruct.UnionA.StructA.VarB, (ComplexGlobalStruct.UnionA.StructA = {}, 0), 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object ComplexGlobalStruct
+ addAll(ComplexGlobalStruct.UnionA.StructA.VarB, ComplexGlobalStruct.UnionA.VarD++, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object ComplexGlobalStruct
+ addAll((ComplexGlobalStruct.UnionA = {}, 0), ComplexGlobalStruct.UnionA.VarD++, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object ComplexGlobalStruct
+
+ addAll(ComplexGlobalStruct.UnionA.StructA.VarB, ComplexGlobalStruct.UnionA.StructA.VarC++, 0, 0);
+
+ addAll(QuiteComplexGlobalStruct.UnionA.UnionB.VarC, QuiteComplexGlobalStruct.UnionA.VarD++, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object QuiteComplexGlobalStruct
+ addAll(QuiteComplexGlobalStruct.UnionA.UnionB.VarC, QuiteComplexGlobalStruct.UnionA.UnionB.VarB++, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object QuiteComplexGlobalStruct
+
+ addAll(QuiteComplexGlobalStruct.UnionA.UnionB.VarC, QuiteComplexGlobalStruct.UnionA.UnionB.VarB++, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object QuiteComplexGlobalStruct
+
+ addAll(QuiteComplexGlobalStruct.UnionA.UnionB.PtrA->VarA, QuiteComplexGlobalStruct.UnionA.UnionB.VarB++, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: read/write conflict on the field of the global object QuiteComplexGlobalStruct
+ addAll(QuiteComplexGlobalStruct.UnionA.UnionB.PtrA->VarA, QuiteComplexGlobalStruct.VarA++, 0, 0);
+}
More information about the cfe-commits
mailing list