[NFC][clang] Fix static analyzer concerns about AUTO_CAUSES_COPY

[Manna]

Summary

Reported by Coverity:

AUTO_CAUSES_COPY
Unnecessary object copies can affect performance

1. Inside "ODRHash.cpp" file, in clang::​ODRHash::​AddCXXRecordDecl(clang::​CXXRecordDecl const *): Using the auto keyword without an & causes the copy of an object of type CXXBaseSpecifier.

2. Inside "Tokens.cpp" file, in clang::​syntax::​TokenBuffer::​dumpForTests[abi:cxx11](): Using the auto keyword without an & causes the copy of an object of type DenseMapPair.

3. Inside "TargetID.cpp" file, in clang::​getCanonicalTargetID[abi:cxx11](llvm::​StringRef, llvm::​StringMap<bool, llvm::​MallocAllocator> const &): Using the auto keyword without an & causes the copy of an object of type pair.

[aaron.ballman]

Thanks for the static analysis fixes, however, it looks like quite a few of them were false positives again. When fixing any static analysis diagnostics, please put in the effort to determine whether it's actually correct or not, as it seem the Coverity diagnostic is not really paying any attention to the size of what's being copied. If you're applying the advice from the tool without further analysis, it can be disruptive (might introduce bugs, churns the code for negative/no benefit, lowers reviewer confidence in the rest of the changes, etc). (Not suggesting you're blindly applying advice, but mostly observing we've had a few reviews like this.)

My suggestion is to always look at the size of what's being copied. As a general rule of thumb, if it's two pointers or less in size, the copy is likely cheaper than the reference and shouldn't be changed.

clang/lib/AST/ExprConstant.cpp

6117

This returns a ParamIdx object which is 32-bits, so it's not expensive to copy.

clang/lib/Basic/TargetID.cpp

136

The map iterator returns a std::pair<StringRef, bool> which is not particularly expensive to copy, but I think this is borderline enough that it's fine to keep.

clang/lib/Basic/Targets/ARM.cpp

463

This returns a StringRef which is not expensive to copy.

clang/lib/CodeGen/CGExpr.cpp

3548

This is a pair of ints, not expensive to copy.

clang/lib/Serialization/ASTReader.cpp

9426

Why is the above considered too expensive but this one is not?

clang/lib/Tooling/Syntax/Tokens.cpp

989

FileID is an int, not expensive to copy.

[tahonermann]

After seeing this review, I filed a support case with Synopsys with a request to stop reporting AUTO_CAUSES_COPY issues for small objects of class types, at least in the absence of additional evidence that a reference would be preferred.

clang/lib/AST/ODRHash.cpp

597

This seems like a good change; CXXBaseSpecifier isn't particularly large, but this seems like a case where reference semantics are more intuitive anyway.

clang/lib/Serialization/ASTReader.cpp

9426

I'm guessing that Coverity reported it as a second occurrence and those are easy to overlook.

clang/lib/Tooling/Syntax/Tokens.cpp

989

Files has type llvm::DenseMap<FileID, MarkedFile>, so the element type is DenseMapPair<FileID, MarkedFile>. MarkedFile contains a couple of vectors, so this looks like a win to me.

[steakhal]

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

1053

I think this is supposed to be just some lightweight handle: some discriminator and pointer under the hood. sizeof(I)
is just 40 bytes. Another point is that the copy construction is not customized, so the sizeof for such objects should be a good proxy for estimating how costly the copy is.

For me personally, if it fits into a cacheline (on x86_64 I think its 64 bytes) (and ~trivially-copyable) then it should've probably taken by value.
I haven't measured this theory, so take it with a pinch of salt.

[Manna]

Thank you everyone for reviews and feedbacks. I misunderstood some of the False positive Coverity cases. Thanks for the explanation and suggestions. I will keep this in mind while investigating static analysis in future. This patch removes FP cases.

[Manna]

clang/lib/Serialization/ASTReader.cpp

9426

Yes, i missed this case. I will investigate it further, so this case has been removed from the current patch.

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

1053

Thanks for the explanation!

[tahonermann]

This set of changes looks good to me.

[Manna]

@aaron.ballman, do you have any more concerns or comments? Thank you

[Manna]

In D148639#4284085, @tahonermann wrote:

This set of changes looks good to me.

Thank you @tahonermann

clang/lib/AST/ExprConstant.cpp

