This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Fix import of objects with anonymous types
ClosedPublic

Authored by martong on Jun 29 2018, 8:30 AM.

Details

Summary

Currently, anonymous types are merged into the same redecl chain even if they
are structurally inequivalent. This results that global objects are not
imported, if there are at least two global objects with different anonymous
types. This patch provides a fix.

Diff Detail

Repository
rC Clang

Event Timeline

martong created this revision.Jun 29 2018, 8:30 AM

Hello Gabor,

The change looks reasonable to me. But could you please check if we can have some code unification with structural matching?

lib/AST/ASTImporter.cpp
2075

Is it possible to use the added code for the entire condition if (auto *FoundRecord = dyn_cast<RecordDecl>(Found)), replacing its body? Our structural matching already knows how to handle unnamed structures, and the upper code partially duplicates IsStructurallyEquivalent(StructuralEquivalenceContext &Context,RecordDecl *D1, RecordDecl *D2). Can we change structural matching to handle this stuff instead of doing it in ASTNodeImporter?

martong updated this revision to Diff 153917.Jul 3 2018, 7:55 AM
martong marked an inline comment as done.

Remove redundant code and use only StructurlaEquivalence

martong added inline comments.Jul 3 2018, 7:55 AM
lib/AST/ASTImporter.cpp
2075

Yes, this is a good point, updated the patch accordingly.

balazske added inline comments.Jul 3 2018, 8:32 AM
lib/AST/ASTImporter.cpp
2075

My idea was to remove this whole if (!SearchName) branch, but some restructuring of the next conditions may be needed. Setting of PrevDecl in the if branch does not look correct (if !SearchName is false it is never set, unlike in the old code).

martong marked an inline comment as done.Jul 4 2018, 2:25 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
2075

I tried what you mentioned Balazs.
It turned out, we can't just skip the whole if (!SearchName) branch. Because in the below case we would match the first unnamed union with the second, therefore we will break functionality (and some lit tests). The continue at line 2078 is very important in this edge case. (I agree that this code is very messy and would be good if we could refactor that to be cleaner, particularly if we could remove the SearchName variable would make it way better.)

// Matches
struct Unnamed {
  union {
    struct {
      int i;
    } S;
    struct {
      float i;
    } R;
  } U;
} x14;

The failing lit test and it's output:

/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:84:5: warning: type 'struct Unnamed::(anonymous at /home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:84:5)' has incompatible definitions in different translation units
    struct {
    ^
/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:85:11: note: field 'i' has type 'int' here
      int i;
          ^
/home/egbomrt/WORK/llvm3/git/llvm/tools/clang/test/ASTMerge/struct/Inputs/struct1.c:88:13: note: field 'i' has type 'float' here
      float i;
            ^
martong marked an inline comment as done.Jul 4 2018, 2:27 AM
a_sidorin accepted this revision.Jul 4 2018, 1:52 PM

Nice!

This revision is now accepted and ready to land.Jul 4 2018, 1:52 PM
martong updated this revision to Diff 154187.Jul 5 2018, 1:36 AM

PrevDecl is set after the !SearchName branch, as it had been done before.
This way original behaviour is kept as much as possible.

balazske accepted this revision.Jul 5 2018, 1:40 AM
This revision was automatically updated to reflect the committed changes.