[clang][ASTImporter] Generic attribute import handling (first step).

[balazske]

Summary

Import of Attr objects was incomplete in ASTImporter.
This change introduces support for a generic way of importing an attribute.
For an usage example import of the attribute AssertCapability is
added to ASTImporter.
Updating the old attribute import code and adding new attributes or extending
the generic functions (if needed) is future work.

[martong]

Thanks, nice work! I like the direction, however, I'd like to ask comments from @teemperor .

clang/lib/AST/ASTImporter.cpp

656–671

What's the reason of moving this hunk?

8468–8469

Why can't we use importChecked here?

8480–8481

Could you please remove this comment?

8569–8570

Could we hide these arguments?
I mean we probably need a higher abstraction, something like

Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(From);

should be sufficient, isn't it. We do want to import all arguments of all kind of attributes, don't we?

[shafik]

clang/lib/AST/ASTImporter.cpp

711

Looks like left over unused code?

8443

@martong the ASTImporter file is quite large as it is and I think it makes sense to have AttrImporter but perhaps it would help long-term maintainability to split it out into its own source file? What do you think?

[martong]

clang/lib/AST/ASTImporter.cpp

8443

Yes, good idea, I agree. Although, I wouldn't insist to do that in this patch, a following patch is okay for me.

After all, it would make sense to split ASTNodeImporter into it's own implementation file. Or, even better, we could put Visit*Type, Visit*Decl and Visit*Stmt into separate files. Other than that, the ASTImporterTest.cpp test file compiles painfully slow, so that is among my TODOs for a while to split that up as well.

[balazske]

clang/lib/AST/ASTImporter.cpp

656–671

This is moved into the public section. Other such functions are public already.

8468–8469

For Identifier* no error is possible and importChecked does not work (could be added to have uniform code).

8569–8570

It should be possible to pass every kind of needed arguments (after it is imported) to the constructor of the newly created Attr object. The problem is solved here by the AttrArgImporter that imports the object and can store it in a temporary value until it is passed to the constructor. The temporary value is important if an array is imported. To import the array the size must be passed to the array importer before, and the size must be passed to the constructor of the Attr too, therefore it exists 2 times in the code. An AttrArgImporter can provide only one value to the constructor of the new Attr object. (Probably other solution is possible with std::tuple and std::apply but not sure if it is better.)

[martong]

@steakhal, could you please take a look?

[steakhal]

I like the adapter layer idea. However, I agree with @martong that it should be more 'abstract' ~ terse.
It's remarkable how compact the test is. Good job.

clang/lib/AST/ASTImporter.cpp

8412–8413

What about the rest of the special member functions like assignment operators?

All the rest of the code mentions this template parameter using the AT name. Could you please consolidate this? It would make it more readable.

8420

So, the value() sometimes returns const ref, and other times it returns a mutable raw pointer...

I suspect, attribute constructors expect simple reference arguments and pointers for lists. And this is why it behaves like this. Am I right about this?

8434

Please fix this.

8455–8457

The name AT suggests to me that you expect the template type parameter to be a subtype of class Attr. However, I suspect it's not always the case.
For example in case of the OwnershipAttr the args() returns a sequence of ParamIdx* objects. So, in that sense, the AT name is not properly justified.

Restricting template parameter types makes the code cleaner, so I would suggest introducing a metafunction, that you could use in a static_assert that you could use to check this requirement as the first instruction in the function.

template <typename T>
constexpr static bool AttrOrParamIdx = std::is_base_of<Attr, T>::value || std::is_same_v<T, ParamIdx>;

static_assert(AttrOrParamIdx<T>);

8461

I think you should be consistently using typename or class. I'm generally in favor of using typename though.

8462

So, this accepts universal references, shouldn't we std::forward when we consume them?
If not, why do you use && ?

8468–8469

Why don't you initialize the variables directly? Same for the other variables. This way they could be const as well.

8569–8570

What about having two implementations. One for any T that has args() and one for anything else and using SFINAE choosing the right one.
In this context, we should have the appropriate dynamic type, so by using template type deduction on From could achieve this dispatch.

