[CrossTU] Handle case when no USR could be generated during Decl search.

[balazske]

Summary

When searching for a declaration to be loaded the "lookup name" for every
other Decl is computed. If the USR can not be determined here should be
not an assert, instead skip this Decl.

[martong]

Thank you for this patch Balazs! Could you please elaborate in which cases we cannot generate the USR? Does it happen only if we have an anonymous union inside a function? Are there no other cases?

[balazske]

It looks like that the problem can happen when the anonymous union is in any DeclContext and for CTU import the import of a variable is requested and that variable is in a related DeclContext (it can be at upper or lower level). (See code of findDefInDeclContext: If used for variables, it goes through every DeclContext and computes USR for specific VarDecls until the one is found. If we have luck it may be found before the union. An anonymous union can not be imported because we get no USR for it so it is OK to ignore these.) So the test can be written in many other forms but this case (the code of f) was encountered in protobuf code.

[martong]

In D65445#1607803, @balazske wrote:

It looks like that the problem can happen when the anonymous union is in any DeclContext and for CTU import the import of a variable is requested and that variable is in a related DeclContext (it can be at upper or lower level). (See code of findDefInDeclContext: If used for variables, it goes through every DeclContext and computes USR for specific VarDecls until the one is found. If we have luck it may be found before the union. An anonymous union can not be imported because we get no USR for it so it is OK to ignore these.) So the test can be written in many other forms but this case (the code of f) was encountered in protobuf code.

Thanks for the explanation.

So, IIUC we cannot generate the USR for the member of the anonymous union, right? f or i below does not have a USR.

class TestAnonUnionUSR {
public:
  inline float f(int value) {
    union {
      float f;
      int i;
    };
    i = value;
    return f;
  }
  static const int Test;
};

And findDefInDeclContext goes through all children DC. So if the DC is TestAnonUnionUSR then we may try to get the USR for f and i. Please confirm if I understand it correctly or not.

[balazske]

Probably this is a problem case too but only if the f or i has an initializer expression and really no USR is generated for f or i. But what I have found is that there is a VarDecl for a "invisible" variable whose type is the anonymous union and it has a (default) initializer too. If the search reaches DC of the f it will try to get the USR for this variable.

[martong]

In D65445#1608277, @balazske wrote:

Probably this is a problem case too but only if the f or i has an initializer expression and really no USR is generated for f or i. But what I have found is that there is a VarDecl for a "invisible" variable whose type is the anonymous union and it has a (default) initializer too. If the search reaches DC of the f it will try to get the USR for this variable.

OK. Just for the record and for later reference, I copy the test code and the AST here.

class TestAnonUnionUSR {
public:
  inline float f(int value) {
    union {
      float f;
      int i;
    };
    i = value;
    return f;
  }
  static const int Test;
};

