Page MenuHomePhabricator

[ASTImporter] properly import SrcLoc of Attr
ClosedPublic

Authored by r.stahl on Apr 26 2018, 5:14 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

r.stahl created this revision.Apr 26 2018, 5:14 AM

This is unfinished. Posting to make you aware of the issue. There are other occurances of imported attrs without importing the srcloc in:

  • VisitIndirectFieldDecl
  • VisitAttributedStmt

So I was thinking this would suggest to add a public API ASTImporter::Import(Attr *). Does it make sense to add this? Is there anything else I need to do to change public API?

Merging of Attr would also be missing and I wouldn't know how to approach this.

Testing also added soon!

+Alexey because he's the ASTImporter guy.

Hi all. I'm already here, invited by the Herald - just had no time to take a look (will change the script to add me as a reviewer). But thank you anyway! :)

In the initial implementation (https://raw.githubusercontent.com/haoNoQ/clang/summary-ipa-draft/lib/AST/ASTImporter.cpp), we already had this feature, and it was even called similarly - ImportAttributes(). But the centralized import inside Imported (or even `Import()?), as it is done in this patch, looks to be a better option to me.
I'm not sure that we have to "merge" attributes. Moreover, declarations with different attributes can be considered structurally different (possibly, in an another patch). But I can possibly be missing something so feel free to correct me.

Maybe this is a user error of CrossTU, but it seemed to import a FuncDecl with attributes, causing the imported FuncDecl to have all those attributes twice. That's why I thought merging would maybe make sense. However I did not encounter any issue with the duplicate attributes. Only the wrong source locations produced odd crashes.

Thanks for the input, then I will prepare the full patch.

r.stahl updated this revision to Diff 145486.May 7 2018, 9:38 AM
r.stahl edited the summary of this revision. (Show Details)

full patch with test

Hi Rafael! Please find my comments inline.

lib/AST/ASTImporter.cpp
2650 ↗(On Diff #145486)

Could we just remove 'const' qualifier from A to avoid const_cast? (Same below)

6547 ↗(On Diff #145486)

Is it possible to get into a situation where a nullptr attribute is imported?

7211 ↗(On Diff #145486)

As I can see, Import(Attr *) can return nullptr. And I'm not sure that addAttr(nullptr) has well-defined behaviour.

test/Import/attr/Inputs/S.cpp
4 ↗(On Diff #145486)

Could you please clang-format the file?

Sorry, two more nits.

include/clang/AST/ASTImporter.h
137 ↗(On Diff #145486)

nullptr

lib/AST/ASTImporter.cpp
4724 ↗(On Diff #145486)

Could we use ImportArrayChecked instead?

r.stahl updated this revision to Diff 145641.May 8 2018, 12:53 AM
r.stahl marked 6 inline comments as done.
r.stahl added inline comments.
include/clang/AST/ASTImporter.h
137 ↗(On Diff #145486)

I tried to stay consistent with the other descriptions. Will be removed anyway (see below).

lib/AST/ASTImporter.cpp
2650 ↗(On Diff #145486)

I made the interface const instead.

6547 ↗(On Diff #145486)

I looked at the Decl variant and thought it should act similarly, but as far as I can tell, null attributes should not exist.

a.sidorin accepted this revision.May 8 2018, 2:34 AM

Looks good!

lib/AST/ASTImporter.cpp
2650 ↗(On Diff #145486)

Yes, that's even better!

4724 ↗(On Diff #145486)

We can use just ImportArrayChecked(FromAttrs, ToAttrs.begin()). But this is more a hint than a requirement.

This revision is now accepted and ready to land.May 8 2018, 2:34 AM
r.stahl updated this revision to Diff 145660.May 8 2018, 3:33 AM

Didn't see that overload, thanks!

I need someone to commit this for me.

This revision was automatically updated to reflect the committed changes.