Internally it could do the AI.importArrayArg(From->args(), From->args_size()).value() for the args() case.
WDYT?

clang/unittests/AST/ASTImporterTest.cpp

6414

Should we dump to the stderr?

6442–6443

<3 More!

6467–6468

An iterator range is just a pair of iterators, am I right?
We could still pass them by value.

6551–6553

You could probably use the regular "void test(int A1, int A2) __attribute__((assert_capability(A1, A2)));" string literal. It would still fit in the line and would consume only a single line.

[balazske]

Fixed review issues.
Added code comments.

clang/lib/AST/ASTImporter.cpp

8420

The data returned by value is the one that is passed to the attribute constructor. It is in exact the same form, and for arrays this is a simple pointer to the array data. The array in the attribute has a size parameter too, this can be passed separately from this object.

One such "importer" is created for every attribute argument even if it is of an array type. For the array size no such object has to be created, only for the array data.

8455–8457

In this case AT is the element type of the array attribute argument, mostly Expr * but can be any other. The AT should mean "Argument Type". Now is is renamed to T because it is the only parameter, and every other place T is used too. I think it is not needed to make a static assert for every possible attribute argument type. The class can be still used incorrectly and theoretically attribute argument can be of any type, so the assert adds not much value.

8461

Here the first parameter is really a class, the second not.

8569–8570

Probably we can have a specialization for exactly the case when the attribute has one parameter called "Args" that is of an array type. We need to check at how many attributes this is the case, probably it is only used at the thread safety part (relatively) many times.

clang/unittests/AST/ASTImporterTest.cpp

6467–6468

Probably enough to pass by value but this should be not worse than that.

[balazske]

clang/unittests/AST/ASTImporterTest.cpp

6414

It would be good to have a "sink stream" that just discards the input. In this case it is not needed to allocate memory buffer, the text output of the dump is not used. ASTImporterTest has already many unneeded output.

[steakhal]

I think I'm convinced.
Thanks.

clang/unittests/AST/ASTImporterTest.cpp

6414

llvm::nulls() returns such a stream.

[martong]

Look good! Thanks!

[balazske]

Dump to 'llvm::nulls()'.

clang/lib/AST/ASTImporter.cpp

class ASTNodeImporter : public TypeVisitor<ASTNodeImporter, ExpectedType>,
     // Import an Optional<T> by importing the contained T, if any.
     template<typename T>
     Expected<Optional<T>> import(Optional<T> From) {
       if (!From)
         return Optional<T>();
       return import(*From);
     }
 
