This is an archive of the discontinued LLVM Phabricator instance.

[clang][ASTImporter] Add isNewDecl
ClosedPublic

Authored by martong on Apr 13 2022, 9:06 AM.

Details

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

Diff Detail

Event Timeline

martong created this revision.Apr 13 2022, 9:06 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
martong requested review of this revision.Apr 13 2022, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2022, 9:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

steakhal added inline comments.Apr 19 2022, 1:08 AM
clang/include/clang/AST/ASTImporterSharedState.h
68–74

Why does this API accept only non-const pointers?

xazax.hun added inline comments.
clang/include/clang/AST/ASTImporterSharedState.h
42

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?).

80

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
288

Should this be part of Importer.RegisterImportedDecl?

martong marked 4 inline comments as done.May 6 2022, 11:44 AM

@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.)

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
42

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?

68–74

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.

80

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
288

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

xazax.hun accepted this revision.May 6 2022, 11:54 AM

Thanks, this looks good to me!

clang/include/clang/AST/ASTImporterSharedState.h
42

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.

This revision is now accepted and ready to land.May 6 2022, 11:54 AM
martong marked 4 inline comments as done.May 13 2022, 12:24 AM
martong added inline comments.
clang/include/clang/AST/ASTImporterSharedState.h
82

markAsNewDecl sounds better, I'll update before commit.

martong updated this revision to Diff 429212.May 13 2022, 6:01 AM
  • setNewDecl -> markAsNewDecl
This revision was landed with ongoing or failed builds.May 18 2022, 1:37 AM
This revision was automatically updated to reflect the committed changes.