`-CXXRecordDecl 0xdb9fe0 </tmp/usr.cpp:1:1, line:12:1> line:1:7 class TestAnonUnionUSR definition
  |-DefinitionData pass_in_registers empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
  | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
  | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveConstructor exists simple trivial needs_implicit
  | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveAssignment exists simple trivial needs_implicit
  | `-Destructor simple irrelevant trivial needs_implicit
  |-CXXRecordDecl 0xdba0f8 <col:1, col:7> col:7 implicit class TestAnonUnionUSR
  |-AccessSpecDecl 0xdba188 <line:2:1, col:7> col:1 public
  |-CXXMethodDecl 0xdba2c0 <line:3:3, line:10:3> line:3:16 f 'float (int)' inline
  | |-ParmVarDecl 0xdba1c8 <col:18, col:22> col:22 used value 'int'
  | `-CompoundStmt 0xdbae70 <col:29, line:10:3>
  |   |-DeclStmt 0xdbac68 <line:4:5, line:7:6>
  |   | |-CXXRecordDecl 0xdba400 <line:4:5, line:7:5> line:4:5 union definition
  |   | | |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal has_variant_members
  |   | | | |-DefaultConstructor exists trivial
  |   | | | |-CopyConstructor simple trivial has_const_param implicit_has_const_param
  |   | | | |-MoveConstructor exists simple trivial
  |   | | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
  |   | | | |-MoveAssignment exists simple trivial needs_implicit
  |   | | | `-Destructor simple irrelevant trivial needs_implicit
  |   | | |-FieldDecl 0xdba530 <line:5:7, col:13> col:13 referenced f 'float'
  |   | | |-FieldDecl 0xdba598 <line:6:7, col:11> col:11 referenced i 'int'
  |   | | |-CXXConstructorDecl 0xdba6d8 <line:4:5> col:5 implicit used  'void () noexcept' inline default trivial
  |   | | | `-CompoundStmt 0xdbab68 <col:5>
  |   | | |-CXXConstructorDecl 0xdba808 <col:5> col:5 implicit constexpr  'void (const (anonymous union at /tmp/usr.cpp:4:5) &)' inline default trivial noexcept-unevaluated 0xdba808
  |   | | | `-ParmVarDecl 0xdba918 <col:5> col:5 'const (anonymous union at /tmp/usr.cpp:4:5) &'
  |   | | `-CXXConstructorDecl 0xdba9b8 <col:5> col:5 implicit constexpr  'void ((anonymous union at /tmp/usr.cpp:4:5) &&)' inline default trivial noexcept-unevaluated 0xdba9b8
  |   | |   `-ParmVarDecl 0xdbaac8 <col:5> col:5 '(anonymous union at /tmp/usr.cpp:4:5) &&'
  |   | `-VarDecl 0xdba658 <col:5> col:5 implicit used '(anonymous union at /tmp/usr.cpp:4:5)' callinit
  |   |   `-CXXConstructExpr 0xdbab78 <col:5> '(anonymous union at /tmp/usr.cpp:4:5)' 'void () noexcept'
  |   |-BinaryOperator 0xdbad08 <line:8:5, col:9> 'int' lvalue '='
  |   | |-MemberExpr 0xdbaca0 <col:5> 'int' lvalue .i 0xdba598
  |   | | `-DeclRefExpr 0xdbac80 <col:5> '(anonymous union at /tmp/usr.cpp:4:5)' lvalue Var 0xdba658 '' '(anonymous union at /tmp/usr.cpp:4:5)'
  |   | `-ImplicitCastExpr 0xdbacf0 <col:9> 'int' <LValueToRValue>
  |   |   `-DeclRefExpr 0xdbacd0 <col:9> 'int' lvalue ParmVar 0xdba1c8 'value' 'int'
  |   `-ReturnStmt 0xdbae60 <line:9:5, col:12>
  |     `-ImplicitCastExpr 0xdbae48 <col:12> 'float' <LValueToRValue>
  |       `-MemberExpr 0xdbae18 <col:12> 'float' lvalue .f 0xdba530
  |         `-DeclRefExpr 0xdbadf8 <col:12> '(anonymous union at /tmp/usr.cpp:4:5)' lvalue Var 0xdba658 '' '(anonymous union at /tmp/usr.cpp:4:5)'
  `-VarDecl 0xdba380 <line:11:3, col:20> col:20 Test 'const int' static

[martong]

LGTM! Please proceed and commit! :)

[balazske]

Rebase and add a new testing case.

[balazske]

Rebase.