-    // Helper for chaining together multiple imports. If an error is detected,
-    // subsequent imports will return default constructed nodes, so that failure
-    // can be detected with a single conditional branch after a sequence of
-    // imports.
-    template <typename T> T importChecked(Error &Err, const T &From) {
-      // Don't attempt to import nodes if we hit an error earlier.
-      if (Err)
-        return T{};
-      Expected<T> MaybeVal = import(From);
-      if (!MaybeVal) {
-        Err = MaybeVal.takeError();
-        return T{};
-      }
-      return *MaybeVal;
-    }
-
     ExplicitSpecifier importExplicitSpecifier(Error &Err,
                                               ExplicitSpecifier ESpec);
 
     // Wrapper for an overload set.
     template <typename ToDeclT> struct CallOverloadedCreateFun {
       template <typename... Args> decltype(auto) operator()(Args &&... args) {
         return ToDeclT::Create(std::forward<Args>(args)...);
       }
public:
     ExpectedStmt VisitArrayInitIndexExpr(ArrayInitIndexExpr *E);
     ExpectedStmt VisitCXXDefaultInitExpr(CXXDefaultInitExpr *E);
     ExpectedStmt VisitCXXNamedCastExpr(CXXNamedCastExpr *E);
     ExpectedStmt VisitSubstNonTypeTemplateParmExpr(SubstNonTypeTemplateParmExpr *E);
     ExpectedStmt VisitTypeTraitExpr(TypeTraitExpr *E);
     ExpectedStmt VisitCXXTypeidExpr(CXXTypeidExpr *E);
     ExpectedStmt VisitCXXFoldExpr(CXXFoldExpr *E);
 
+    // Helper for chaining together multiple imports. If an error is detected,
+    // subsequent imports will return default constructed nodes, so that failure
+    // can be detected with a single conditional branch after a sequence of
+    // imports.
+    template <typename T> T importChecked(Error &Err, const T &From) {
+      // Don't attempt to import nodes if we hit an error earlier.
+      if (Err)
+        return T{};
+      Expected<T> MaybeVal = import(From);
+      if (!MaybeVal) {
+        Err = MaybeVal.takeError();
+        return T{};
+      }
+      return *MaybeVal;
+    }
+
     template<typename IIter, typename OIter>
     Error ImportArrayChecked(IIter Ibegin, IIter Iend, OIter Obegin) {
       using ItemT = std::remove_reference_t<decltype(*Obegin)>;
       for (; Ibegin != Iend; ++Ibegin, ++Obegin) {
         Expected<ItemT> ToOrErr = import(*Ibegin);
         if (!ToOrErr)
           return ToOrErr.takeError();
         *Obegin = *ToOrErr;
public:
     Expected<FunctionDecl *> FindFunctionTemplateSpecialization(
         FunctionDecl *FromFD);
 
     // Returns true if the given function has a placeholder return type and
     // that type is declared inside the body of the function.
     // E.g. auto f() { struct X{}; return X(); }
     bool hasAutoReturnTypeDeclaredInside(FunctionDecl *D);
   };
 
 template <typename InContainerTy>
 Error ASTNodeImporter::ImportTemplateArgumentListInfo(
     SourceLocation FromLAngleLoc, SourceLocation FromRAngleLoc,
     const InContainerTy &Container, TemplateArgumentListInfo &Result) {
   auto ToLAngleLocOrErr = import(FromLAngleLoc);
   if (!ToLAngleLocOrErr)
     return ToLAngleLocOrErr.takeError();
   auto ToRAngleLocOrErr = import(FromRAngleLoc);
if (!TOrErr)
     return TOrErr.takeError();
   ExpectedSLoc BeginLocOrErr = Import(FromTSI->getTypeLoc().getBeginLoc());
   if (!BeginLocOrErr)
     return BeginLocOrErr.takeError();
 
   return ToContext.getTrivialTypeSourceInfo(*TOrErr, *BeginLocOrErr);
 }
 
+// To use this object, it should be created before the new attribute is created,
+// and destructed after it is created. The construction already performs the
+// import of the data.
+template <typename T> struct AttrArgImporter {
+  AttrArgImporter<T>(const AttrArgImporter<T> &) = delete;
+  AttrArgImporter<T>(AttrArgImporter<T> &&) = default;
+  AttrArgImporter<T> &operator=(const AttrArgImporter<T> &) = delete;
+  AttrArgImporter<T> &operator=(AttrArgImporter<T> &&) = default;
+
+  AttrArgImporter(ASTNodeImporter &I, Error &Err, const T &From)
+      : To(I.importChecked(Err, From)) {}
+
+  const T &value() { return To; }
+
+private:
+  T To;
+};
+
+// To use this object, it should be created before the new attribute is created,
+// and destructed after it is created. The construction already performs the
+// import of the data. The array data is accessible in a pointer form, this form
+// is used by the attribute classes. This object should be created once for the
+// array data to be imported (the array size is not imported, just copied).
+template <typename T> struct AttrArgArrayImporter {
+  AttrArgArrayImporter<T>(const AttrArgArrayImporter<T> &) = delete;
+  AttrArgArrayImporter<T>(AttrArgArrayImporter<T> &&) = default;
+  AttrArgArrayImporter<T> &operator=(const AttrArgArrayImporter<T> &) = delete;
+  AttrArgArrayImporter<T> &operator=(AttrArgArrayImporter<T> &&) = default;
+
+  AttrArgArrayImporter(ASTNodeImporter &I, Error &Err,
+                       const llvm::iterator_range<T *> &From,
+                       unsigned ArraySize) {
+    if (Err)
+      return;
+    To.reserve(ArraySize);
+    Err = I.ImportContainerChecked(From, To);
+  }
+
+  T *value() { return To.data(); }
+
+private:
+  llvm::SmallVector<T, 2> To;
+};
+
+class AttrImporter {
+  Error Err = Error::success();
+  ASTImporter &Importer;
+  ASTNodeImporter NImporter;
+
+public:
+  AttrImporter(ASTImporter &I) : Importer(I), NImporter(I) {}
+
+  // Create an "importer" for an attribute parameter.
+  // Result of the 'value()' of that object is to be passed to the function
+  // 'createImpoprtedAttr', in the order that is expected by the attribute
+  // class.
+  template <class T> AttrArgImporter<T> importArg(const T &From) {
+    return AttrArgImporter<T>(NImporter, Err, From);
+  }
+
+  // Create an "importer" for an attribute parameter that has array type.
+  // Result of the 'value()' of that object is to be passed to the function
+  // 'createImpoprtedAttr', then the size of the array as next argument.
+  template <typename T>
+  AttrArgArrayImporter<T> importArrayArg(const llvm::iterator_range<T *> &From,
+                                         unsigned ArraySize) {
+    return AttrArgArrayImporter<T>(NImporter, Err, From, ArraySize);
+  }
+
+  // Create an attribute object with the specified arguments.
+  // The 'FromAttr' is the original (not imported) attribute, the 'ImportedArg'
+  // should be values that are passed to the 'Create' function of the attribute.
+  // (The 'Create' with 'ASTContext' first and 'AttributeCommonInfo' last is
+  // used here.) As much data is copied or imported from the old attribute
+  // as possible. The passed arguments should be already imported.
+  template <typename T, typename... Arg>
+  Expected<Attr *> createImportedAttr(const T *FromAttr, Arg &&...ImportedArg) {
+    static_assert(std::is_base_of<Attr, T>::value,
+                  "T should be subclass of Attr.");
+
+    const IdentifierInfo *ToAttrName = Importer.Import(FromAttr->getAttrName());
+    const IdentifierInfo *ToScopeName =
+        Importer.Import(FromAttr->getScopeName());
+    SourceRange ToAttrRange =
+        NImporter.importChecked(Err, FromAttr->getRange());
+    SourceLocation ToScopeLoc =
+        NImporter.importChecked(Err, FromAttr->getScopeLoc());
+
+    if (Err)
+      return std::move(Err);
+
+    AttributeCommonInfo ToI(ToAttrName, ToScopeName, ToAttrRange, ToScopeLoc,
+                            FromAttr->getParsedKind(), FromAttr->getSyntax(),
+                            FromAttr->getAttributeSpellingListIndex());
+    // The "SemanticSpelling" is not needed to be passed to the constructor.
+    // That value is recalculated from the SpellingListIndex if needed.
+    T *ToAttr = T::Create(Importer.getToContext(),
+                          std::forward<Arg>(ImportedArg)..., ToI);
+
+    ToAttr->setImplicit(FromAttr->isImplicit());
+    ToAttr->setPackExpansion(FromAttr->isPackExpansion());
+    if (auto *ToInheritableAttr = dyn_cast<InheritableAttr>(ToAttr))
+      ToInheritableAttr->setInherited(FromAttr->isInherited());
+
+    return ToAttr;
+  }
+};
+
 Expected<Attr *> ASTImporter::Import(const Attr *FromAttr) {
   Attr *ToAttr = nullptr;
+  // FIXME: Use AttrImporter as much as possible, try to remove the import
+  // of range from here.
   SourceRange ToRange;
   if (Error Err = importInto(ToRange, FromAttr->getRange()))
     return std::move(Err);
 
   // FIXME: Is there some kind of AttrVisitor to use here?
   switch (FromAttr->getKind()) {
   case attr::Aligned: {
     auto *From = cast<AlignedAttr>(FromAttr);
case attr::Format: {
     FormatAttr *To;
     IdentifierInfo *ToAttrType = Import(From->getType());
     To = FormatAttr::Create(ToContext, ToAttrType, From->getFormatIdx(),
                             From->getFirstArg(), ToRange, From->getSyntax());
     To->setInherited(From->isInherited());
     ToAttr = To;
     break;
   }
+
+  case attr::AssertCapability: {
+    const auto *From = cast<AssertCapabilityAttr>(FromAttr);
+    AttrImporter AI(*this);
+    Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
+        From, AI.importArrayArg(From->args(), From->args_size()).value(),
+        From->args_size());
+    if (ToAttrOrErr)
+      ToAttr = *ToAttrOrErr;
+    else
+      return ToAttrOrErr.takeError();
+    break;
+  }
+
   default:
     // FIXME: 'clone' copies every member but some of them should be imported.
     // Handle other Attrs that have parameters that should be imported.
     ToAttr = FromAttr->clone(ToContext);
     ToAttr->setRange(ToRange);
     break;
   }
   assert(ToAttr && "Attribute should be created.");

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ImportSourceLocations, OverwrittenFileBuffer) {
 
   // Make sure the imported buffer has the overwritten contents.
   SourceManager &ToSM = ToAST->getSourceManager();
   FileID ImportedID = ToSM.getFileID(ImportedLoc);
   EXPECT_EQ(Contents,
             ToSM.getBufferOrFake(ImportedID, SourceLocation()).getBuffer());
 }
 
+struct ImportAttributes : public ASTImporterOptionSpecificTestBase {
+  void checkAttrImportCommon(const Attr *From, const Attr *To,
+                             const Decl *ToD) {
+
+    // Verify that dump does not crash because invalid data.
+    ToD->dump(llvm::nulls());
+
+    EXPECT_EQ(From->getParsedKind(), To->getParsedKind());
+    EXPECT_EQ(From->getSyntax(), To->getSyntax());
+    if (From->getAttrName()) {
+      EXPECT_TRUE(To->getAttrName());
+      EXPECT_STREQ(From->getAttrName()->getNameStart(),
+                   To->getAttrName()->getNameStart());
+    } else {
+      EXPECT_FALSE(To->getAttrName());
+    }
+    if (From->getScopeName()) {
+      EXPECT_TRUE(To->getScopeName());
+      EXPECT_STREQ(From->getScopeName()->getNameStart(),
+                   To->getScopeName()->getNameStart());
+    } else {
+      EXPECT_FALSE(To->getScopeName());
+    }
+    EXPECT_EQ(From->getSpellingListIndex(), To->getSpellingListIndex());
+    EXPECT_STREQ(From->getSpelling(), To->getSpelling());
+    EXPECT_EQ(From->isInherited(), To->isInherited());
+    EXPECT_EQ(From->isImplicit(), To->isImplicit());
+    EXPECT_EQ(From->isPackExpansion(), To->isPackExpansion());
+    EXPECT_EQ(From->isLateParsed(), To->isLateParsed());
+  }
+
+  template <class DT, class AT>
+  void importAttr(const char *Code, AT *&FromAttr, AT *&ToAttr) {
+    static_assert(std::is_base_of<Attr, AT>::value, "AT should be an Attr");
+    static_assert(std::is_base_of<Decl, DT>::value, "DT should be a Decl");
+
+    Decl *FromTU = getTuDecl(Code, Lang_CXX11, "input.cc");
+    DT *FromD =
+        FirstDeclMatcher<DT>().match(FromTU, namedDecl(hasName("test")));
+    ASSERT_TRUE(FromD);
+
+    DT *ToD = Import(FromD, Lang_CXX11);
+    ASSERT_TRUE(ToD);
+
+    FromAttr = FromD->template getAttr<AT>();
+    ToAttr = ToD->template getAttr<AT>();
+    ASSERT_TRUE(FromAttr);
+    EXPECT_TRUE(ToAttr);
+
+    checkAttrImportCommon(FromAttr, ToAttr, ToD);
+  }
+
+  template <class T> void checkImported(const T *From, const T *To) {
+    EXPECT_TRUE(To);
+    EXPECT_NE(From, To);
+  }
+
+  template <class T>
+  void checkImportVariadicArg(const llvm::iterator_range<T **> &From,
+                              const llvm::iterator_range<T **> &To) {
+    for (auto FromI = From.begin(), ToI = To.begin(); FromI != From.end();
+         ++FromI, ++ToI) {
+      ASSERT_NE(ToI, To.end());
+      checkImported(*FromI, *ToI);
+    }
+  }
+};
+
+template <>
+void ImportAttributes::checkImported<Decl>(const Decl *From, const Decl *To) {
+  EXPECT_TRUE(To);
+  EXPECT_NE(From, To);
+  EXPECT_EQ(To->getTranslationUnitDecl(),
+            ToAST->getASTContext().getTranslationUnitDecl());
+}
+
+// FIXME: Use ImportAttributes for this test.
 TEST_P(ASTImporterOptionSpecificTestBase, ImportExprOfAlignmentAttr) {
   // Test if import of these packed and aligned attributes does not trigger an
   // error situation where source location from 'From' context is referenced in
   // 'To' context through evaluation of the alignof attribute.
   // This happens if the 'alignof(A)' expression is not imported correctly.
   Decl *FromTU = getTuDecl(
       R"(
       struct __attribute__((packed)) A { int __attribute__((aligned(8))) X; };
TEST_P(ASTImporterOptionSpecificTestBase, ImportExprOfAlignmentAttr) {
   auto *ToA = FirstDeclMatcher<CXXRecordDecl>().match(
       ToD->getTranslationUnitDecl(),
       cxxRecordDecl(hasName("A"), unless(isImplicit())));
   // Ensure that 'struct A' was imported (through reference from attribute of
   // 'S').
   EXPECT_TRUE(ToA);
 }
 
+// FIXME: Use ImportAttributes for this test.
 TEST_P(ASTImporterOptionSpecificTestBase, ImportFormatAttr) {
   Decl *FromTU = getTuDecl(
       R"(
       int foo(const char * fmt, ...)
       __attribute__ ((__format__ (__scanf__, 1, 2)));
       )",
       Lang_CXX03, "input.cc");
   auto *FromD = FirstDeclMatcher<FunctionDecl>().match(
TEST_P(ASTImporterOptionSpecificTestBase, ImportFormatAttr) {
   EXPECT_EQ(FromAttr->isInherited(), ToAttr->isInherited());
   EXPECT_EQ(FromAttr->isPackExpansion(), ToAttr->isPackExpansion());
   EXPECT_EQ(FromAttr->isImplicit(), ToAttr->isImplicit());
   EXPECT_EQ(FromAttr->getSyntax(), ToAttr->getSyntax());
   EXPECT_EQ(FromAttr->getAttributeSpellingListIndex(),
             ToAttr->getAttributeSpellingListIndex());
   EXPECT_EQ(FromAttr->getType()->getName(), ToAttr->getType()->getName());
 }
+
+TEST_P(ImportAttributes, ImportAssertCapability) {
+  AssertCapabilityAttr *FromAttr, *ToAttr;
+  importAttr<FunctionDecl>(
+      "void test(int A1, int A2) __attribute__((assert_capability(A1, A2)));",
+      FromAttr, ToAttr);
+  checkImportVariadicArg(FromAttr->args(), ToAttr->args());
+}
+
 template <typename T>
 auto ExtendWithOptions(const T &Values, const std::vector<std::string> &Args) {
   auto Copy = Values;
   for (std::vector<std::string> &ArgV : Copy) {
     for (const std::string &Arg : Args) {
       ArgV.push_back(Arg);
     }
   }
INSTANTIATE_TEST_SUITE_P(ParameterizedTests, LLDBLookupTest,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportSourceLocations,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportWithExternalSource,
                          DefaultTestValuesForRunOptions);
 
+INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportAttributes,
+                         DefaultTestValuesForRunOptions);
+
 } // end namespace ast_matchers
 } // end namespace clang