[ASTImporter] Fix wrong conflict detections for unnamed structures

[tk1012]

Summary

This patch fixes wrong conflict detections for unnamed structures.
Current ASTImporter mistakenly identifies two different unnamed structs as the same one.
This is because ASTImporter checks the name of each RecordDecl for the conflict identification and the both of them have the same "unnamed" name.
To avoid this, this patch skips the confliction check if SearchName is the null string and also adds a test case.

[xazax.hun]

Doug added anonymous structure handling, added as a reviewer in case he wants to have a look.

[a.sidorin]

Hello Takafumi,

Thank you for this patch. Looks like you're trying to disable lookup for similar structures if the structure is anonymous but there are two things I'm worrying about this solution.

1. Are import conflicts for anonymous structures resolved correctly?
2. Are equal structures present in both TUs imported correctly, without duplication?

Could you please check this and add tests if possible?

lib/AST/ASTImporter.cpp

1634

D->isAnonymousStructOrUnion()?

unittests/AST/ASTImporterTest.cpp

503

Broken indentation?

[tk1012]

lib/AST/ASTImporter.cpp

1634

According to include/clang/AST/Decl.h file, we cannot use "RecordDecl::isAnonymousStructOrUnion()" here because "struct { int a; } A;" is not an anonymous struct but is unnamed.

[tk1012]

Hallo Aleksei and Gábor,

Thank you for your response.

1. Are import conflicts for anonymous structures resolved correctly?

In fact, this patch only fixes the unnamed structures that are not anonymous.

In D39886#923188, @tk1012 wrote:

I added an inline comment

2. Are equal structures present in both TUs imported correctly, without duplication?

As far as my experience, ASTImporter cannot import without duplication when the same structure definition exists in the both TUs ( e.g. include the same header file).
Then, in some cases ( e.g. using ICmpExpr for the imported structures), LLVM asserts and fails in the compilation.

First, I think this situation is not considered in the usage of ASTImporter ( I mean ASTImporter assumes that one structure is defined only once).
But, is this not correct?

In any case, I will check both things and try to test them.

[tk1012]

Hello,

I update the diff to solve the below thing.

1. Are import conflicts for anonymous structures resolved correctly?

Before I describe the updates, I want to detail the difference between unnamed structs and anonymous structs in clang.
According to the comment of RecordDecl::isAnonymousStructOrUnion in include/AST/Decl.h,
there are unnamed structures that are not anonymous.
For example,

union { int i; float f; } obj;
struct { int a } A;

both of them are not anonymous but unnamed.

Originally, the anonymous structs/unions are checked by making sure that they occur in the same location.
However, the unnamed structures that are not anonymous are only checked by their name.
This results in the different unnamed structures are identified as the same one because their names are the null string.

So, this update makes the skip of the confliction check more strictly.
It also adds a test for importing the anonymous unions

Are equal structures present in both TUs imported correctly, without duplication?

I think this is a more complicated problem because this does not depend on the structures are unnamed or not.
For example, I try the below test and "make check-clang" fails due to "the Multiple declarations were found!".

TEST(ImportDecl, ImportSameRecordDecl) {
  MatchVerifier<Decl> Verifier;
  EXPECT_TRUE(
        testImport(
          "struct A {"
          " int a;"
          "};",
          Lang_C,
          "struct A {"
          " int a;"
          "};",
          Lang_C, Verifier,
          recordDecl()));
}

struct A is not anonymous and also not unnamed.

To avoid this, in my application, I check the same structure already exists in the "To" ASTContext before the import.
If the same struct is found, I map the "From" RecordDecl to the "To" one in advance.
However, I'm not sure I should request the review of this solution. ( I mean, should ASTImporter support this ?)
Even if I should, I think this feature must be in a different patch.

Best regards

[tk1012]

Fix the broken indentation.

[tk1012]

lib/AST/ASTImporter.cpp

1660

Anonymous structs/unions are checked here.

[a.sidorin]

Hello Takafumi,

I think you are correct. So, these are not unnamed structures that need conflict resolution but declarations that instantiate the type. Therefore, we can omit both lookup and conflict resolution part for unnamed structures.

Thank you! I'm accepting the patch but I'd like to hear the opinion of other reviewer if any.

[a.sidorin]

Hello Takafumi,

Could you investigate the patch D30876? It should fix same issue. However, it also handles conflict resolution. I accidentally forgot about this patch.

[tk1012]

Oh, yes.

D30876 is motivated by the same problem.

The notable difference between this patch and D30876 is that ASTImporter should check the conflict resolution for the unnamed structs/unions in a record context or not.
Then, I think D30876's solution is more conservative.
In fact, simply omitting the conflict check for all unnamed structures works well in my experience.
I have not hit the case where two unnamed structs have the same Index.
However, according to two past patches (1cef459 and 11ce5fe ), he clearly added the anonymous structs/unions to the checking path.
So, I guess there is an example where ASTImporter has to handle them rather than simply skips them.
D30876 more reflects the policy of these patches.

Furthermore, I also agree with the fix of findUntaggedStructOrUnionIndex(), although findUntaggedStructOrUnionIndex() is not in ASTImporter.cpp anymore.
The function exists in ASTStructuralEquivalence.cpp instead.

Sorry for this redundant patch.

[a.sidorin]

No need to apologize. Thank you for your work anyway. Even if the bug-fixing part of this patch will be omitted, I'd still like to add your tests into the test suite.

[tk1012]

Hello Aleksei,

