[analyzer][ctu] Only import const and trivial VarDecls

[martong]

Summary

Do import the definition of objects from a foreign translation unit if that's type is const and trivial.

[steakhal]

Please add a test case where the class is trivial but the global variable of such is non-const, thus the initialized expression is not imported.

clang/include/clang/CrossTU/CrossTranslationUnit.h

112–115

definition

clang/lib/CrossTU/CrossTranslationUnit.cpp

224

extra blank line

clang/test/Analysis/Inputs/ctu-other.cpp

129

AFAIK you don't need the extern here, since this is the definition of that global, not only a declaration.
Similarly applies to the rest of the cases.

[martong]

In D122805#3419129, @steakhal wrote:

Please add a test case where the class is trivial but the global variable of such is non-const, thus the initialized expression is not imported.

I've added a new case, but that is not very meaningful, because clang-extdef-mapping is not capable of emitting the USR for the non-const variable.

clang/include/clang/CrossTU/CrossTranslationUnit.h

112–115

thx, fixed.

clang/lib/CrossTU/CrossTranslationUnit.cpp

224

Thanks!

clang/test/Analysis/Inputs/ctu-other.cpp

129

Actually, linkage of const variable definitions are internal in C++, unless the extern modifier is provided. So, we need it.

If it was a non-const then the definition's linkage would be external.

[martong]

• Remove blank line
• Fix comment
• Add new check

[steakhal]

In D122805#3419662, @martong wrote:

In D122805#3419129, @steakhal wrote:

Please add a test case where the class is trivial but the global variable of such is non-const, thus the initialized expression is not imported.

I've added a new case, but that is not very meaningful, because clang-extdef-mapping is not capable of emitting the USR for the non-const variable.

What do you mean by not capable?

[martong]

In D122805#3420111, @steakhal wrote:

In D122805#3419662, @martong wrote:

In D122805#3419129, @steakhal wrote:

Please add a test case where the class is trivial but the global variable of such is non-const, thus the initialized expression is not imported.

I've added a new case, but that is not very meaningful, because clang-extdef-mapping is not capable of emitting the USR for the non-const variable.

What do you mean by not capable?

It uses cross_tu::shouldImport() which returns true only for const declarations. So, having a hand-written extDevMapping.txt that contains an USR for a non-const is not that meaningful, imho.

[martong]

In D122805#3421676, @martong wrote:

In D122805#3420111, @steakhal wrote:

In D122805#3419662, @martong wrote:

In D122805#3419129, @steakhal wrote:

Please add a test case where the class is trivial but the global variable of such is non-const, thus the initialized expression is not imported.

I've added a new case, but that is not very meaningful, because clang-extdef-mapping is not capable of emitting the USR for the non-const variable.

What do you mean by not capable?

It uses cross_tu::shouldImport() which returns true only for const declarations. So, having a hand-written extDevMapping.txt that contains an USR for a non-const is not that meaningful, imho.

I changed my mind: we can test one side of the logic if we do add extNonConstS to the externalDefMap.txt. So, just did that.

[martong]

• Add extNonConstS to the externalDefMap.txt

[steakhal]

In D122805#3421746, @martong wrote:

In D122805#3421676, @martong wrote:

In D122805#3420111, @steakhal wrote:

In D122805#3419662, @martong wrote:

In D122805#3419129, @steakhal wrote:

Please add a test case where the class is trivial but the global variable of such is non-const, thus the initialized expression is not imported.

I've added a new case, but that is not very meaningful, because clang-extdef-mapping is not capable of emitting the USR for the non-const variable.

What do you mean by not capable?

It uses cross_tu::shouldImport() which returns true only for const declarations. So, having a hand-written extDevMapping.txt that contains an USR for a non-const is not that meaningful, IMHO.

What I wanted to emphasize is that previously for all const globals it returned true, unlike now when we only return true if it's const AND trivial.
This test case would demonstrate the behavioral change and would nicely underpin the reason behind this patch.

In D122805#3421746, @martong wrote:

I changed my mind: we can test one side of the logic if we do add extNonConstS to the externalDefMap.txt. So, just did that.