clang/include/clang/CrossTU/CrossTranslationUnit.h

 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_CROSSTU_CROSSTRANSLATIONUNIT_H
 #define LLVM_CLANG_CROSSTU_CROSSTRANSLATIONUNIT_H
 
 #include "clang/AST/ASTImporterSharedState.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Error.h"
 
 namespace clang {
 class CompilerInstance;
 class ASTContext;
 class ASTImporter;
public:
   ///
   /// \return Returns the resulting definition or an error.
   llvm::Expected<const FunctionDecl *> importDefinition(const FunctionDecl *FD,
                                                         ASTUnit *Unit);
   llvm::Expected<const VarDecl *> importDefinition(const VarDecl *VD,
                                                    ASTUnit *Unit);
 
   /// Get a name to identify a named decl.
-  static std::string getLookupName(const NamedDecl *ND);
+  static llvm::Optional<std::string> getLookupName(const NamedDecl *ND);
 
   /// Emit diagnostics for the user for potential configuration errors.
   void emitCrossTUDiagnostics(const IndexError &IE);
 
   /// Determine the original source location in the original TU for an
   /// imported source location.
   /// \p ToLoc Source location in the imported-to AST.
   /// \return Source location in the imported-from AST and the corresponding

clang/lib/CrossTU/CrossTranslationUnit.cpp

 }
 
 CrossTranslationUnitContext::CrossTranslationUnitContext(CompilerInstance &CI)
     : Context(CI.getASTContext()), ASTStorage(CI),
       CTULoadThreshold(CI.getAnalyzerOpts()->CTUImportThreshold) {}
 
 CrossTranslationUnitContext::~CrossTranslationUnitContext() {}
 
-std::string CrossTranslationUnitContext::getLookupName(const NamedDecl *ND) {
+llvm::Optional<std::string>
+CrossTranslationUnitContext::getLookupName(const NamedDecl *ND) {
   SmallString<128> DeclUSR;
   bool Ret = index::generateUSRForDecl(ND, DeclUSR);
-  (void)Ret;
-  assert(!Ret && "Unable to generate USR");
-  return DeclUSR.str();
+  if (Ret)
+    return {};
+  return std::string(DeclUSR.str());
 }
 
 /// Recursively visits the decls of a DeclContext, and returns one with the
 /// given USR.
 template <typename T>
 const T *
 CrossTranslationUnitContext::findDefInDeclContext(const DeclContext *DC,
                                                   StringRef LookupName) {
   assert(DC && "Declaration Context must not be null");
   for (const Decl *D : DC->decls()) {
     const auto *SubDC = dyn_cast<DeclContext>(D);
     if (SubDC)
       if (const auto *ND = findDefInDeclContext<T>(SubDC, LookupName))
         return ND;
 
     const auto *ND = dyn_cast<T>(D);
     const T *ResultDecl;
     if (!ND || !hasBodyOrInit(ND, ResultDecl))
       continue;
-    if (getLookupName(ResultDecl) != LookupName)
+    llvm::Optional<std::string> ResultLookupName = getLookupName(ResultDecl);
+    if (!ResultLookupName || *ResultLookupName != LookupName)
       continue;
     return ResultDecl;
   }
   return nullptr;
 }
 
 template <typename T>
 llvm::Expected<const T *> CrossTranslationUnitContext::getCrossTUDefinitionImpl(
     const T *D, StringRef CrossTUDir, StringRef IndexName,
     bool DisplayCTUProgress) {
   assert(D && "D is missing, bad call to this function!");
   assert(!hasBodyOrInit(D) &&
          "D has a body or init in current translation unit!");
   ++NumGetCTUCalled;
-  const std::string LookupName = getLookupName(D);
-  if (LookupName.empty())
+  const llvm::Optional<std::string> LookupName = getLookupName(D);
+  if (!LookupName)
     return llvm::make_error<IndexError>(
         index_error_code::failed_to_generate_usr);
   llvm::Expected<ASTUnit *> ASTUnitOrError =
-      loadExternalAST(LookupName, CrossTUDir, IndexName, DisplayCTUProgress);
+      loadExternalAST(*LookupName, CrossTUDir, IndexName, DisplayCTUProgress);
   if (!ASTUnitOrError)
     return ASTUnitOrError.takeError();
   ASTUnit *Unit = *ASTUnitOrError;
   assert(&Unit->getFileManager() ==
          &Unit->getASTContext().getSourceManager().getFileManager());
 
   const llvm::Triple &TripleTo = Context.getTargetInfo().getTriple();
   const llvm::Triple &TripleFrom =
if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
       LangTo.CPlusPlus17 != LangFrom.CPlusPlus17 ||
       LangTo.CPlusPlus2a != LangFrom.CPlusPlus2a) {
     ++NumLangDialectMismatch;
     return llvm::make_error<IndexError>(
         index_error_code::lang_dialect_mismatch);
   }
 
   TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
-  if (const T *ResultDecl = findDefInDeclContext<T>(TU, LookupName))
+  if (const T *ResultDecl = findDefInDeclContext<T>(TU, *LookupName))
     return importDefinition(ResultDecl, Unit);
   return llvm::make_error<IndexError>(index_error_code::failed_import);
 }
 
 llvm::Expected<const FunctionDecl *>
 CrossTranslationUnitContext::getCrossTUDefinition(const FunctionDecl *FD,
                                                   StringRef CrossTUDir,
                                                   StringRef IndexName,

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

struct SCC {
   const int a;
 };
 SCC extSCC{7};
 union U {
   const int a;
   const unsigned int b;
 };
 U extU = {.a = 4};
+
+class TestAnonUnionUSR {
+public:
+  inline float f(int value) {
+    union {
+      float f;
+      int i;
+    };
+    i = value;
+    return f;
+  }
+  static const int Test;
+};
+const int TestAnonUnionUSR::Test = 5;

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

 c:@extS ctu-other.cpp.ast
 c:@S@A@a ctu-other.cpp.ast
 c:@extSC ctu-other.cpp.ast
 c:@S@ST@sc ctu-other.cpp.ast
 c:@extSCN ctu-other.cpp.ast
 c:@extSubSCN ctu-other.cpp.ast
 c:@extSCC ctu-other.cpp.ast
 c:@extU ctu-other.cpp.ast
+c:@S@TestAnonUnionUSR@Test ctu-other.cpp.ast

clang/test/Analysis/ctu-main.cpp

 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}}
 }
 
