Merge branch 'main' into fix/111854

Author: a-tarasyuk

clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst

@@ -76,6 +76,16 @@ For example:
      }
    }
 
+Exception: accessor methods
+```````````````````````````
+
+The check assumes *accessor* methods of a class are stable, with a heuristic to
+determine which methods are accessors. Specifically, parameter-free ``const``
+methods are treated as accessors. Note that this is not guaranteed to be safe
+-- but, it is widely used (safely) in practice, and so we have chosen to treat
+it as generally safe. Calls to non ``const`` methods are assumed to modify
+the state of the object and affect the stability of earlier accessor calls.
+
 Rely on invariants of uncommon APIs
 -----------------------------------
 

clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h

@@ -17,6 +17,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/NoopLattice.h"
@@ -39,23 +40,28 @@ struct UncheckedOptionalAccessModelOptions {
   bool IgnoreSmartPointerDereference = false;
 };
 
+using UncheckedOptionalAccessLattice = CachedConstAccessorsLattice<NoopLattice>;
+
 /// Dataflow analysis that models whether optionals hold values or not.
 ///
 /// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
 class UncheckedOptionalAccessModel
-    : public DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice> {
+    : public DataflowAnalysis<UncheckedOptionalAccessModel,
+                              UncheckedOptionalAccessLattice> {
 public:
   UncheckedOptionalAccessModel(ASTContext &Ctx, dataflow::Environment &Env);
 
   /// Returns a matcher for the optional classes covered by this model.
   static ast_matchers::DeclarationMatcher optionalClassDecl();
 
-  static NoopLattice initialElement() { return {}; }
+  static UncheckedOptionalAccessLattice initialElement() { return {}; }
 
-  void transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env);
+  void transfer(const CFGElement &Elt, UncheckedOptionalAccessLattice &L,
+                Environment &Env);
 
 private:
-  CFGMatchSwitch<TransferState<NoopLattice>> TransferMatchSwitch;
+  CFGMatchSwitch<TransferState<UncheckedOptionalAccessLattice>>
+      TransferMatchSwitch;
 };
 
 class UncheckedOptionalAccessDiagnoser {
@@ -65,7 +71,8 @@ class UncheckedOptionalAccessDiagnoser {
 
   llvm::SmallVector<SourceLocation>
   operator()(const CFGElement &Elt, ASTContext &Ctx,
-             const TransferStateForDiagnostics<NoopLattice> &State) {
+             const TransferStateForDiagnostics<UncheckedOptionalAccessLattice>
+                 &State) {
     return DiagnoseMatchSwitch(Elt, Ctx, State.Env);
   }
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -10188,7 +10188,7 @@ def warn_new_dangling_initializer_list : Warning<
   "will be destroyed at the end of the full-expression">,
   InGroup<DanglingInitializerList>;
 def warn_dangling_pointer_assignment : Warning<
-   "object backing the pointer %0 "
+   "object backing %select{|the pointer }0%1 "
    "will be destroyed at the end of the full-expression">,
    InGroup<DanglingAssignment>;
 

clang/include/clang/Basic/Module.h

@@ -48,6 +48,7 @@ namespace clang {
 
 class FileManager;
 class LangOptions;
+class ModuleMap;
 class TargetInfo;
 
 /// Describes the name of a module.
@@ -99,6 +100,15 @@ struct ASTFileSignature : std::array<uint8_t, 20> {
   }
 };
 
+/// Required to construct a Module.
+///
+/// This tag type is only constructible by ModuleMap, guaranteeing it ownership
+/// of all Module instances.
+class ModuleConstructorTag {
+  explicit ModuleConstructorTag() = default;
+  friend ModuleMap;
+};
+
 /// Describes a module or submodule.
 ///
 /// Aligned to 8 bytes to allow for llvm::PointerIntPair<Module *, 3>.
@@ -497,8 +507,9 @@ class alignas(8) Module {
   std::vector<Conflict> Conflicts;
 
   /// Construct a new module or submodule.
-  Module(StringRef Name, SourceLocation DefinitionLoc, Module *Parent,
-         bool IsFramework, bool IsExplicit, unsigned VisibilityID);
+  Module(ModuleConstructorTag, StringRef Name, SourceLocation DefinitionLoc,
+         Module *Parent, bool IsFramework, bool IsExplicit,
+         unsigned VisibilityID);
 
   ~Module();
 
@@ -749,7 +760,6 @@ class alignas(8) Module {
   ///
   /// \returns The submodule if found, or NULL otherwise.
   Module *findSubmodule(StringRef Name) const;
-  Module *findOrInferSubmodule(StringRef Name);
 
   /// Get the Global Module Fragment (sub-module) for this module, it there is
   /// one.

clang/include/clang/Lex/ModuleMap.h

@@ -93,9 +93,12 @@ class ModuleMap {
   /// named LangOpts::CurrentModule, if we've loaded it).
   Module *SourceModule = nullptr;
 
+  /// The allocator for all (sub)modules.
+  llvm::SpecificBumpPtrAllocator<Module> ModulesAlloc;
+
   /// Submodules of the current module that have not yet been attached to it.
-  /// (Ownership is transferred if/when we create an enclosing module.)
-  llvm::SmallVector<std::unique_ptr<Module>, 8> PendingSubmodules;
+  /// (Relationship is set up if/when we create an enclosing module.)
+  llvm::SmallVector<Module *, 8> PendingSubmodules;
 
   /// The top-level modules that are known.
   llvm::StringMap<Module *> Modules;
@@ -502,6 +505,8 @@ class ModuleMap {
   /// \returns The named module, if known; otherwise, returns null.
   Module *findModule(StringRef Name) const;
 
+  Module *findOrInferSubmodule(Module *Parent, StringRef Name);
+
   /// Retrieve a module with the given name using lexical name lookup,
   /// starting at the given context.
   ///

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

@@ -17,13 +17,14 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchersMacros.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/Formula.h"
-#include "clang/Analysis/FlowSensitive/NoopLattice.h"
+#include "clang/Analysis/FlowSensitive/RecordOps.h"
 #include "clang/Analysis/FlowSensitive/StorageLocation.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/SourceLocation.h"
@@ -104,10 +105,17 @@ static const CXXRecordDecl *getOptionalBaseClass(const CXXRecordDecl *RD) {
   return nullptr;
 }
 
+static bool isSupportedOptionalType(QualType Ty) {
+  const CXXRecordDecl *Optional =
+      getOptionalBaseClass(Ty->getAsCXXRecordDecl());
+  return Optional != nullptr;
+}
+
 namespace {
 
 using namespace ::clang::ast_matchers;
-using LatticeTransferState = TransferState<NoopLattice>;
+
+using LatticeTransferState = TransferState<UncheckedOptionalAccessLattice>;
 
 AST_MATCHER(CXXRecordDecl, optionalClass) { return hasOptionalClassName(Node); }
 
@@ -325,6 +333,19 @@ auto isValueOrNotEqX() {
                                ComparesToSame(integerLiteral(equals(0)))));
 }
 
+auto isZeroParamConstMemberCall() {
+  return cxxMemberCallExpr(
+      callee(cxxMethodDecl(parameterCountIs(0), isConst())));
+}
+
+auto isNonConstMemberCall() {
+  return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst()))));
+}
+
+auto isNonConstMemberOperatorCall() {
+  return cxxOperatorCallExpr(callee(cxxMethodDecl(unless(isConst()))));
+}
+
 auto isCallReturningOptional() {
   return callExpr(hasType(qualType(
       anyOf(desugarsToOptionalOrDerivedType(),
@@ -523,6 +544,99 @@ void transferCallReturningOptional(const CallExpr *E,
   setHasValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
 }
 
+void handleConstMemberCall(const CallExpr *CE,
+                           dataflow::RecordStorageLocation *RecordLoc,
+                           const MatchFinder::MatchResult &Result,
+                           LatticeTransferState &State) {
+  // If the const method returns an optional or reference to an optional.
+  if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) {
+    StorageLocation *Loc =
+        State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+            *RecordLoc, CE, State.Env, [&](StorageLocation &Loc) {
+              setHasValue(cast<RecordStorageLocation>(Loc),
+                          State.Env.makeAtomicBoolValue(), State.Env);
+            });
+    if (Loc == nullptr)
+      return;
+    if (CE->isGLValue()) {
+      // If the call to the const method returns a reference to an optional,
+      // link the call expression to the cached StorageLocation.
+      State.Env.setStorageLocation(*CE, *Loc);
+    } else {
+      // If the call to the const method returns an optional by value, we
+      // need to use CopyRecord to link the optional to the result object
+      // of the call expression.
+      auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
+      copyRecord(*cast<RecordStorageLocation>(Loc), ResultLoc, State.Env);
+    }
+    return;
+  }
+
+  // Cache if the const method returns a boolean type.
+  // We may decide to cache other return types in the future.
+  if (RecordLoc != nullptr && CE->getType()->isBooleanType()) {
+    Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc, CE,
+                                                                 State.Env);
+    if (Val == nullptr)
+      return;
+    State.Env.setValue(*CE, *Val);
+    return;
+  }
+
+  // Perform default handling if the call returns an optional
+  // but wasn't handled above (if RecordLoc is nullptr).
+  if (isSupportedOptionalType(CE->getType())) {
+    transferCallReturningOptional(CE, Result, State);
+  }
+}
+
+void transferValue_ConstMemberCall(const CXXMemberCallExpr *MCE,
+                                   const MatchFinder::MatchResult &Result,
+                                   LatticeTransferState &State) {
+  handleConstMemberCall(
+      MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
+}
+
+void handleNonConstMemberCall(const CallExpr *CE,
+                              dataflow::RecordStorageLocation *RecordLoc,
+                              const MatchFinder::MatchResult &Result,
+                              LatticeTransferState &State) {
+  // When a non-const member function is called, reset some state.
+  if (RecordLoc != nullptr) {
+    for (const auto &[Field, FieldLoc] : RecordLoc->children()) {
+      if (isSupportedOptionalType(Field->getType())) {
+        auto *FieldRecordLoc = cast_or_null<RecordStorageLocation>(FieldLoc);
+        if (FieldRecordLoc) {
+          setHasValue(*FieldRecordLoc, State.Env.makeAtomicBoolValue(),
+                      State.Env);
+        }
+      }
+    }
+    State.Lattice.clearConstMethodReturnValues(*RecordLoc);
+    State.Lattice.clearConstMethodReturnStorageLocations(*RecordLoc);
+  }
+
+  // Perform default handling if the call returns an optional.
+  if (isSupportedOptionalType(CE->getType())) {
+    transferCallReturningOptional(CE, Result, State);
+  }
+}
+
+void transferValue_NonConstMemberCall(const CXXMemberCallExpr *MCE,
+                                      const MatchFinder::MatchResult &Result,
+                                      LatticeTransferState &State) {
+  handleNonConstMemberCall(
+      MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result, State);
+}
+
+void transferValue_NonConstMemberOperatorCall(
+    const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
+    LatticeTransferState &State) {
+  auto *RecordLoc = cast_or_null<dataflow::RecordStorageLocation>(
+      State.Env.getStorageLocation(*OCE->getArg(0)));
+  handleNonConstMemberCall(OCE, RecordLoc, Result, State);
+}
+
 void constructOptionalValue(const Expr &E, Environment &Env,
                             BoolValue &HasValueVal) {
   RecordStorageLocation &Loc = Env.getResultObjectLocation(E);
@@ -899,7 +1013,17 @@ auto buildTransferMatchSwitch() {
             transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env);
           })
 
-      // returns optional
+      // const accessor calls
+      .CaseOfCFGStmt<CXXMemberCallExpr>(isZeroParamConstMemberCall(),
+                                        transferValue_ConstMemberCall)
+      // non-const member calls that may modify the state of an object.
+      .CaseOfCFGStmt<CXXMemberCallExpr>(isNonConstMemberCall(),
+                                        transferValue_NonConstMemberCall)
+      .CaseOfCFGStmt<CXXOperatorCallExpr>(
+          isNonConstMemberOperatorCall(),
+          transferValue_NonConstMemberOperatorCall)
+
+      // other cases of returning optional
       .CaseOfCFGStmt<CallExpr>(isCallReturningOptional(),
                                transferCallReturningOptional)
 
@@ -958,7 +1082,8 @@ UncheckedOptionalAccessModel::optionalClassDecl() {
 
 UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx,
                                                            Environment &Env)
-    : DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice>(Ctx),
+    : DataflowAnalysis<UncheckedOptionalAccessModel,
+                       UncheckedOptionalAccessLattice>(Ctx),
       TransferMatchSwitch(buildTransferMatchSwitch()) {
   Env.getDataflowAnalysisContext().setSyntheticFieldCallback(
       [&Ctx](QualType Ty) -> llvm::StringMap<QualType> {
@@ -972,7 +1097,8 @@ UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx,
 }
 
 void UncheckedOptionalAccessModel::transfer(const CFGElement &Elt,
-                                            NoopLattice &L, Environment &Env) {
+                                            UncheckedOptionalAccessLattice &L,
+                                            Environment &Env) {
   LatticeTransferState State(L, Env);
   TransferMatchSwitch(Elt, getASTContext(), State);
 }

clang/lib/Basic/Module.cpp

@@ -34,8 +34,9 @@
 
 using namespace clang;
 
-Module::Module(StringRef Name, SourceLocation DefinitionLoc, Module *Parent,
-               bool IsFramework, bool IsExplicit, unsigned VisibilityID)
+Module::Module(ModuleConstructorTag, StringRef Name,
+               SourceLocation DefinitionLoc, Module *Parent, bool IsFramework,
+               bool IsExplicit, unsigned VisibilityID)
     : Name(Name), DefinitionLoc(DefinitionLoc), Parent(Parent),
       VisibilityID(VisibilityID), IsUnimportable(false),
       HasIncompatibleModuleFile(false), IsAvailable(true),
@@ -58,11 +59,7 @@ Module::Module(StringRef Name, SourceLocation DefinitionLoc, Module *Parent,
   }
 }
 
-Module::~Module() {
-  for (auto *Submodule : SubModules) {
-    delete Submodule;
-  }
-}
+Module::~Module() = default;
 
 static bool isPlatformEnvironment(const TargetInfo &Target, StringRef Feature) {
   StringRef Platform = Target.getPlatformName();
@@ -361,21 +358,6 @@ Module *Module::findSubmodule(StringRef Name) const {
   return SubModules[Pos->getValue()];
 }
 
-Module *Module::findOrInferSubmodule(StringRef Name) {
-  llvm::StringMap<unsigned>::const_iterator Pos = SubModuleIndex.find(Name);
-  if (Pos != SubModuleIndex.end())
-    return SubModules[Pos->getValue()];
-  if (!InferSubmodules)
-    return nullptr;
-  Module *Result = new Module(Name, SourceLocation(), this, false, InferExplicitSubmodules, 0);
-  Result->InferExplicitSubmodules = InferExplicitSubmodules;
-  Result->InferSubmodules = InferSubmodules;
-  Result->InferExportWildcard = InferExportWildcard;
-  if (Result->InferExportWildcard)
-    Result->Exports.push_back(Module::ExportDecl(nullptr, true));
-  return Result;
-}
-
 Module *Module::getGlobalModuleFragment() const {
   assert(isNamedModuleUnit() && "We should only query the global module "
                                 "fragment from the C++20 Named modules");

clang/lib/Basic/Targets/AMDGPU.cpp

@@ -260,9 +260,9 @@ AMDGPUTargetInfo::AMDGPUTargetInfo(const llvm::Triple &Triple,
 void AMDGPUTargetInfo::adjust(DiagnosticsEngine &Diags, LangOptions &Opts) {
   TargetInfo::adjust(Diags, Opts);
   // ToDo: There are still a few places using default address space as private
-  // address space in OpenCL, which needs to be cleaned up, then Opts.OpenCL
-  // can be removed from the following line.
-  setAddressSpaceMap(/*DefaultIsPrivate=*/Opts.OpenCL ||
+  // address space in OpenCL, which needs to be cleaned up, then the references
+  // to OpenCL can be removed from the following line.
+  setAddressSpaceMap((Opts.OpenCL && !Opts.OpenCLGenericAddressSpace) ||
                      !isAMDGCN(getTriple()));
 }