static bool EvaluateArgs(ArrayRef<const Expr *> Args, CallRef Call,
   llvm::SmallBitVector ForbiddenNullArgs;
   if (Callee->hasAttr<NonNullAttr>()) {
     ForbiddenNullArgs.resize(Args.size());
     for (const auto *Attr : Callee->specific_attrs<NonNullAttr>()) {
       if (!Attr->args_size()) {
         ForbiddenNullArgs.set();
         break;
       } else
-        for (auto Idx : Attr->args()) {
+        for (const auto &Idx : Attr->args()) {
           unsigned ASTIdx = Idx.getASTIndex();
           if (ASTIdx >= Args.size())
             continue;
           ForbiddenNullArgs[ASTIdx] = true;
         }
     }
   }
   for (unsigned I = 0; I < Args.size(); I++) {

clang/lib/AST/ODRHash.cpp

void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) {
   const ClassTemplateDecl *TD = Record->getDescribedClassTemplate();
   AddBoolean(TD);
   if (TD) {
     AddTemplateParameterList(TD->getTemplateParameters());
   }
 
   ID.AddInteger(Record->getNumBases());
   auto Bases = Record->bases();
-  for (auto Base : Bases) {
+  for (const auto &Base : Bases) {
     AddQualType(Base.getType());
     ID.AddInteger(Base.isVirtual());
     ID.AddInteger(Base.getAccessSpecifierAsWritten());
   }
 }
 
 void ODRHash::AddRecordDecl(const RecordDecl *Record) {
   assert(!isa<CXXRecordDecl>(Record) &&

clang/lib/Basic/TargetID.cpp

 // A canonical target ID is a target ID containing a canonical processor name
 // and features in alphabetical order.
 std::string getCanonicalTargetID(llvm::StringRef Processor,
                                  const llvm::StringMap<bool> &Features) {
   std::string TargetID = Processor.str();
   std::map<const llvm::StringRef, bool> OrderedMap;
   for (const auto &F : Features)
     OrderedMap[F.first()] = F.second;
-  for (auto F : OrderedMap)
+  for (const auto &F : OrderedMap)
     TargetID = TargetID + ':' + F.first.str() + (F.second ? "+" : "-");
   return TargetID;
 }
 
 // For a specific processor, a feature either shows up in all target IDs, or
 // does not show up in any target IDs. Otherwise the target ID combination
 // is invalid.
 std::optional<std::pair<llvm::StringRef, llvm::StringRef>>

clang/lib/Basic/Targets/ARM.cpp

bool ARMTargetInfo::initFeatureMap(
   // get default FPU features
   llvm::ARM::FPUKind FPUKind = llvm::ARM::getDefaultFPU(CPU, Arch);
   llvm::ARM::getFPUFeatures(FPUKind, TargetFeatures);
 
   // get default Extension features
   uint64_t Extensions = llvm::ARM::getDefaultExtensions(CPU, Arch);
   llvm::ARM::getExtensionFeatures(Extensions, TargetFeatures);
 
-  for (auto Feature : TargetFeatures)
+  for (const auto &Feature : TargetFeatures)
     if (Feature[0] == '+')
       Features[Feature.drop_front(1)] = true;
 
   // Enable or disable thumb-mode explicitly per function to enable mixed
   // ARM and Thumb code generation.
   if (isThumb())
     Features["thumb-mode"] = true;
   else

clang/lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitCfiCheckFail() {
   const std::pair<int, SanitizerMask> CheckKinds[] = {
       {CFITCK_VCall, SanitizerKind::CFIVCall},
       {CFITCK_NVCall, SanitizerKind::CFINVCall},
       {CFITCK_DerivedCast, SanitizerKind::CFIDerivedCast},
       {CFITCK_UnrelatedCast, SanitizerKind::CFIUnrelatedCast},
       {CFITCK_ICall, SanitizerKind::CFIICall}};
 
   SmallVector<std::pair<llvm::Value *, SanitizerMask>, 5> Checks;
-  for (auto CheckKindMaskPair : CheckKinds) {
+  for (const auto &CheckKindMaskPair : CheckKinds) {
     int Kind = CheckKindMaskPair.first;
     SanitizerMask Mask = CheckKindMaskPair.second;
     llvm::Value *Cond =
         Builder.CreateICmpNE(CheckKind, llvm::ConstantInt::get(Int8Ty, Kind));
     if (CGM.getLangOpts().Sanitize.has(Mask))
       EmitCheck(std::make_pair(Cond, Mask), SanitizerHandler::CFICheckFail, {},
                 {Data, Addr, ValidVtable});
     else

clang/lib/CodeGen/CGVTables.cpp

if (S1 < S2)
       return true;
     if (S1 != S2)
       return false;
 
     return AP1.second < AP2.second;
   });
 
   ArrayRef<VTableComponent> Comps = VTLayout.vtable_components();
-  for (auto AP : AddressPoints) {
+  for (const auto &AP : AddressPoints) {
     // Create type metadata for the address point.
     AddVTableTypeMetadata(VTable, ComponentWidth * AP.second, AP.first);
 
     // The class associated with each address point could also potentially be
     // used for indirect calls via a member function pointer, so we need to
     // annotate the address of each function pointer with the appropriate member
     // function pointer type.
     for (unsigned I = 0; I != Comps.size(); ++I) {

clang/lib/Sema/SemaInit.cpp

struct RevertToOldSizeRAII {
   ~RevertToOldSizeRAII() { Path.resize(OldSize); }
 };
 
 using LocalVisitor = llvm::function_ref<bool(IndirectLocalPath &Path, Local L,
                                              ReferenceKind RK)>;
 }
 
 static bool isVarOnPath(IndirectLocalPath &Path, VarDecl *VD) {
-  for (auto E : Path)
+  for (const auto &E : Path)
     if (E.Kind == IndirectLocalPathEntry::VarInit && E.D == VD)
       return true;
   return false;
 }
 
 static bool pathContainsInit(IndirectLocalPath &Path) {
   return llvm::any_of(Path, [=](IndirectLocalPathEntry E) {
     return E.Kind == IndirectLocalPathEntry::DefaultInit ||

clang/lib/Serialization/ASTReader.cpp

while (!PendingObjCExtensionIvarRedeclarations.empty()) {
       StructuralEquivalenceContext Ctx(
           ExtensionsPair.first->getASTContext(),
           ExtensionsPair.second->getASTContext(), NonEquivalentDecls,
           StructuralEquivalenceKind::Default, /*StrictTypeSpelling =*/false,
           /*Complain =*/false,
           /*ErrorOnTagTypeMismatch =*/true);
       if (Ctx.IsEquivalent(ExtensionsPair.first, ExtensionsPair.second)) {
         // Merge redeclared ivars with their predecessors.
-        for (auto IvarPair : DuplicateIvars) {
+        for (const auto &IvarPair : DuplicateIvars) {
           ObjCIvarDecl *Ivar = IvarPair.first, *PrevIvar = IvarPair.second;
           // Change semantic DeclContext but keep the lexical one.
           Ivar->setDeclContextsImpl(PrevIvar->getDeclContext(),
                                     Ivar->getLexicalDeclContext(),
                                     getContext());
           getContext().setPrimaryMergedDecl(Ivar, PrevIvar->getCanonicalDecl());
         }
         // Invalidate duplicate extension and the cached ivar list.
         ExtensionsPair.first->setInvalidDecl();
         ExtensionsPair.second->getClassInterface()
             ->getDefinition()
             ->setIvarList(nullptr);
       } else {
         for (auto IvarPair : DuplicateIvars) {
           Diag(IvarPair.first->getLocation(),
                diag::err_duplicate_ivar_declaration)
               << IvarPair.first->getIdentifier();
           Diag(IvarPair.second->getLocation(), diag::note_previous_definition);
         }
       }
       PendingObjCExtensionIvarRedeclarations.pop_back();
     }

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

if (!ReferenceStmt) {
     assert(K == ProgramPoint::PostStmtPurgeDeadSymbolsKind &&
            "Use PostStmtPurgeDeadSymbolsKind for clearing a LocationContext");
     LC = LC->getParent();
   }
 
   const StackFrameContext *SFC = LC ? LC->getStackFrame() : nullptr;
   SymbolReaper SymReaper(SFC, ReferenceStmt, SymMgr, getStoreManager());
 
-  for (auto I : CleanedState->get<ObjectsUnderConstruction>()) {
+  for (const auto &I : CleanedState->get<ObjectsUnderConstruction>()) {
     if (SymbolRef Sym = I.second.getAsSymbol())
       SymReaper.markLive(Sym);
     if (const MemRegion *MR = I.second.getAsRegion())
       SymReaper.markLive(MR);
   }
 
   getCheckerManager().runCheckersForLiveSymbols(CleanedState, SymReaper);
 

clang/lib/Tooling/Syntax/Tokens.cpp

std::string TokenBuffer::dumpForTests() const {
 
   OS << "expanded tokens:\n"
      << "  ";
   // (!) we do not show '<eof>'.
   DumpTokens(OS, llvm::ArrayRef(ExpandedTokens).drop_back());
   OS << "\n";
 
   std::vector<FileID> Keys;
-  for (auto F : Files)
+  for (const auto &F : Files)
     Keys.push_back(F.first);
   llvm::sort(Keys);
 
   for (FileID ID : Keys) {
     const MarkedFile &File = Files.find(ID)->second;
     auto *Entry = SourceMgr->getFileEntryForID(ID);
     if (!Entry)
       continue; // Skip builtin files.