[ASTImporter] FriendDecl importing improvements

[szepet]

Summary

Set FriendObjectKind in case of decls.
RD->pushFriendDecl(FrD); statement removed from L2240 since it is done in function Create(..).
IsStructurallyEquivalent function(s) of FriendDecl and dependent decls (RecordDecl, FunctionDecl) added.
Test cases added,

[szepet]

ping

[spyffe]

Sorry for the late review, I saw your ping but the last week was busy. Anyway, thanks for the tests!

lib/AST/ASTImporter.cpp

2219

Since getFriendObjectKind() returns an enum, in my opinion it's better style to check explicitly whether it != FOK_None.

2222

From a close reading of setObjectOfFriendDecl(), it looks to me like we might not set the IDNS_Tag or IDNS_Ordinary bits in IdentifierNamespace since we're passing PerformFriendInjection = false and since getPreviousDecl() might return nullptr in ToContext.

This means that getFriendObjectKind() might return FOK_Undeclared whereas it returned FOK_Declared for the original. Does this matter?

lib/AST/ASTStructuralEquivalence.cpp

922

Would it be appropriate to emit a diagnostic here as well?

1152

What if D1->getFriendDecl() != nullptr but D2->getFriendType() != nullptr or vice versa?
I wouldn't want to return true in that case…

1162

For attributes on the function (e.g., __attribute__ ((always_inline))) are these relevant to structural equvalence? Or does really only the type matter?

1372

What about if FrD1->getFriendDecl() && FrD2->getFriendDecl()?

[szepet]

Updates based on comments.

[xazax.hun]

Could you add some context?

Generally LGTM!

One high level comment, I think we need to modify the importing of CXXRecordDecls to properly support friend declarations. When we are building the redeclarable chain of CXXRecordDecl declarations I suspect we also need to consider names in the IDNS_TagFriend namespace.

[martong]

lib/AST/ASTImporter.cpp

2260

This will assert unless the IdentifierNamespace is properly imported. See https://github.com/Ericsson/clang/pull/266/files#diff-7f5f26dbdb3322c6308b1018ff0dbe5aR6735

[xazax.hun]

Note that changes from https://reviews.llvm.org/D44100 might affect this.

[szepet]

Rebase + set the IDNS in a more general way.

[martong]

LGTM!

[a.sidorin]

Looks good to me as well. Thank you!

[szepet]

I have commited this patch (r330847) but unfortunately "refactored" the differential revision link from the commit message out. Added it as a dependent commit.

include/clang/Basic/DiagnosticASTKinds.td

def err_odr_function_type_inconsistent : Error<
   "external function %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
 def warn_odr_tag_type_inconsistent : Warning<
   "type %0 has incompatible definitions in different translation units">,
   InGroup<DiagGroup<"odr">>;
 def note_odr_tag_kind_here: Note<
   "%0 is a %select{struct|interface|union|class|enum}1 here">;
 def note_odr_field : Note<"field %0 has type %1 here">;
+def note_odr_friend : Note<"friend declared here">;
 def note_odr_missing_field : Note<"no corresponding field here">;
+def note_odr_missing_friend : Note<"no corresponding friend here">;
 def note_odr_bit_field : Note<"bit-field %0 with type %1 and length %2 here">;
 def note_odr_not_bit_field : Note<"field %0 is not a bit-field">;
 def note_odr_base : Note<"class has base type %0">;
 def note_odr_virtual_base : Note<
   "%select{non-virtual|virtual}0 derivation here">;
 def note_odr_missing_base : Note<"no corresponding base class here">;
 def note_odr_number_of_bases : Note<
   "class has %0 base %plural{1:class|:classes}0">;

lib/AST/ASTImporter.cpp

if (D->getFriendDecl() && ImportedFriend->getFriendDecl()) {
             ImportedFriend->getFriendType()->getType(), true))
         return Importer.Imported(D, ImportedFriend);
     }
     ImportedFriend = ImportedFriend->getNextFriend();
   }
 
   // Not found. Create it.
   FriendDecl::FriendUnion ToFU;
-  if (NamedDecl *FriendD = D->getFriendDecl())
-    ToFU = cast_or_null<NamedDecl>(Importer.Import(FriendD));
+  if (NamedDecl *FriendD = D->getFriendDecl()) {
+    auto ToFriendD = cast_or_null<NamedDecl>(Importer.Import(FriendD));
+    if (ToFriendD && FriendD->getFriendObjectKind())
+      ToFriendD->setObjectOfFriendDecl();
+     ToFU = ToFriendD;
+  }
   else
     ToFU = Importer.Import(D->getFriendType());
   if (!ToFU)
     return nullptr;
 
   SmallVector<TemplateParameterList *, 1> ToTPLists(D->NumTPLists);
   TemplateParameterList **FromTPLists =
       D->getTrailingObjects<TemplateParameterList *>();
   for (unsigned I = 0; I < D->NumTPLists; I++) {
     TemplateParameterList *List = ImportTemplateParameterList(FromTPLists[I]);
     if (!List)
       return nullptr;
     ToTPLists[I] = List;
   }
 
   FriendDecl *FrD = FriendDecl::Create(Importer.getToContext(), DC,
                                        Importer.Import(D->getLocation()),
                                        ToFU, Importer.Import(D->getFriendLoc()),
                                        ToTPLists);
 
   Importer.Imported(D, FrD);
