This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Fields are imported first and reordered for correct layout.
ClosedPublic

Authored by danix800 on Jul 8 2023, 4:26 AM.

Details

Summary

Fields are imported first and reordered for correct layout.

For partially imported record, layout computation is incorrect.

Diff Detail

Event Timeline

danix800 created this revision.Jul 8 2023, 4:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2023, 4:26 AM
Herald added a subscriber: martong. · View Herald Transcript
danix800 requested review of this revision.Jul 8 2023, 4:26 AM
danix800 updated this revision to Diff 538500.Jul 9 2023, 8:21 PM

Apply git-clang-format.

danix800 added a reviewer: aprantl.
danix800 added a reviewer: balazske.
danix800 retitled this revision from [clang] ASTImport: force recomputing ASTRecordLayout when importing to [clang] [ASTImporter]: force recomputing ASTRecordLayout when importing.Jul 10 2023, 8:09 AM
danix800 planned changes to this revision.Jul 13 2023, 7:01 AM
danix800 updated this revision to Diff 540254.Jul 13 2023, 8:57 PM
danix800 retitled this revision from [clang] [ASTImporter]: force recomputing ASTRecordLayout when importing to [ASTImporter] Fields are imported first and reordered for correct layout..
danix800 edited the summary of this revision. (Show Details)
danix800 set the repository for this revision to rG LLVM Github Monorepo.

Fields are imported first and reordered for correct layout.

If I see correctly this change does that the re-ordered members are imported before all other, specially fields come before functions. This way order of fields is already correct when a function is imported.

clang/lib/AST/ASTImporter.cpp
1863

Braces are not required here (and at the other similar places).

If I see correctly this change does that the re-ordered members are imported before all other, specially fields come before functions. This way order of fields is already correct when a function is imported.

That's correct.

danix800 updated this revision to Diff 541037.Jul 17 2023, 7:57 AM

Remove unnecessary braces.

balazske accepted this revision.Jul 17 2023, 8:02 AM
This revision is now accepted and ready to land.Jul 17 2023, 8:02 AM
shafik added inline comments.Jul 27 2023, 5:35 PM
clang/unittests/AST/ASTImporterTest.cpp
8018

So is it the case that this caused f2 to be imported first and then f1? Which is the opposite of the example in the comments:

struct declToImport {
         int a = c + b;
         int b = 1;
         int c = 2;
     };

which causes the order to be c, b and then a.

danix800 added inline comments.Jul 27 2023, 9:23 PM
clang/unittests/AST/ASTImporterTest.cpp
8018

The issue caused by this case

int m() {
   return &((A *)0)->f1 - &((A *)0)->f2;
}
int f1;
int f2;

is that before fixing, method m is imported first, which triggers expr (UnaryOperator) dependency computation, further relies on the correct RecordLayout of declToImport, but f1 & f2 is not imported at this moment.

This is not the opposite to the case like int a = c + b;, but both are caused by circular refs.

Similar record layout issue will also be triggered like:

class B;
class A {
  B* b;
  int c;
};
class B {
   A *f() { return &((B *)0)->a; }
   A a;
};

https://reviews.llvm.org/D156201 removes the unnecessary record layout as another fix to this kind of circular refs
(which cannot be fixed simply by re-adjusting import ordering).

danix800 added inline comments.Jul 27 2023, 9:35 PM
clang/unittests/AST/ASTImporterTest.cpp
8018

Sorry for typos, they are both import ordering issues (not circular ref issues).