I don't think it's necessary, I would rather see an on-the-fly generated externalDefMap.txt to enforce that this is supposed to work all the time, even if something changes in the extdefmap tool.
Forcefully putting entries into that map is sortof like introducing dead-code, I'm not on board with that.

[martong]

In D122805#3421802, @steakhal wrote:

In D122805#3421746, @martong wrote:

In D122805#3421676, @martong wrote:

In D122805#3420111, @steakhal wrote:

In D122805#3419662, @martong wrote:

In D122805#3419129, @steakhal wrote:

Please add a test case where the class is trivial but the global variable of such is non-const, thus the initialized expression is not imported.

I've added a new case, but that is not very meaningful, because clang-extdef-mapping is not capable of emitting the USR for the non-const variable.

What do you mean by not capable?

It uses cross_tu::shouldImport() which returns true only for const declarations. So, having a hand-written extDevMapping.txt that contains an USR for a non-const is not that meaningful, IMHO.

What I wanted to emphasize is that previously for all const globals it returned true, unlike now when we only return true if it's const AND trivial.
This test case would demonstrate the behavioral change and would nicely underpin the reason behind this patch.

In D122805#3421746, @martong wrote:

I changed my mind: we can test one side of the logic if we do add extNonConstS to the externalDefMap.txt. So, just did that.

I don't think it's necessary, I would rather see an on-the-fly generated externalDefMap.txt to enforce that this is supposed to work all the time, even if something changes in the extdefmap tool.
Forcefully putting entries into that map is sortof like introducing dead-code, I'm not on board with that.

Currently ALL ctu tests are done with hand-written externalDefMap.txts. I agree it would be nice to generate them on-the-fly, but that should be the subject of another change. And if we do that then we will not have extNonConstS in the extdef file for this test which makes it impossible to test what you request. Please be consistent with your review.

[martong]

even if something changes in the extdefmap tool

We already have a separate test file for that. That is func-mapping-test.cpp.

[martong]

• Rebase to llvm/main

[martong]

• remove extNonConstS from the externalDefMap.txt

[steakhal]

LGTM

clang/include/clang/CrossTU/CrossTranslationUnit.h

 /// The format is expected to be a mapping from from absolute source file
 /// paths in the filesystem to a list of command-line parts, which
 /// constitute the invocation needed to compile that file. That invocation
 /// will be used to produce the AST of the TU.
 llvm::Expected<InvocationListTy> parseInvocationList(
     StringRef FileContent,
     llvm::sys::path::Style PathStyle = llvm::sys::path::Style::posix);
 
-// Returns true if the variable or any field of a record variable is const.
-bool containsConst(const VarDecl *VD, const ASTContext &ACtx);
+/// Returns true if it makes sense to import a foreign variable definition.
+/// For instance, we don't want to import variables that have non-trivial types
+/// because the constructor might have side-effects.
+bool shouldImport(const VarDecl *VD, const ASTContext &ACtx);
 
 /// This class is used for tools that requires cross translation
 ///        unit capability.
 ///
 /// This class can load definitions from external AST sources.
 /// The loaded definition will be merged back to the original AST using the
 /// AST Importer.
 /// In order to use this class, an index file is required that describes

clang/lib/CrossTU/CrossTranslationUnit.cpp

 createCrossTUIndexString(const llvm::StringMap<std::string> &Index) {
   std::ostringstream Result;
   for (const auto &E : Index)
     Result << E.getKey().size() << ':' << E.getKey().str() << ' '
            << E.getValue() << '\n';
   return Result.str();
 }
 