-  RD->pushFriendDecl(FrD);
 
   FrD->setAccess(D->getAccess());
   FrD->setLexicalDeclContext(LexicalDC);
   LexicalDC->addDeclInternal(FrD);
   return FrD;
 }
 
 Decl *ASTNodeImporter::VisitObjCIvarDecl(ObjCIvarDecl *D) {
   // Import the major distinguishing characteristics of an ivar.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
   if (ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return nullptr;
   if (ToD)
     return ToD;
 
   // Determine whether we've already imported this ivar
   SmallVector<NamedDecl *, 2> FoundDecls;
   DC->getRedeclContext()->localUncachedLookup(Name, FoundDecls);
   for (unsigned I = 0, N = FoundDecls.size(); I != N; ++I) {
     if (ObjCIvarDecl *FoundIvar = dyn_cast<ObjCIvarDecl>(FoundDecls[I])) {
       if (Importer.IsStructurallyEquivalent(D->getType(),
                                             FoundIvar->getType())) {

lib/AST/ASTStructuralEquivalence.cpp

if (CXXRecordDecl *D2CXX = dyn_cast<CXXRecordDecl>(D2)) {
             Context.Diag2(Base2->getLocStart(), diag::note_odr_virtual_base)
                 << Base2->isVirtual() << Base2->getSourceRange();
             Context.Diag1(Base1->getLocStart(), diag::note_odr_base)
                 << Base1->isVirtual() << Base1->getSourceRange();
           }
           return false;
         }
       }
+
+      // Check the friends for consistency.
+      CXXRecordDecl::friend_iterator Friend2 = D2CXX->friend_begin(),
+              Friend2End = D2CXX->friend_end();
+      for (CXXRecordDecl::friend_iterator Friend1 = D1CXX->friend_begin(),
+                   Friend1End = D1CXX->friend_end();
+           Friend1 != Friend1End; ++Friend1, ++Friend2) {
+        if (Friend2 == Friend2End) {
+          if (Context.Complain) {
+            Context.Diag2(D2->getLocation(),
+                          diag::warn_odr_tag_type_inconsistent)
+                    << Context.ToCtx.getTypeDeclType(D2CXX);
+            Context.Diag1((*Friend1)->getFriendLoc(), diag::note_odr_friend);
+            Context.Diag2(D2->getLocation(), diag::note_odr_missing_friend);
+          }
+          return false;
+        }
+
+        if (!IsStructurallyEquivalent(Context, *Friend1, *Friend2))
+          return false;
+      }
+
+      if (Friend2 != Friend2End) {
+        if (Context.Complain) {
+          Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+                  << Context.ToCtx.getTypeDeclType(D2);
+          Context.Diag2((*Friend2)->getFriendLoc(), diag::note_odr_friend);
+          Context.Diag1(D1->getLocation(), diag::note_odr_missing_friend);
+        }
+        return false;
+      }
     } else if (D1CXX->getNumBases() > 0) {
       if (Context.Complain) {
         Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
             << Context.ToCtx.getTypeDeclType(D2);
         const CXXBaseSpecifier *Base1 = D1CXX->bases_begin();
         Context.Diag1(Base1->getLocStart(), diag::note_odr_base)
             << Base1->getType() << Base1->getSourceRange();
         Context.Diag2(D2->getLocation(), diag::note_odr_missing_base);
if (!IsStructurallyEquivalent(Context, D1->getTemplateParameters(),
                                 D2->getTemplateParameters()))
     return false;
 
   // Check the templated declaration.
   return Context.IsStructurallyEquivalent(D1->getTemplatedDecl(),
                                           D2->getTemplatedDecl());
 }
 
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+                                     FriendDecl *D1, FriendDecl *D2) {
+  if (D1->getFriendType() && D2->getFriendType()) {
+    if (!::IsStructurallyEquivalent(Context,
+                                    D1->getFriendType()->getType(),
+                                    D2->getFriendType()->getType()))
+      return false;
+  } else if (D1->getFriendDecl() && D2->getFriendDecl()) {
+    if (!::IsStructurallyEquivalent(Context, D1->getFriendDecl(),
+                                    D2->getFriendDecl()))
+      return false;
+  }
+
+  return true;
+}
+
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+                                     FunctionDecl *D1, FunctionDecl *D2) {
+  if (!::IsStructurallyEquivalent(Context, D1->getType(), D2->getType()))
+    return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two declarations.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      Decl *D1, Decl *D2) {
   // FIXME: Check for known structural equivalences via a callback of some sort.
 
   // Check whether we already know that these two declarations are not
   // structurally equivalent.
   if (Context.NonEquivalentDecls.count(
if (RecordDecl *Record1 = dyn_cast<RecordDecl>(D1)) {
       if (TemplateTemplateParmDecl *TTP2 =
               dyn_cast<TemplateTemplateParmDecl>(D2)) {
         if (!::IsStructurallyEquivalent(*this, TTP1, TTP2))
           Equivalent = false;
       } else {
         // Kind mismatch.
         Equivalent = false;
       }
+    } else if (FunctionDecl *FD1 = dyn_cast<FunctionDecl>(D1)) {
+      if (FunctionDecl *FD2 = dyn_cast<FunctionDecl>(D2)) {
+        if (!::IsStructurallyEquivalent(FD1->getIdentifier(),
+                                        FD2->getIdentifier()))
+          Equivalent = false;
+        if (!::IsStructurallyEquivalent(*this, FD1, FD2))
+          Equivalent = false;
+      } else {
+        // Kind mismatch.
+        Equivalent = false;
+      }
+    } else if (FriendDecl *FrD1 = dyn_cast<FriendDecl>(D1)) {
+      if (FriendDecl *FrD2 = dyn_cast<FriendDecl>(D2)) {
+        if (FrD1->getFriendType() && FrD2->getFriendType()) {
+          if (!::IsStructurallyEquivalent(*this, FrD1, FrD2))
+            Equivalent = false;
+        } else {
+          // Kind mismatch.
+          Equivalent = false;
+        }
+      }
     }
 
     if (!Equivalent) {
       // Note that these two declarations are not equivalent (and we already
       // know about it).
       NonEquivalentDecls.insert(
           std::make_pair(D1->getCanonicalDecl(), D2->getCanonicalDecl()));
       return true;
     }
     // FIXME: Check other declaration kinds!
   }
 
   return false;
 }
 } // namespace clang

test/ASTMerge/class/Inputs/class1.cpp

struct C {
   C(const C&);
   C &operator=(C&);
   ~C();
 };
 
 enum E {
   b = 1
 };
+
+//Friend import tests
+void f();
+int g(int a);
+struct X;
+struct Y;
+
+struct F1 {
+public:
+  int x;
+  friend struct X;
+  friend int g(int);
+  friend void f();
+};
+
+struct F2 {
+public:
+  int x;
+  friend struct X;
+  friend void f();
+};
+
+struct F3 {
+public:
+  int x;
+  friend int g(int);
+  friend void f();
+};

test/ASTMerge/class/Inputs/class2.cpp

 struct A {
   public:
     int x;
 };
 
 struct B : A {
   int y;
   int foo();
 };
 
 enum E {
   a = 0,
   b = 1
 };
+
+//Friend import tests
+void f();
+int g(int a);
+struct X;
+struct Y;
+
+struct F1 {
+public:
+  int x;
+  friend struct X;
+  friend int g(int);
+  friend void f();
+};
+
+struct F2 {
+public:
+  int x;
+  friend struct X;
+};
+
+struct F3 {
+public:
+  int x;
+  friend void f();
+};

test/ASTMerge/class/test.cpp

 // RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/class1.cpp
 // RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/class2.cpp
 // RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s
 // RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 -Wno-odr -Werror
 
 // CHECK: class1.cpp:6:8: warning: type 'B' has incompatible definitions in different translation units
 // CHECK: class1.cpp:7:9: note: field 'y' has type 'float' here
 // CHECK: class2.cpp:7:7: note: field 'y' has type 'int' here
 
 // FIXME: we should also complain about mismatched types on the method
 
 // CHECK: class1.cpp:18:6: warning: type 'E' has incompatible definitions in different translation units
 // CHECK: class1.cpp:19:3: note: enumerator 'b' with value 1 here
 // CHECK: class2.cpp:12:3: note: enumerator 'a' with value 0 here
+
+// CHECK: class1.cpp:36:8: warning: type 'F2' has incompatible definitions in different translation units
+// CHECK: class1.cpp:39:3: note: friend declared here
+// CHECK: class2.cpp:30:8: note: no corresponding friend here
+
+// CHECK: class1.cpp:43:8: warning: type 'F3' has incompatible definitions in different translation units
+// CHECK: class1.cpp:46:3: note: friend declared here
+// CHECK: class2.cpp:36:8: note: no corresponding friend here
+
+// CHECK: 4 warnings generated.