+class TestAnonUnionUSR {
+public:
+  inline float f(int value) {
+    union {
+      float f;
+      int i;
+    };
+    i = value;
+    return f;
+  }
+  static const int Test;
+};
+
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(4) == 3); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(5) == 3); // expected-warning{{FALSE}}
   clang_analyzer_eval(g(4) == 6); // expected-warning{{TRUE}}
   clang_analyzer_eval(h(2) == 8); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(myns::fns(2) == 9);                   // expected-warning{{TRUE}}
int main() {
   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/test/Analysis/func-mapping-test.cpp

 // CHECK-DAG: c:@arr
 
 union U {
   const int a;
   const unsigned int b;
 };
 U u = {.a = 6};
 // CHECK-DAG: 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 (const auto *DC = dyn_cast<DeclContext>(D))
     for (const Decl *D : DC->decls())
       handleDecl(D);
 }
 
 void MapExtDefNamesConsumer::addIfInMain(const DeclaratorDecl *DD,
                                          SourceLocation defStart) {
-  std::string LookupName = CrossTranslationUnitContext::getLookupName(DD);
+  llvm::Optional<std::string> LookupName =
+      CrossTranslationUnitContext::getLookupName(DD);
+  if (!LookupName)
+    return;
+  assert(!LookupName->empty() && "Lookup name should be non-empty.");
+
   if (CurrentFileName.empty()) {
     CurrentFileName =
         SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
     if (CurrentFileName.empty())
       CurrentFileName = "invalid_file";
   }
 
   switch (DD->getLinkageInternal()) {
   case ExternalLinkage:
   case VisibleNoLinkage:
   case UniqueExternalLinkage:
     if (SM.isInMainFile(defStart))
-      Index[LookupName] = CurrentFileName;
+      Index[*LookupName] = CurrentFileName;
     break;
   default:
     break;
   }
 }
 
 class MapExtDefNamesAction : public ASTFrontendAction {
 protected: