[clang-tidy] Added Conflicting Global Accesses checker #130421
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
ConcreteCactus
changed the title
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: cc (ConcreteCactus) ChangesThis checker attempts to cover the global variable case in rule EXP-30 in the SEI CERT C and rule EXP-50 in the SEI CERT C++ Coding Standards. It recurses into functions in the same translation unit and can handle global struct and union members as well. Patch is 40.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/130421.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index b780a85bdf3fe..09e3f481e9d1d 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -20,6 +20,7 @@
#include "CastingThroughVoidCheck.h"
#include "ChainedComparisonCheck.h"
#include "ComparePointerToMemberVirtualFunctionCheck.h"
+#include "ConflictingGlobalAccesses.h"
#include "CopyConstructorInitCheck.h"
#include "CrtpConstructorAccessibilityCheck.h"
#include "DanglingHandleCheck.h"
@@ -127,6 +128,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 e310ea9c94543..9754df6c06995 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -16,6 +16,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..48906bf715ab2
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
@@ -0,0 +1,748 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 = llvm::DenseMap<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);
+
+ 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(bool IsWritePossibleThroughFunctionParam);
+
+ // startTraversal is called to start a new traversal. It increments the
+ // TraversalIndex, which in turn will generate new TraversalResults.
+ void startTraversal(Expr *E);
+
+ const llvm::SmallVector<TraversalAggregation> &getGlobalsFound() const;
+
+ const llvm::SmallVector<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();
+
+ llvm::SmallVector<TraversalAggregation> GlobalsFound;
+ llvm::SmallVector<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.
+ llvm::SmallVector<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;
+
+ // Same as the HandleMutableFunctionParametersAsWrites option.
+ bool IsWritePossibleThroughFunctionParam;
+
+ 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),
+ HandleMutableFunctionParametersAsWrites(
+ Options.get("HandleMutableFunctionParametersAsWrites", false))
+{}
+
+void ConflictingGlobalAccesses::storeOptions(ClangTidyOptions::OptionMap& Opts)
+{
+ Options.store(Opts, "HandleMutableFunctionParametersAsWrites",
+ HandleMutableFunctionParametersAsWrites);
+}
+
+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(HandleMutableFunctionParametersAsWrites);
+ 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 llvm::SmallVector<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 llvm::SmallVector<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(bool IsWritePossibleThroughFunctionParam)
+ : TraversalIndex(0), IsInFunction(false),
+ IsWritePossibleThroughFunctionParam(IsWritePossibleThroughFunctionParam)
+{}
+
+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();
+ assert(T);
+
+ 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());
+ assert(Decl);
+
+ FieldIndices[NodeCount - 1] = Decl->getFieldIndex();
+ const RecordDecl *Record = Decl->getParent();
+ assert(Record);
+
+ if (Record->isUnion()) {
+ 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) {
+ assert(Modified);
+
+ 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) {
+
+ if (IsWritePossibleThroughFunctionParam || isa<CXXOperatorCallExpr>(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 llvm::SmallVector<TraversalAggregation> &
+GlobalRWVisitor::getGlobalsFound() const {
+ return GlobalsFound;
+}
+
+const llvm::SmallVector<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) {
+ Global...
[truncated]
|
PiotrZSL
reviewed
Mar 31, 2025
clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.h
Outdated
Show resolved
Hide resolved
Comment on lines
18
to
64
| /// 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. | ||
| /// | ||
| /// It's possible to enable handling mutable reference and pointer function | ||
| /// parameters as writes using the HandleMutableFunctionParametersAsWrites | ||
| /// option. For example: \code | ||
| /// | ||
| /// void func(int& a); | ||
| /// | ||
| /// int globalVar; | ||
| /// func(globalVar); // <- this could be a write to globalVar. | ||
| /// | ||
| /// \endcode | ||
| /// | ||
| /// This option is disabled by default. |
There was a problem hiding this comment.
this should be in documentation file, not here
There was a problem hiding this comment.
I put the comment into the documentation file. Please, let me know if you feel that more documentation is needed.
clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
Outdated
Show resolved
Hide resolved
| }; | ||
|
|
||
| /// GlobalRWVisitor (global read write visitor) does all the traversals. | ||
| class GlobalRWVisitor : public RecursiveASTVisitor<GlobalRWVisitor> { |
There was a problem hiding this comment.
i'm not sure about this, look into check bugprone-unchecked-optional, it uses different framework, maybe it could be used here, or look into framework used by bugprone-unhanded-exception.
For me this "call" based framework could be extracted into some utility class, and made available for everyone, so dev could derive from it, implement some handlers/visitors and use, instead of re-implementing it. And this does not cover everything, like modify of global object from destructor of temporary variable.
There was a problem hiding this comment.
Thanks, I'll look into how those checkers solve this.
There was a problem hiding this comment.
I looked into the framework used by bugprone-unchecked-optional. I think using the data flow analysis framework has some benefits and some drawbacks. For example, using a more thorough framework could help eliminate false-positives that require branch evaluation, and there wouldn't be a need for this GlobalRWVisitor. However, the checker would probably get slower, and it would need to be rewritten substantially. Maybe even moved into clang-sa.
I would prefer using this GlobalRWVisitor, but if you still think it's better to do a data flow analysis, then I will rewrite the checker to use that.
By the way, I couldn't find a checker named bugprone-unhandled-exception in clang-tidy. Only one named bugprone-unhandled-exception-at-new. But I don't think you are referring to that.
There was a problem hiding this comment.
Exception check is called bugprone-exception-escape and the checker file is ExceptionEscapeCheck.cpp. It uses framework defined in ExceptionAnalyzer.cpp.
There was a problem hiding this comment.
Looks like the ExceptionAnalyzer does its own recursion instead of relying to RecursiveASTVisitor. I think in my case its easier to rely on the visitor because this checker requires recursing into expressions which I find complicated.
In my last commit, I moved some of the functionality in GlobalRWVisitor to a new utility called ExecutionVisitor. This ExecutionVisitor contains the parts of the old GlobalRWVisitor that in my opinion could be useful in other checkers. That is, ExecutionVisitor traverses statements that could be executed while executing another statement. It will traverse function bodies of call expressions, initializers and bodies of constructors, and also destructors if they could be called.
|
If this is a CERT check, please add the corresponding aliases towards the CERT module. |
vbvictor
reviewed
Apr 2, 2025
clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.h
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/conflicting-global-accesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/ConflictingGlobalAccesses.cpp
Outdated
Show resolved
Hide resolved
ConcreteCactus
force-pushed
the
main
branch
3 times, most recently
from
6879af3 to
7b02721
Compare
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.
|
Hi Everyone, Can I get a re-review on this PR? I think I addressed most of the comments. |
vbvictor
reviewed
Jun 1, 2025
There was a problem hiding this comment.
Mostly left comments in docs about general improvements. Didn't look closely at the code, but here are some general recommendations that you should do before major review:
There are a lot of new code that is relatively hard to review, you should try to follow LLVM coding guidelines, most importantly these:
- Don’t Use Braces on Simple Single-Statement Bodies of if/else/loop Statements (many violations, with this fix the code should become smaller by line count, thus easier to review)
- Use range-based for loops wherever possible
| @@ -151,6 +157,14 @@ New checks | |||
| New check aliases | |||
| ^^^^^^^^^^^^^^^^^ | |||
|
|
|||
| - New `cert-exp30-c <cert/exp30-c>` alias for | |||
There was a problem hiding this comment.
Please create entries in docs/ folder for these aliases, they should have filling like this:
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/docs/clang-tidy/checks/cert/con54-cpp.rst
| bugprone-conflicting-global-accesses | ||
| ==================================== | ||
|
|
||
| Finds conflicting accesses on global variables. |
There was a problem hiding this comment.
"conflicting" may not be very meaningful in this description, maybe use "unsequenced" just as in compiler flag?
There was a problem hiding this comment.
Please synchronize first statement with Release Notes.
|
|
||
| For example:: | ||
|
|
||
| int a = 0; |
There was a problem hiding this comment.
use .. code-block:: c++ for c++ code-blocks in docs
| However global variables allow for more complex scenarios that | ||
| -Wunsequenced doesn't detect. E.g. :: | ||
|
|
||
| int globalVar = 0; |
| already covered by -Wunsequenced. Global unions and structs are also handled. | ||
| For example:: | ||
|
|
||
| typedef struct { |
| parameters as writes using the HandleMutableFunctionParametersAsWrites | ||
| option. For example: | ||
|
|
||
| void func(int& a); |
| int globalVar; | ||
| func(globalVar); // <- this could be a write to globalVar. | ||
|
|
||
| This option is disabled by default. |
There was a problem hiding this comment.
Suggested change
| This option is disabled by default. | |
| Default value is `false`. |
Comment on lines
+62
to
+64
| It's possible to enable handling mutable reference and pointer function | ||
| parameters as writes using the HandleMutableFunctionParametersAsWrites | ||
| option. For example: |
There was a problem hiding this comment.
This option need some rephrasing and clarification IMO.
It should be something like:
"When `true`, treat calls of functions that accept a mutable reference or a pointer as a write to the variable that was passed to the function."
I'm not totally satisfied with my wording, but I tried to eliminate this ambiguity:
What does "handling parameters as writes" mean here? Especially word "handling".
Also, give an example of some real code that doesn't get flagged without this option and gets flagged with the option enabled.
Comment on lines
+10
to
+12
| unspecified order. This checker is similar to the -Wunsequenced clang warning, | ||
| however it only looks at global variables and therefore can find conflicting | ||
| actions recursively inside functions as well. |
There was a problem hiding this comment.
I think you should not explicitly exclude cases that are covered by "-Wunsequenced". It's okay for check to have duplicate functionality with compiler flags. Moreover, for gcc there is "-Wsequence-point" and we don't know to what extent "-Wunsequenced" can find bugs, so It's better just to mention that functionality of this check may overlap with existing compiler flags such as "-Wunsequenced", "-Wsequence-point".
|
|
||
| Modifying twice or reading and modifying a memory location without a | ||
| defined sequence of the operations is either undefined behavior or has | ||
| unspecified order. This checker is similar to the -Wunsequenced clang warning, |
There was a problem hiding this comment.
Suggested change
| unspecified order. This checker is similar to the -Wunsequenced clang warning, | |
| unspecified order. This check is similar to the `-Wunsequenced` Clang warning, |
| int b = (a++) - a; // This is flagged by -Wunsequenced. | ||
|
|
||
| However global variables allow for more complex scenarios that | ||
| -Wunsequenced doesn't detect. E.g. :: |
There was a problem hiding this comment.
Suggested change
| -Wunsequenced doesn't detect. E.g. :: | |
| `-Wunsequenced` doesn't detect. E.g. :: |
| return globalVar + incFun(); // This is not detected by -Wunsequenced. | ||
| } | ||
|
|
||
| This checker attempts to detect such cases. It recurses into functions that are |
There was a problem hiding this comment.
Suggested change
| This checker attempts to detect such cases. It recurses into functions that are | |
| This check attempts to detect such cases. It recurses into functions that are |
|
|
||
| This checker attempts to detect such cases. 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. |
There was a problem hiding this comment.
Suggested change
| already covered by -Wunsequenced. Global unions and structs are also handled. | |
| already covered by `-Wunsequenced`. Global unions and structs are also handled. |
| .. option:: HandleMutableFunctionParametersAsWrites | ||
|
|
||
| It's possible to enable handling mutable reference and pointer function | ||
| parameters as writes using the HandleMutableFunctionParametersAsWrites |
There was a problem hiding this comment.
Suggested change
| parameters as writes using the HandleMutableFunctionParametersAsWrites | |
| parameters as writes using the `HandleMutableFunctionParametersAsWrites` |
| bugprone-conflicting-global-accesses | ||
| ==================================== | ||
|
|
||
| Finds conflicting accesses on global variables. |
There was a problem hiding this comment.
Please synchronize first statement with Release Notes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This checker attempts to cover the global variable case in rule EXP-30 in the SEI CERT C and rule EXP-50 in the SEI CERT C++ Coding Standards.
It recurses into functions in the same translation unit and can handle global struct and union members as well.