[ASTImporter] Unnamed structs import

[szepet]

Summary

I think we should check the unnamed structs locations/positions in the context otherwise the Importer thinks in case of the "struct Unnamed" test case that the inner structs types (R and S) should be equivalent and would give a warning because of type mismatch.

On the other hand it is not enough since the findUntaggedStructOrUnionIndex should be able to deal with nested structs when the RecordType is deep inside an ElaboratedType.

With these changes I do not see the point of the removed else statement (L3079).
Test cases added.

Important to notice that:

union { int i; float f; };

is an anonymous (and unnamed) union but

union { int i; float f; } U;

is not anonymous but I referred to it as unnamed. (Hopefully good wording.)

[a.sidorin]

Thank you for catching this! Looks mostly good, I just have some inline questions.

lib/AST/ASTImporter.cpp

1225 ↗

(On Diff #91509)

Iterating over ElaboratedTypes rings a bell for me that we may need to use getCanonicalType(). Does this code deals with typedefs well?

test/ASTMerge/struct/Inputs/struct2.c

62 ↗

(On Diff #91509)

Please leave a space after // in comments.

112 ↗

(On Diff #91509)

Please point that mismatch should occur here, it is very hard to compare structures manually.

[szepet]

Nits resolved, dependency added in order to not to conflict the 2 patches.

[a.sidorin]

Looks good. I will commit this if there will be no comments from other reviewers. Thank you!

[szepet]

lib/AST/ASTImporter.cpp

1225 ↗

(On Diff #91509)

Well, this function does not deal with typedefs but it checked via IsStructuralMatch function call (Line 3055). This is why the importer can recognize that they are different structs. Furthermore this function "returns an empty option if the context is not a record". And typedefs inside structs only allowed in C++ where an unnamed struct typedef would seem strange. So I dont think this function needs to deal with typedefs but if you prefer that way then I can write an ifstmt getting the underlying type in case of a typedef.

[xazax.hun]

Unfortunately this patch can not be applied to current top of tree (due to conflicts in the tests).

[a.sidorin]

Seems like this patch breaks windows buildbots: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/732/steps/test-check-all/logs/stdio

[xazax.hun]

Reopened until a fix for windows builds.

[dyung]

In D30876#717256, @xazax.hun wrote:

Reopened until a fix for windows builds.

If you aren't going to fix this soon, could you please revert the change so that it doesn't mask other potential problems?

[xazax.hun]

In D30876#717499, @dyung wrote:

In D30876#717256, @xazax.hun wrote:

Reopened until a fix for windows builds.

If you aren't going to fix this soon, could you please revert the change so that it doesn't mask other potential problems?

I did revert that before I reopened this.

[xazax.hun]

Ok, https://reviews.llvm.org/D39886 is an independent fix for the same issue. It might be useful to add the test cases from that patch to this one as Aleksei pointed out.
The current status of this one, the windows build bot failed, but we could not reproduce it (even on windows). Is there someone who could reproduce the fail? Should we just recommit this hoping that the fail was unrelated to this patch?

[a.sidorin]

Hi Gabor and Peter,
I cannot reproduce the issue. I have updated the patch in my local repo and will try to re-commit it tomorrow. Right now, all buildbots are red.

[dyung]

FYI, when this was commited in r329301, it again caused failures on at least the PS4 Windows bot. The problem was due to the explicit "/struct" being searched for in your CHECK lines. I have committed a fix in r329361 which seems to fix it by removing the leading '/' character, so it gets rolled into the regex PATH_TO_INPUTS.

[a.sidorin]

Hi Douglas,

Could you provide the reference to the buildbot? I haven't received any blame e-mails related to the change after the commit.

[dyung]

In D30876#1059421, @a.sidorin wrote:

Hi Douglas,

Could you provide the reference to the buildbot? I haven't received any blame e-mails related to the change after the commit.

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16288

Your change was initially tested in http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16279, however due to a build failure, your test was not actually run until the build failure was fixed in the above run.

[a.sidorin]

Your change was initially tested in http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16279, however due to a build failure, your test was not actually run until the build failure was fixed in the above run.

Oh, I see now. Thank you!

cfe/trunk/lib/AST/ASTImporter.cpp

for (unsigned I = 0, N = FoundDecls.size(); I != N; ++I) {
       
       Decl *Found = FoundDecls[I];
       if (TypedefNameDecl *Typedef = dyn_cast<TypedefNameDecl>(Found)) {
         if (const TagType *Tag = Typedef->getUnderlyingType()->getAs<TagType>())
           Found = Tag->getDecl();
       }
       
       if (RecordDecl *FoundRecord = dyn_cast<RecordDecl>(Found)) {
-        if (D->isAnonymousStructOrUnion() && 
-            FoundRecord->isAnonymousStructOrUnion()) {
-          // If both anonymous structs/unions are in a record context, make sure
+        if (!SearchName) {
+          // If both unnamed structs/unions are in a record context, make sure
           // they occur in the same location in the context records.
           if (Optional<unsigned> Index1 =
                   StructuralEquivalenceContext::findUntaggedStructOrUnionIndex(
                       D)) {
             if (Optional<unsigned> Index2 = StructuralEquivalenceContext::
                     findUntaggedStructOrUnionIndex(FoundRecord)) {
               if (*Index1 != *Index2)
                 continue;

cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp

if (F->isAnonymousStructOrUnion()) {
         break;
       ++Index;
       continue;
     }
 
     // If the field looks like this:
     // struct { ... } A;
     QualType FieldType = F->getType();
+    // In case of nested structs.
+    while (const auto *ElabType = dyn_cast<ElaboratedType>(FieldType))
+      FieldType = ElabType->getNamedType();
+
     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;
       }

cfe/trunk/test/ASTMerge/struct/Inputs/struct1.c

 
 // Mismatch
 typedef struct {
   Float i; // Mismatch here.
   float f;
 } S13;
 
 S13 x13;
+
+// Matches
+struct Unnamed {
+  union {
+    struct {
+      int i;
+    } S;
+    struct {
+      float i;
+    } R;
+  } U;
+} x14;
+
+// Matches
+struct DeepUnnamed {
+  union {
+    union {
+      struct {
+        long i;
+      } S;
+      struct {
+        int i;
+      } R;
+    } U1;
+    union {
+      struct {
+        long i;
+      } S;
+      struct {
+        float i;
+      } T;
+    } U2;
+  } U;
+  struct {
+    long i;
+  } V;
+} x15;
+
+// Mismatch due to unnamed struct used internally
+struct DeepUnnamedError {
+  union {
+    union {
+      struct {
+        long i;
+      } S;
+      struct {
+        int i;
+      } R;
+    } U1;
+    union {
+      struct {
+        long i; // Mismatch here.
+      } S;
+      struct {
+        float i;
+      } T;
+    } U2;
+  } U;
+  struct {
+    long i;
+  } V;
+} x16;

cfe/trunk/test/ASTMerge/struct/Inputs/struct2.c

 
 // Mismatch
 typedef struct {
   int i; // Mismatch here.
   float f;
 } S13;
 
 S13 x13;
+
+// Matches
+struct Unnamed {
+  union {
+    struct {
+      int i;
+    } S;
+    struct {
+      float i;
+    } R;
+  } U;
+} x14;
+
+// Matches
+struct DeepUnnamed {
+  union {
+    union {
+      struct {
+        long i;
+      } S;
+      struct {
+        int i;
+      } R;
+    } U1;
+    union {
+      struct {
+        long i;
+      } S;
+      struct {
+        float i;
+      } T;
+    } U2;
+  } U;
+  struct {
+    long i;
+  } V;
+} x15;
+
+// Mismatch due to unnamed struct used internally
+struct DeepUnnamedError {
+  union {
+    union {
+      struct {
+        long i;
+      } S;
+      struct {
+        int i;
+      } R;
+    } U1;
+    union {
+      struct {
+        float i; // Mismatch here.
+      } S;
+      struct {
+        float i;
+      } T;
+    } U2;
+  } U;
+  struct {
+    long i;
+  } V;
+} x16;

cfe/trunk/test/ASTMerge/struct/test.c

 // CHECK: struct2.c:53:43: note: field 'Deeper' has type 'struct DeeperError *' here
 // CHECK: struct2.c:54:3: error: external variable 'xDeep' declared with incompatible types in different translation units ('struct DeepError' vs. 'struct DeepError')
 // CHECK: struct1.c:57:3: note: declared here with type 'struct DeepError'
 // CHECK: struct1.c:74:9: warning: type 'S13' has incompatible definitions in different translation units
 // CHECK: struct1.c:75:9: note: field 'i' has type 'Float' (aka 'float') here
 // CHECK: struct2.c:72:7: note: field 'i' has type 'int' here
 // CHECK: struct2.c:76:5: error: external variable 'x13' declared with incompatible types in different translation units ('S13' vs. 'S13')
 // CHECK: struct1.c:79:5: note: declared here with type 'S13'
-// CHECK: 9 warnings and 8 errors generated
+// CHECK: struct1.c:130:7: warning: type 'struct DeepUnnamedError::(anonymous at [[PATH_TO_INPUTS:.+]]/struct1.c:130:7)' has incompatible definitions in different translation units
+// CHECK: struct1.c:131:14: note: field 'i' has type 'long' here
+// CHECK: struct2.c:128:15: note: field 'i' has type 'float' here
+// CHECK: struct1.c:129:5: warning: type 'union DeepUnnamedError::(anonymous at [[PATH_TO_INPUTS]]/struct1.c:129:5)' has incompatible definitions in different translation units
+// CHECK: struct1.c:132:9: note: field 'S' has type 'struct (anonymous struct at [[PATH_TO_INPUTS]]/struct1.c:130:7)' here
+// CHECK: struct2.c:129:9: note: field 'S' has type 'struct (anonymous struct at [[PATH_TO_INPUTS]]/struct2.c:127:7)' here
+// CHECK: struct2.c:138:3: error: external variable 'x16' declared with incompatible types in different translation units ('struct DeepUnnamedError' vs. 'struct DeepUnnamedError')
+// CHECK: struct1.c:141:3: note: declared here with type 'struct DeepUnnamedError'
+// CHECK: 11 warnings and 9 errors generated