Unfortunately, I find the related problem with the unnamed structs/unions, even if I apply D30876.

For example, in PostgreSQL, there is a part of code like below.

typedef struct { int  a; } b;

struct { const char *x; } y;

then, original AST becomse like below.

-RecordDecl 0x7ac3900 ... struct definition
| `-FieldDecl 0x7b18690 ... a 'int'
|-TypedefDecl 0x7b18730 ... b 'struct b':'b'
| `-ElaboratedType 0x7b186e0 'struct b' sugar
|   `-RecordType 0x7ac3990 'b'
|     `-Record 0x7ac3900 ''
|-RecordDecl 0x7b187a0 ... struct definition
| `-FieldDecl 0x7b18868 ... x 'const char *'
`-VarDecl 0x7b18900 ... y 'struct (anonymous struct at ...)':'struct (anonymous at ...)'

However, ASTImporter imports the AST incorrectly.
The result AST becomes

|-RecordDecl 0x7f696c07ee50 ... struct definition
| `-FieldDecl 0x7f696c177470 ... a 'int'
|-TypedefDecl 0x7f696c177510 ...  b 'struct b':'b'
| `-ElaboratedType 0x7f696c1774c0 'struct b' sugar
|   `-RecordType 0x7f696c07eee0 'b'
|     `-Record 0x7f696c07ee50 ''
|-RecordDecl 0x7f696c177568 prev 0x7f696c07ee50 ...  struct
`-VarDecl 0x7f696c177660 ...  y 'struct b':'b'

The variable y's type becomes struct b mistakenly.
This is because FoundDecl is set into PrevDecl at L1676.
In this case, FoundDecl is struct { int a; }.
Then, ASTImporter set PrevDecl as a previous RecordDecl of the imported RecordDecl at L1772.

To avoid this, I think there are two possible solutions.

1. Like this patch, skipping conflict resolution part for the unnamed structs/unions.
2. Add a condition for setting the previous decl at L1676.

What do you think?
( I guess the first is unexpectedly dependable. )

p.s.
Should I also share this in D30876?

lib/AST/ASTImporter.cpp

1671

highlight

1767

highlight

lib/AST/ASTImporter.cpp

if (!SearchName && D->getTypedefNameForAnonDecl()) {
     SearchName = Importer.Import(D->getTypedefNameForAnonDecl()->getDeclName());
     IDNS = Decl::IDNS_Ordinary;
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
     IDNS |= Decl::IDNS_Ordinary;
 
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *AdoptDecl = nullptr;
   RecordDecl *PrevDecl = nullptr;
-  if (!DC->isFunctionOrMethod()) {
+  if (!DC->isFunctionOrMethod() && SearchName.getAsString() != "") {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     SmallVector<NamedDecl *, 2> FoundDecls;
     DC->getRedeclContext()->localUncachedLookup(SearchName, FoundDecls);
 
     if (!FoundDecls.empty()) {
       // We're going to have to compare D against potentially conflicting Decls, so complete it.
       if (D->hasExternalLexicalStorage() && !D->isCompleteDefinition())
         D->getASTContext().getExternalSource()->CompleteType(D);
for (unsigned I = 0, N = FoundDecls.size(); I != N; ++I) {
           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
           // 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;
             }
           }
         }
 
         PrevDecl = FoundRecord;
 
         if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
           if ((SearchName && !D->isCompleteDefinition())
               || (D->isCompleteDefinition() &&
                   D->isAnonymousStructOrUnion()
                     == FoundDef->isAnonymousStructOrUnion() &&
                   IsStructuralMatch(D, FoundDef))) {
             // The record types structurally match, or the "from" translation
if (!D2) {
     
     D2->setQualifierInfo(Importer.Import(D->getQualifierLoc()));
     D2->setLexicalDeclContext(LexicalDC);
     LexicalDC->addDeclInternal(D2);
     if (D->isAnonymousStructOrUnion())
       D2->setAnonymousStructOrUnion(true);
     if (PrevDecl) {
       // FIXME: do this for all Redeclarables, not just RecordDecls.
       D2->setPreviousDecl(PrevDecl);
     }
   }
   
   Importer.Imported(D, D2);
 
   if (D->isCompleteDefinition() && ImportDefinition(D, D2, IDK_Default))
     return nullptr;
 

unittests/AST/ASTImporterTest.cpp

EXPECT_TRUE(testImport("void declToImport() { typedef _Atomic(int) a_int; }",
                              compoundStmt(
                                has(
                                  declStmt(
                                    has(
                                      typedefDecl(
                                        has(atomicType()))))))))));
 }
 
+TEST(ImportDecl, ImportUnnamedRecordDecl) {
+  MatchVerifier<Decl> Verifier;
+  EXPECT_TRUE(
+        testImport(
+          "void declToImport() {"
+          "  struct Root {"
+          "    struct { int a; } A;"
+          "    struct { float b; } B;"
+          "  } root;"
+          "}",
+          Lang_C, "", Lang_C, Verifier,
+          functionDecl(
+            hasBody(
+              compoundStmt(
+                has(
+                declStmt(
+                  has(
+                    recordDecl(
+                      has(
+                        recordDecl(
+                          has(
+                            fieldDecl(
+                              hasType(asString("int")))))),
+                      has(
+                        recordDecl(
+                          has(
+                            fieldDecl(
+                              hasType(asString("float"))))))
+                      )))))))));
+}
 
 } // end namespace ast_matchers
 } // end namespace clang