Page MenuHomePhabricator

[ASTImporter] make sure that ACtx::getParents still works
AbandonedPublic

Authored by r.stahl on May 16 2018, 6:09 AM.

Details

Summary

If an AST node is imported after a call to getParents in the ToCtx, it was no longer possible to retrieve its parents since the old results were cached.

Valid types for getParents:

  • Decl, Stmt, NestedNameSpecifier: handled explicitly
  • Type: not supported by ast importer
  • TypeLoc: not imported
  • NestedNameSpecifierLoc: calls import for NNS

Diff Detail

Event Timeline

r.stahl created this revision.May 16 2018, 6:09 AM

Hello Rafael,

The patch is awesome. The only thing I'm uncomfortable with is that we call invalidation on every stmt import - not only during the top-level one. Fixing this requires separating entry point from internal imports and is out of scope of this patch, but adding some FIXMEs is very appreciated.

lib/AST/ASTImporter.cpp
6757

We already do the invalidation in Import(Stmt), so it looks redundant here.

r.stahl added inline comments.May 16 2018, 7:59 AM
lib/AST/ASTImporter.cpp
6757

Yes, sorry this should actually be in Import(Decl) as I wrote in the description!

r.stahl updated this revision to Diff 147268.May 17 2018, 1:52 AM
r.stahl marked 2 inline comments as done.

addressed review comments

martong added inline comments.May 18 2018, 7:47 AM
lib/AST/ASTImporter.cpp
6776

Can an Expr has a parent too? If yes, why not invalidate the parents in Import(Expr*) ?
I am also a bit concerned to call the invalidation during the import of all Stmt (and possible during the import of Exprs too).
Isn't it enough to invalidate only during Import(Decl*)?

r.stahl added inline comments.May 18 2018, 8:04 AM
lib/AST/ASTImporter.cpp
6776

The Import(Expr) function just defers to Import(Stmt) so a call there would be redundant as Alexey commented.

I don't think it would be enough to invalidate in Import(Decl), because Import(Stmt) is part of the public API. I don't know if it makes sense to just import Stmts and if there is any user code doing that, but it is theoretically possible. In that case we should make Import(Stmt) and Import(Expr) private.

I wouldn't be too concerned about too many calls to invalidate, since the function doesn't do anything once the parent info has been released and it will only be rebuilt once someone calls ACtx::getParents - which is not the case while importing.

martong accepted this revision.May 18 2018, 8:07 AM

Ok, thanks for the explanation. Now it looks good to me.

This revision is now accepted and ready to land.May 18 2018, 8:07 AM
a.sidorin accepted this revision.May 18 2018, 8:35 AM

LGTM. Aaron, could you please confirm that AST changes are fine?

include/clang/AST/ASTContext.h
638

I'd prefer to call reset(), but I won't insist on it.

Adding Richard as he has more background in this area. In general, I think it's okay - if we invalidate too often, we do lose some performance, but the correctness should still be there. I'll let Richard provide the final sign-off, however.

@rsmith do you have a chance to take a look or assign someone else?

rsmith added a comment.EditedJun 28 2018, 4:42 PM

This is not specific to the ASTImporter; any change to the AST after a call to getParents could at least potentially have similar problems. Generally, responsibility for dealing with this must lie with the consumer of the parent map, not with the ASTContext, since the ASTContext generally doesn't even know when the AST gets mutated.

I think that the parent map should not be a member of the ASTContext at all. That'd make it much clearer that the responsibility for not reusing it across AST mutations lies with the consumer, not with the mutator of the AST. (Also, having it in ASTContext invites bugs; we do not want the parent map to ever be used by anything in Sema, for instance, largely due to the invalidation problems. In fact, I don't think the clang binary needs the parent map code at all; it really belongs in libTooling.)

@rsmith Yes, this should generally better be handled outside of the ASTContext. That would be out of scope of this patch. Is it fine to proceed with this one for now?

For my usage scenario it actually is convenient this way. In my checker I use getParents, but if I would replace this with some tooling functions I wouldn't know when the maps become invalid since the invalidation happens on a lower level within the analyzer. I'd have to search the parents on every callback invocation which would be a massive performance issue.

Once an external parent utility exists, we would still need some invalidation notification. If anyone uses the analyzer and parent maps he'd want to know when they become invalid for performance reasons. Ideally only when necessary like the invalidateParents calls in this patch.

Hello Richard,

Thank you for clarification. However, I think that moving ParentMap into libTooling is out of scope for this patch. Are you OK if this change will be committed with adding a TODO or FIXME for this move?

r.stahl abandoned this revision.Apr 8 2019, 1:55 AM

ASTContext::getParents should not be used this way. This use-case is solved by function-scoped ParentMaps or AnalysisDeclContext::getParentMap. Discussion: http://lists.llvm.org/pipermail/cfe-dev/2019-April/061944.html