[clang][ASTImporter] Add isNewDecl

[martong]

Summary

Add a new function with which we can query if a Decl had been newly
created during the import process. This feature is a must if we want to
have a different static analysis strategy for such newly created
declarations.

This is a dependent patch that is needed for the new CTU implementation
discribed at
https://discourse.llvm.org/t/rfc-much-faster-cross-translation-unit-ctu-analysis-implementation/61728

[steakhal]

What happens if the import fails? Or alternatively the InitializeImportedDecl(FromD, ToD) fails?

[steakhal]

clang/include/clang/AST/ASTImporterSharedState.h

69–75

Why does this API accept only non-const pointers?

[xazax.hun]

clang/include/clang/AST/ASTImporterSharedState.h

43

ASTImporter already has something like ImportedFromDecls. Is that not sufficient to check if a declaration is new?

Is it possible that we may want the "degree" of the imported definition? I.e., how many hops did we do to import it (is it imported as a result of evaluating an imported call?).

81

I assume this would only be applicable for definitions, so I wonder whether IsNewDefinition() would be more descriptive. Or maybe IsImportedDefinition?

clang/lib/AST/ASTImporter.cpp

248

Should this be part of Importer.RegisterImportedDecl?

[martong]

@xazax.hun Hey Gabor, thanks a lot for your time and effort for reviewing this patch set!
(And of course thank you for you too @steakhal, but you've already seen most of it.)

In D123685#3458329, @steakhal wrote:

What happens if the import fails?

Then Import returns with an Error object.

Or alternatively the InitializeImportedDecl(FromD, ToD) fails?

That function can never fail, that is just setting a few bits:

void InitializeImportedDecl(Decl *FromD, Decl *ToD) {
  ToD->IdentifierNamespace = FromD->IdentifierNamespace;
  if (FromD->isUsed())
    ToD->setIsUsed();
  if (FromD->isImplicit())
    ToD->setImplicit();
}

clang/include/clang/AST/ASTImporterSharedState.h

43

What we need here is a list of those declarations that have been created and linked into the destination TU by the ASTImporter.

ImportedFromDecls is a mapping of declarations from the "source" context to the "destination" context. It might happen that we do not create a new declaration during the import, however, the mapping is still updated, so next time we can just simply get that from there (it's a cache).

E.g. during the import of foo from b.cpp to a.cpp we are going to import X as well. But during the import of X we find the existing definition and then we just simply update the mapping to that. This happens all the time when we include the same header to two different translation units.

// a.cpp
struct X {};
// b.cpp
struct X {};
void foo(X);

Is it possible that we may want the "degree" of the imported definition? I.e., how many hops did we do to import it

I am not sure how that would be useful, I mean how and for what could we use that information?

69–75

This is legacy from the old times, internal AST functions all take non-const pointers.
However, I agree, we could extend the API with overloads that take a const, but that should be orthogonal to this change.

81

In the context of CTU, you are right, we are interested in definitions only. However, it would be over-complication to do that distinction at the ASTImporter level, I think.

clang/lib/AST/ASTImporter.cpp

248

No. RegisterImportedDecl is being overwritten in some of the descendant lldb classes. We want setNewDecl called no matter what.

[xazax.hun]

Thanks, this looks good to me!

clang/include/clang/AST/ASTImporterSharedState.h

43

I am not sure how that would be useful, I mean how and for what could we use that information?

It could be part of the heuristics whether we want something inlined during the analysis, but we might not need this information at all.

[martong]

clang/include/clang/AST/ASTImporterSharedState.h

83

markAsNewDecl sounds better, I'll update before commit.

[martong]

• setNewDecl -> markAsNewDecl

clang/include/clang/AST/ASTImporterSharedState.h

class ASTImporterSharedState {
   /// Mapping from the already-imported declarations in the "to"
   /// context to the error status of the import of that declaration.
   /// This map contains only the declarations that were not correctly
   /// imported. The same declaration may or may not be included in
   /// ImportedFromDecls. This map is updated continuously during imports and
   /// never cleared (like ImportedFromDecls).
   llvm::DenseMap<Decl *, ImportError> ImportErrors;
 
+  /// Set of the newly created declarations.
+  llvm::DenseSet<Decl *> NewDecls;
+
   // FIXME put ImportedFromDecls here!
   // And from that point we can better encapsulate the lookup table.
 
 public:
   ASTImporterSharedState() = default;
 
   ASTImporterSharedState(TranslationUnitDecl &ToTU) {
     LookupTable = std::make_unique<ASTImporterLookupTable>(ToTU);
   }
 
   ASTImporterLookupTable *getLookupTable() { return LookupTable.get(); }
 
   void addDeclToLookup(Decl *D) {
     if (LookupTable)
       if (auto *ND = dyn_cast<NamedDecl>(D))
         LookupTable->add(ND);
   }
 
   void removeDeclFromLookup(Decl *D) {
     if (LookupTable)
       if (auto *ND = dyn_cast<NamedDecl>(D))
         LookupTable->remove(ND);
   }
 
   llvm::Optional<ImportError> getImportDeclErrorIfAny(Decl *ToD) const {
     auto Pos = ImportErrors.find(ToD);
     if (Pos != ImportErrors.end())
       return Pos->second;
     else
       return Optional<ImportError>();
   }
 
   void setImportDeclError(Decl *To, ImportError Error) {
     ImportErrors[To] = Error;
   }
+
+  bool isNewDecl(const Decl *ToD) const { return NewDecls.count(ToD); }
+
+  void setNewDecl(Decl *ToD) { NewDecls.insert(ToD); }
 };
 
 } // namespace clang
 #endif // LLVM_CLANG_AST_ASTIMPORTERSHAREDSTATE_H

clang/lib/AST/ASTImporter.cpp

GetImportedOrCreateSpecialDecl(ToDeclT *&ToD, CreateFunT CreateFun,
         return true; // Already imported but with error.
       }
       ToD = cast_or_null<ToDeclT>(Importer.GetAlreadyImportedOrNull(FromD));
       if (ToD)
         return true; // Already imported.
       ToD = CreateFun(std::forward<Args>(args)...);
       // Keep track of imported Decls.
       Importer.RegisterImportedDecl(FromD, ToD);
+      Importer.SharedState->setNewDecl(ToD);
       InitializeImportedDecl(FromD, ToD);
       return false; // A new Decl is created.
     }
 
     void InitializeImportedDecl(Decl *FromD, Decl *ToD) {
       ToD->IdentifierNamespace = FromD->IdentifierNamespace;
       if (FromD->isUsed())
         ToD->setIsUsed();

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase,
   // Test that during import of 'foo' the record layout (of 'A') can be obtained
   // without crash. It is not possible to have all of the fields of 'A' imported
   // at that time (without big code changes).
   auto *ToA = Import(FromA, Lang_CXX11);
   EXPECT_TRUE(ToA);
   EXPECT_TRUE(ToA->isCompleteDefinition());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isNewDecl) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      int bar() {
+        return 0;
+      }
+      void other() {
+        bar();
+      }
+      )",
+      Lang_CXX11);
+  Decl *ToTU = getToTuDecl(
+      R"(
+      int bar() {
+        return 0;
+      }
+      )",
+      Lang_CXX11);
+  auto *FromOther = FirstDeclMatcher<FunctionDecl>().match(
+      FromTU, functionDecl(hasName("other")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_CXX11);
+  ASSERT_TRUE(ToOther);
+
+  auto *ToBar = FirstDeclMatcher<FunctionDecl>().match(
+      ToTU, functionDecl(hasName("bar")));
+
+  EXPECT_TRUE(SharedStatePtr->isNewDecl(ToOther));
+  EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportPath,
                          ::testing::Values(std::vector<std::string>()));
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportExpr,
                          DefaultTestValuesForRunOptions);