[ASTImporter] Properly report the locations of anonymous structs declared as part of named fields

[spyffe]

Summary

When importing classes and structs with anonymous structs, it is critical that distinct anonymous structs remain distinct despite having similar layout.
This is already ensured by distinguishing based on their placement in the parent struct, using the function findAnonymousStructOrUnionIndex.
The problem is that this function only handles

class Foo { struct { int a; } }

and not

class Foo { struct { int a; } var; }

Both need to be handled, and this patch fixes that. The test case ensures that this functionality doesn't regress.

[manmanren]

I am not sure if we should handle this inside findAnonymousStructOrUnionIndex. Here is the definition of anonymous structure I found: An unnamed member whose type specifier is a structure specifier with no tag is called an anonymous structure.

Cheers,
Manman

lib/AST/ASTImporter.cpp

1054

Use const auto * here?

1058

Combining the two ifs?

1062

Should we continue here instead of increasing the Index?

Maybe we can reorder here to reduce nesting?
if (F->isAnonymousStructOrUnion() && ...)

break;

if (F->isAnonymousStructOrUnion())

Index++;
continue;

// If the field looks like this: ...

test/ASTMerge/anonymous-fields.cpp

4

Does this test fail without the change in ASTImporter.cpp? Should we check the output of the AST merging?

[spyffe]

Thank you very much for your review, Manman! I can implement all your individual fixes, those look fine. In answer to two of your bigger questions, though:

• I see what you mean about the definition of an anonymous structure. It looks like our structure is an untagged structure, not an anonymous one. That said, this change is safe for all the places that use this function, so it may be appropriate to change the name to findAnonymousStructOrUnionIndex. What do you think?
• The test case is unfortunately only an approximation to the more complicated behavior that occurs in lldb. The difference is that the testing infrastructure inside Clang does not implement an ExternalASTSource, which LLDB does. As a result, I've used the test case to verify that we don't break parsing by making this change.

[spyffe]

I mean findUntaggedStructOrUnionIndex

[manmanren]

In D22270#483027, @spyffe wrote:

Thank you very much for your review, Manman! I can implement all your individual fixes, those look fine. In answer to two of your bigger questions, though:

• I see what you mean about the definition of an anonymous structure. It looks like our structure is an untagged structure, not an anonymous one. That said, this change is safe for all the places that use this function, so it may be appropriate to change the name to findAnonymousStructOrUnionIndex. What do you think?

Sounds good!

• The test case is unfortunately only an approximation to the more complicated behavior that occurs in lldb. The difference is that the testing infrastructure inside Clang does not implement an ExternalASTSource, which LLDB does. As a result, I've used the test case to verify that we don't break parsing by making this change.

Ok, thanks for the explanation!

Cheers,
Manman

[spyffe]

Applied Manman's changes:

• const auto* instead of const RecordType*
• kept the if's separate because I...
• ...moved the Index++ and continue in so that we only increment the counter when we're really dealing with something untagged
• changed the function name to findUntaggedStructOrUnionIndex

[manmanren]

LGTM.

Thanks,
Manman

[Eugene.Zelenko]

Committed in r275460.

lib/AST/ASTImporter.cpp

 /// including the next assigned index (if none of them match). Returns an
 /// empty option if the context is not a record, i.e.. if the anonymous
 /// struct/union is at namespace or block scope.
-static Optional<unsigned> findAnonymousStructOrUnionIndex(RecordDecl *Anon) {
+static Optional<unsigned> findUntaggedStructOrUnionIndex(RecordDecl *Anon) {
   ASTContext &Context = Anon->getASTContext();
   QualType AnonTy = Context.getRecordType(Anon);
 

   unsigned Index = 0;
   for (const auto *D : Owner->noload_decls()) {
     const auto *F = dyn_cast<FieldDecl>(D);
-    if (!F || !F->isAnonymousStructOrUnion())
+    if (!F)
       continue;
 
-    if (Context.hasSameType(F->getType(), AnonTy))
-      break;
+    if (F->isAnonymousStructOrUnion()) {
+      if (Context.hasSameType(F->getType(), AnonTy))
+        break;
+      ++Index;
+      continue;
+    }
 
-    ++Index;
+    // If the field looks like this:
+    // struct { ... } A;
+    QualType FieldType = F->getType();
+    if (const auto *RecType = dyn_cast<RecordType>(FieldType)) {
+      const RecordDecl *RecDecl = RecType->getDecl();
+      if (RecDecl->getDeclContext() == Owner &&
+          !RecDecl->getIdentifier()) {
+        if (Context.hasSameType(FieldType, AnonTy))
+          break;
+        ++Index;
+        continue;
+      }
+    }
   }
 
   return Index;

   if (D1->isAnonymousStructOrUnion() && D2->isAnonymousStructOrUnion()) {
     // If both anonymous structs/unions are in a record context, make sure
     // they occur in the same location in the context records.
-    if (Optional<unsigned> Index1 = findAnonymousStructOrUnionIndex(D1)) {
-      if (Optional<unsigned> Index2 = findAnonymousStructOrUnionIndex(D2)) {
+    if (Optional<unsigned> Index1 = findUntaggedStructOrUnionIndex(D1)) {
+      if (Optional<unsigned> Index2 = findUntaggedStructOrUnionIndex(D2)) {
         if (*Index1 != *Index2)
           return false;
       }

           // If both anonymous structs/unions are in a record context, make sure
           // they occur in the same location in the context records.
           if (Optional<unsigned> Index1
-              = findAnonymousStructOrUnionIndex(D)) {
+              = findUntaggedStructOrUnionIndex(D)) {
             if (Optional<unsigned> Index2 =
-                    findAnonymousStructOrUnionIndex(FoundRecord)) {
+                    findUntaggedStructOrUnionIndex(FoundRecord)) {
               if (*Index1 != *Index2)
                 continue;
             }

test/ASTMerge/Inputs/anonymous-fields1.cpp

+class A {
+public:
+  struct { int foo; } f;
+  struct { int foo; } g;
+};

test/ASTMerge/Inputs/anonymous-fields2.cpp

+class A {
+public:
+  struct { int foo; } f;
+  struct { int foo; } g;
+};
+
+inline int useA(A &a) {
+  return (a.f.foo + a.g.foo);
+}

test/ASTMerge/anonymous-fields.cpp

+// RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/anonymous-fields1.cpp
+// RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/anonymous-fields2.cpp
+// RUN: %clang_cc1 -emit-obj -o /dev/null -ast-merge %t.1.ast -ast-merge %t.2.ast %s
+// expected-no-diagnostics