-bool containsConst(const VarDecl *VD, const ASTContext &ACtx) {
+bool shouldImport(const VarDecl *VD, const ASTContext &ACtx) {
   CanQualType CT = ACtx.getCanonicalType(VD->getType());
-  if (!CT.isConstQualified()) {
-    const RecordType *RTy = CT->getAs<RecordType>();
-    if (!RTy || !RTy->hasConstFields())
-      return false;
-  }
-  return true;
+  return CT.isConstQualified() && VD->getType().isTrivialType(ACtx);
 }
 
 static bool hasBodyOrInit(const FunctionDecl *D, const FunctionDecl *&DefD) {
   return D->hasBody(DefD);
 }
 static bool hasBodyOrInit(const VarDecl *D, const VarDecl *&DefD) {
   return D->getAnyInitializer(DefD);
 }

clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

bool VisitDecl(Decl *D) {
     return true;
   }
 
   bool VisitVarDecl(VarDecl *VD) {
     if (!Opts->IsNaiveCTUEnabled)
       return true;
 
     if (VD->hasExternalStorage() || VD->isStaticDataMember()) {
-      if (!cross_tu::containsConst(VD, *Ctx))
+      if (!cross_tu::shouldImport(VD, *Ctx))
         return true;
     } else {
       // Cannot be initialized in another TU.
       return true;
     }
 
     if (VD->getAnyInitializer())
       return true;

clang/test/Analysis/Inputs/ctu-other.cpp

 extern const int extInt = 2;
 namespace intns {
 extern const int extInt = 3;
 }
 struct S {
   int a;
 };
 extern const S extS = {.a = 4};
+extern S extNonConstS = {.a = 4};
+struct NonTrivialS {
+  int a;
+  ~NonTrivialS();
+};
+extern const NonTrivialS extNTS = {.a = 4};
 struct A {
   static const int a;
 };
 const int A::a = 3;
 struct SC {
   const int a;
 };
-SC extSC = {.a = 8};
+extern const SC extSC = {.a = 8};
 struct ST {
-  static struct SC sc;
+  static const struct SC sc;
 };
-struct SC ST::sc = {.a = 2};
+const struct SC ST::sc = {.a = 2};
 struct SCNest {
   struct SCN {
     const int a;
   } scn;
 };
 SCNest extSCN = {.scn = {.a = 9}};
-SCNest::SCN extSubSCN = {.a = 1};
+extern SCNest::SCN const extSubSCN = {.a = 1};
 struct SCC {
   SCC(int c) : a(c) {}
   const int a;
 };
 SCC extSCC{7};
 union U {
   const int a;
   const unsigned int b;
 };
-U extU = {.a = 4};
+extern const U extU = {.a = 4};
 
 class TestAnonUnionUSR {
 public:
   inline float f(int value) {
     union {
       float f;
       int i;
     };

clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.ast-dump.txt

 15:c:@F@h_chain#I# ctu-chain.cpp.ast
 27:c:@N@chns@S@chcls@F@chf4#I# ctu-chain.cpp.ast
 19:c:@N@chns@F@chf2#I# ctu-chain.cpp.ast
 29:c:@F@fun_using_anon_struct#I# ctu-other.cpp.ast
 24:c:@F@other_macro_diag#I# ctu-other.cpp.ast
 9:c:@extInt ctu-other.cpp.ast
 17:c:@N@intns@extInt ctu-other.cpp.ast
 7:c:@extS ctu-other.cpp.ast
+15:c:@extNonConstS ctu-other.cpp.ast
+9:c:@extNTS ctu-other.cpp.ast
 8:c:@S@A@a ctu-other.cpp.ast
 8:c:@extSC ctu-other.cpp.ast
 10:c:@S@ST@sc ctu-other.cpp.ast
 9:c:@extSCN ctu-other.cpp.ast
 12:c:@extSubSCN ctu-other.cpp.ast
 9:c:@extSCC ctu-other.cpp.ast
 7:c:@extU ctu-other.cpp.ast
 26:c:@S@TestAnonUnionUSR@Test ctu-other.cpp.ast
 53:c:@F@testImportOfIncompleteDefaultParmDuringImport#I# ctu-other.cpp.ast
 39:c:@F@testImportOfDelegateConstructor#I# ctu-other.cpp.ast

clang/test/Analysis/ctu-main.cpp

 extern const int extInt;
 namespace intns {
 extern const int extInt;
 }
 struct S {
   int a;
 };
 extern const S extS;
+extern S extNonConstS;
+struct NonTrivialS {
+  int a;
+  // User declaring a dtor makes it non-trivial.
+  ~NonTrivialS();
+};
+extern const NonTrivialS extNTS;
 extern const int extHere;
 const int extHere = 6;
 struct A {
   static const int a;
 };
 struct SC {
   const int a;
 };
-extern SC extSC;
+extern const SC extSC;
 struct ST {
-  static struct SC sc;
+  static const struct SC sc;
 };
 struct SCNest {
   struct SCN {
     const int a;
   } scn;
 };
 extern SCNest extSCN;
-extern SCNest::SCN extSubSCN;
+extern const SCNest::SCN extSubSCN;
 struct SCC {
   SCC(int c);
   const int a;
 };
 extern SCC extSCC;
 union U {
   const int a;
   const unsigned int b;
 };
-extern U extU;
+extern const U extU;
 
 void test_virtual_functions(mycls* obj) {
   // The dynamic type is known.
   clang_analyzer_eval(mycls().fvcl(1) == 8);   // expected-warning{{TRUE}}
   clang_analyzer_eval(derived().fvcl(1) == 9); // expected-warning{{TRUE}}
   // We cannot decide about the dynamic type.
   clang_analyzer_eval(obj->fvcl(1) == 8);      // expected-warning{{FALSE}} expected-warning{{TRUE}}
 }
int main() {
 
   clang_analyzer_eval(other_macro_diag(1) == 1); // expected-warning{{TRUE}}
   // expected-warning@Inputs/ctu-other.cpp:93{{REACHABLE}}
   MACRODIAG(); // expected-warning{{REACHABLE}}
 
   clang_analyzer_eval(extInt == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(intns::extInt == 3); // expected-warning{{TRUE}}
   clang_analyzer_eval(extS.a == 4); // expected-warning{{TRUE}}
+  clang_analyzer_eval(extNonConstS.a == 4); // expected-warning{{TRUE}} expected-warning{{FALSE}}
+  // Do not import non-trivial classes' initializers.
+  clang_analyzer_eval(extNTS.a == 4); // expected-warning{{TRUE}} expected-warning{{FALSE}}
   clang_analyzer_eval(extHere == 6); // expected-warning{{TRUE}}
   clang_analyzer_eval(A::a == 3); // expected-warning{{TRUE}}
   clang_analyzer_eval(extSC.a == 8); // expected-warning{{TRUE}}
   clang_analyzer_eval(ST::sc.a == 2); // expected-warning{{TRUE}}
   // clang_analyzer_eval(extSCN.scn.a == 9); // TODO
   clang_analyzer_eval(extSubSCN.a == 1); // expected-warning{{TRUE}}
   // clang_analyzer_eval(extSCC.a == 7); // TODO
   clang_analyzer_eval(extU.a == 4); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(TestAnonUnionUSR::Test == 5); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(testImportOfIncompleteDefaultParmDuringImport(9) == 9); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(testImportOfDelegateConstructor(10) == 10); // expected-warning{{TRUE}}
 }

clang/test/Analysis/func-mapping-test.cpp

struct S {
   int a;
 };
 extern S const s = {.a = 2};
 // CHECK-DAG: 4:c:@s
 
 struct SF {
   const int a;
 };
-SF sf = {.a = 2};
+extern const SF sf = {.a = 2};
 // CHECK-DAG: 5:c:@sf
 
 struct SStatic {
   static const int a = 4;
 };
 const int SStatic::a;
 // CHECK-DAG: 14:c:@S@SStatic@a
 
 extern int const arr[5] = { 0, 1 };
 // CHECK-DAG: 6:c:@arr
 
 union U {
   const int a;
   const unsigned int b;
 };
-U u = {.a = 6};
+extern const U u = {.a = 6};
 // CHECK-DAG: 4:c:@u
 
 // No USR can be generated for this.
 // Check for no crash in this case.
 static union {
   float uf;
   const int ui;
 };

clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

void MapExtDefNamesConsumer::handleDecl(const Decl *D) {
   if (!D)
     return;
 
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
     if (FD->isThisDeclarationADefinition())
       if (const Stmt *Body = FD->getBody())
         addIfInMain(FD, Body->getBeginLoc());
   } else if (const auto *VD = dyn_cast<VarDecl>(D)) {
-    if (cross_tu::containsConst(VD, Ctx) && VD->hasInit())
+    if (cross_tu::shouldImport(VD, Ctx) && VD->hasInit())
       if (const Expr *Init = VD->getInit())
         addIfInMain(VD, Init->getBeginLoc());
   }
 
   if (const auto *DC = dyn_cast<DeclContext>(D))
     for (const Decl *D : DC->decls())
       handleDecl(D);
 }