[ASTImporter] properly import SrcLoc of Attr

[r.stahl]

Summary

The ASTImporter was failing to import the SourceLocation field of Attrs. This adds a public API to import Attrs.

[r.stahl]

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!

[NoQ]

+Alexey because he's the ASTImporter guy.

[a.sidorin]

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.

[r.stahl]

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]

full patch with test

[a.sidorin]

Hi Rafael! Please find my comments inline.

lib/AST/ASTImporter.cpp

2650

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

6541

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

7201–7202

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

5

Could you please clang-format the file?

[a.sidorin]

Sorry, two more nits.

include/clang/AST/ASTImporter.h

137

nullptr

lib/AST/ASTImporter.cpp

4723–4724

Could we use ImportArrayChecked instead?

[r.stahl]

include/clang/AST/ASTImporter.h

137

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

lib/AST/ASTImporter.cpp

2650

I made the interface const instead.

6541

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]

Looks good!

lib/AST/ASTImporter.cpp

2650

Yes, that's even better!

4723–4724

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

[r.stahl]

Didn't see that overload, thanks!

I need someone to commit this for me.

include/clang/AST/ASTImporter.h

 class Decl;
 class DeclContext;
 class Expr;
 class FileManager;
 class NamedDecl;
 class Stmt;
 class TagDecl;
 class TypeSourceInfo;
+class Attr;
 
   /// \brief Imports selected nodes from one AST context into another context,
   /// merging AST nodes where appropriate.
   class ASTImporter {
   public:
     using NonEquivalentDeclSet = llvm::DenseSet<std::pair<Decl *, Decl *>>;
     using ImportedCXXBaseSpecifierMap =
         llvm::DenseMap<const CXXBaseSpecifier *, CXXBaseSpecifier *>;
public:
 
     /// \brief Import the given type source information from the
     /// "from" context into the "to" context.
     ///
     /// \returns the equivalent type source information in the "to"
     /// context, or NULL if an error occurred.
     TypeSourceInfo *Import(TypeSourceInfo *FromTSI);
 
+    /// \brief Import the given attribute from the "from" context into the
+    /// "to" context.
+    ///
+    /// \returns the equivalent attribute in the "to" context.
+    Attr *Import(const Attr *FromAttr);
+
     /// \brief Import the given declaration from the "from" context into the 
     /// "to" context.
     ///
     /// \returns the equivalent declaration in the "to" context, or a NULL type 
     /// if an error occurred.
     Decl *Import(Decl *FromD);
 
     /// \brief Return the copy of the given declaration in the "to" context if

lib/AST/ASTImporter.cpp

if (!D)
       return nullptr;
     NamedChain[i++] = cast<NamedDecl>(D);
   }
 
   IndirectFieldDecl *ToIndirectField = IndirectFieldDecl::Create(
       Importer.getToContext(), DC, Loc, Name.getAsIdentifierInfo(), T,
       {NamedChain, D->getChainingSize()});
 
-  for (const auto *Attr : D->attrs())
-    ToIndirectField->addAttr(Attr->clone(Importer.getToContext()));
+  for (const auto *A : D->attrs())
+    ToIndirectField->addAttr(Importer.Import(A));
 
   ToIndirectField->setAccess(D->getAccess());
   ToIndirectField->setLexicalDeclContext(LexicalDC);
   Importer.Imported(D, ToIndirectField);
   LexicalDC->addDeclInternal(ToIndirectField);
   return ToIndirectField;
 }
 
if (!ToSubStmt && S->getSubStmt())
     return nullptr;
   return new (Importer.getToContext()) LabelStmt(ToIdentLoc, ToLabelDecl,
                                                  ToSubStmt);
 }
 
 Stmt *ASTNodeImporter::VisitAttributedStmt(AttributedStmt *S) {
   SourceLocation ToAttrLoc = Importer.Import(S->getAttrLoc());
   ArrayRef<const Attr*> FromAttrs(S->getAttrs());
   SmallVector<const Attr *, 1> ToAttrs(FromAttrs.size());
-  ASTContext &_ToContext = Importer.getToContext();
-  std::transform(FromAttrs.begin(), FromAttrs.end(), ToAttrs.begin(),
-    [&_ToContext](const Attr *A) -> const Attr * {
-      return A->clone(_ToContext);
-    });
-  for (const auto *ToA : ToAttrs) {
-    if (!ToA)
-      return nullptr;
-  }
+  if (ImportArrayChecked(FromAttrs.begin(), FromAttrs.end(), ToAttrs.begin()))
   Stmt *ToSubStmt = Importer.Import(S->getSubStmt());
   if (!ToSubStmt && S->getSubStmt())
     return nullptr;
   return AttributedStmt::Create(Importer.getToContext(), ToAttrLoc,
                                 ToAttrs, ToSubStmt);
 }
 
 Stmt *ASTNodeImporter::VisitIfStmt(IfStmt *S) {
TypeSourceInfo *ASTImporter::Import(TypeSourceInfo *FromTSI) {
   QualType T = Import(FromTSI->getType());
   if (T.isNull())
     return nullptr;
 
   return ToContext.getTrivialTypeSourceInfo(T, 
            Import(FromTSI->getTypeLoc().getLocStart()));
 }
 
+Attr *ASTImporter::Import(const Attr *FromAttr) {
+  Attr *ToAttr = FromAttr->clone(ToContext);
+  ToAttr->setRange(Import(FromAttr->getRange()));
+  return ToAttr;
+}
+
 Decl *ASTImporter::GetAlreadyImportedOrNull(Decl *FromD) {
   llvm::DenseMap<Decl *, Decl *>::iterator Pos = ImportedDecls.find(FromD);
   if (Pos != ImportedDecls.end()) {
     Decl *ToD = Pos->second;
     ASTNodeImporter(*this).ImportDefinitionIfNeeded(FromD, ToD);
     return ToD;
   } else {
     return nullptr;
void ASTImporter::CompleteDecl (Decl *D) {
   }
   else {
     assert(0 && "CompleteDecl called on a Decl that can't be completed");
   }
 }
 
 Decl *ASTImporter::Imported(Decl *From, Decl *To) {
   if (From->hasAttrs()) {
-    for (auto *FromAttr : From->getAttrs())
-      To->addAttr(FromAttr->clone(To->getASTContext()));
+    for (const auto *FromAttr : From->getAttrs())
+      To->addAttr(Import(FromAttr));
   }
   if (From->isUsed()) {
     To->setIsUsed();
   }
   if (From->isImplicit()) {
     To->setImplicit();
   }
   ImportedDecls[From] = To;

test/Import/attr/Inputs/S.cpp

+extern void f() __attribute__((const));
+
+struct S {
+  struct {
+    int a __attribute__((packed));
+  };
+};
+
+void stmt() {
+#pragma unroll
+  for (;;)
+    ;
+}

test/Import/attr/test.cpp

+// RUN: clang-import-test -dump-ast -import %S/Inputs/S.cpp -expression %s | FileCheck %s
+// CHECK: FunctionDecl
+// CHECK-SAME: S.cpp:1:1, col:13
+// CHECK-NEXT: ConstAttr
+// CHECK-SAME: col:32
+
+// CHECK: IndirectFieldDecl
+// CHECK-NEXT: Field
+// CHECK-NEXT: Field
+// CHECK-NEXT: PackedAttr
+// CHECK-SAME: col:26
+
+// CHECK: AttributedStmt
+// CHECK-NEXT: LoopHintAttr
+// CHECK-SAME: line:10:9
+
+extern void f() __attribute__((const));
+
+struct S;
+
+void stmt();
+
+void expr() {
+  f();
+  struct S s;
+}