Page MenuHomePhabricator

[ASTImporter] Reorder fields after structure import is finished
AcceptedPublic

Authored by a_sidorin on Mar 5 2018, 10:07 AM.

Details

Summary

There are multiple reasons why field structures can be imported in wrong way. The simplest is the ability of field initializers and method bodies to refer fields not in order they are listed in. Unfortunately, there is no clean solution for that currently so I'm leaving a FIXME.

Diff Detail

Repository
rC Clang

Event Timeline

a.sidorin created this revision.Mar 5 2018, 10:07 AM

Hi Aleksei!

Just a minor high level note. If https://reviews.llvm.org/D32947 will be accepted once, we will need to reorder friends as well. Or alternatively, we have to omit the check of friends in structural equivalence in https://reviews.llvm.org/D32947.

Hi Gabor,

Just a minor high level note. If https://reviews.llvm.org/D32947 will be accepted once, we will need to reorder friends as well. Or alternatively, we have to omit the check of friends in structural equivalence in https://reviews.llvm.org/D32947.

I'd prefer to land D32947 before the fix for friend decls reordering so we will have a subject for testing. And the fix for friend decls can become a separate patch. Do you already have such patch or a test for the issue?

Aleksei,

We should definitely try to synchronize our work between Samsung (?) and Ericsson much much more.
Unfortunately, it is often we work on the same issue and this can cause delays for both of us.
Actually, we solved the same issue in our branch a year ago, see
https://github.com/Ericsson/clang/blob/ctu-clang5/lib/AST/ASTImporter.cpp#L1087 and the related test:
https://github.com/Ericsson/clang/blob/ctu-clang5/unittests/AST/ASTImporterTest.cpp#L1365

Gabor

Hi Aleksei,

I am OK with this, I just have a little concern about friend declarations. Since D32947 ( [ASTImporter] FriendDecl importing improvements ) records' structural equivalency depends on the order of their friend declarations.
Anyway, I think friend related issues can be addressed in a separate patch.

What is a bit more concerning is that *ToField can be null and we should handle that.
This could happen if lookup finds the type of the field, but for some reason (e.g. there is an error in structural eq, or the redecl chain is incorrect) it turns out to be non-equivalent with the type of the FromField:

for (auto *FoundDecl : FoundDecls) {
  if (auto *FoundField = dyn_cast<FieldDecl>(FoundDecl)) {
    // For anonymous fields, match up by index.
    if (!Name && getFieldIndex(D) != getFieldIndex(FoundField))
      continue;

    if (Importer.IsStructurallyEquivalent(D->getType(),
                                          FoundField->getType())) {
      Importer.Imported(D, FoundField);
      return FoundField;
    }

    Importer.ToDiag(Loc, diag::err_odr_field_type_inconsistent)
      << Name << D->getType() << FoundField->getType();
    Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
      << FoundField->getType();
    return nullptr;
  }
}
lib/AST/ASTImporter.cpp
1025

Could use auto here, to avoid redundant type specification.

1029

Can't we just import the FromRD, why we need that cast at the end?
auto *ToRD = cast<RecordDecl>(Importer.Import(FromRD)));

1032

I think ToField can be a nullptr, and if so, then ToField->getDeclContext() is UB.
Same could happen at line 1040.
Perhaps we should have and explicit check about the nullptr value:
if (!ToField) continue;

unittests/AST/ASTImporterTest.cpp
1022

The hasFieldOrder matcher is already existing.

1034

We already have this test, we just have to enable it.
DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder

a_sidorin commandeered this revision.Aug 13 2018, 3:55 PM
a_sidorin added a reviewer: a.sidorin.
a_sidorin updated this revision to Diff 160477.Aug 13 2018, 3:57 PM
a_sidorin edited the summary of this revision. (Show Details)

All declarations are reordered now, not only fields. Also some review comments were addressed.

a_sidorin marked 2 inline comments as done.Aug 13 2018, 3:59 PM
a_sidorin added inline comments.
lib/AST/ASTImporter.cpp
1029

We need to cast it to DeclContext anyway, so I don't think that a narrower type will be worse

1032

I have added a check for the return result into the import loop. So, after the import is finished, all nested decls should be non-null.

martong added inline comments.Aug 14 2018, 4:54 AM
lib/AST/ASTImporter.cpp
1317

Is it sure that ToD will never be a nullptr?
I think, removeDecl or addDeclInternal below may crash if we call it with a nullptr.
Also in the assert, ToD->getDeclContext() seems achy if ToD is a nullptr.

a_sidorin marked an inline comment as done.Aug 15 2018, 3:51 PM
a_sidorin added inline comments.
lib/AST/ASTImporter.cpp
1317

We have an early return if such import failed before (line 1300).

martong accepted this revision.Aug 15 2018, 11:39 PM
martong added inline comments.
lib/AST/ASTImporter.cpp
1317

Okay, that line did not catch my attention. Could you please provide a comment which describes that at this point we expect ToD to be non-null, or even better, we could have an explicit assert(ToD) .

This revision is now accepted and ready to land.Aug 15 2018, 11:39 PM

Ping. This is still not committed, however, essential for ctu analyse any C++ project. Aleksei, I am happy to commit it for you if you don't have time, just let me know.

Hi Gabor,
I have tried to rebase it last week but there were some test failures. I hope to fix them this weekend and finally commit the patch.

This revision was automatically updated to reflect the committed changes.

This patch broke lldb, at least on MacOS.
You can reproduce the issue building lldb, applying your patch and then running

$ ./lldb-dotest -p TestCModules

I'm going to revert this to unblock folks, but don't hesitate to ping me in case you need any help reproducing the issue.
cc: @shafik

Oops. Thank you Davide!

Hi Davide,

I'd like to draw your attention to an important relation of this patch to a bug in LLDB which manifests as incorrect vtable layouts for LLDB expression code.
Please see the conversation started with this message: https://lists.llvm.org/pipermail/cfe-dev/2018-August/058842.html
I think that the solution to that problem would be exactly this patch, i.e. to put the methods in the proper order during import.

Are the failing lldb tests related to ObjC code?

Hello everyone.
@martong : this version of patch reorders only fields and friends. No method reordering is done (I can check if it works, though, and add the feature).
@davide: Unfortunately, I was not able to reproduce the problem on Linux. Could you please provide a link to a buildbot failure or some environment description so I can reproduce the issue or, at least, take a look?

No worries :) l

Hi Davide,

I'd like to draw your attention to an important relation of this patch to a bug in LLDB which manifests as incorrect vtable layouts for LLDB expression code.
Please see the conversation started with this message: https://lists.llvm.org/pipermail/cfe-dev/2018-August/058842.html
I think that the solution to that problem would be exactly this patch, i.e. to put the methods in the proper order during import.

Are the failing lldb tests related to ObjC code?

Some of them are, see e.g. http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/11972/console
@shafik is the person who's driving better lldb C++ support on our side, so you might consider chatting with him about this.
I just got the bots in a greener state again.

Hello everyone.
@martong : this version of patch reorders only fields and friends. No method reordering is done (I can check if it works, though, and add the feature).
@davide: Unfortunately, I was not able to reproduce the problem on Linux. Could you please provide a link to a buildbot failure or some environment description so I can reproduce the issue or, at least, take a look?

http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/11972/console
It's unfortunate because this doesn't show a backtrace. I'm afraid at this point the easiest way to reproduce this is on MacOS directly.

I am happy to discuss and review a revised version of this change.

a_sidorin reopened this revision.Nov 18 2018, 2:34 PM
This revision is now accepted and ready to land.Nov 18 2018, 2:34 PM
a_sidorin updated this revision to Diff 174545.Nov 18 2018, 2:37 PM

Hi @davide and @shafik,
Could you please check the updated version of the patch?

Alexsei, I'm afraid I'm not qualified to review this patch. I would really recommend you to find somebody who's familiar with clang to review it, as it already seems to have broken lldb in the past.

Hi Davide,

I don't mean only review. As I guess, you guys have MacOS machines so you can check if the problem is still present in the updated version.
There is no need to remind me about the problem with LLDB since I tried to resolve it.

I don't mean only review. As I guess, you guys have MacOS machines so you can check if the problem is still present in the updated version.

FYI, typically problems with ASTImporter in LLDB are not macOS-specific at all and also reproduce when building LLDB for Linux. You can just check out LLDB into the llvm/tools directory and run ninja && ninja check-lldb

Hi Adrian,
I've already done check-all (this includes LLDB) with this patch. As you can see in this thread, I didn't manage to reproduce some issues on Linux. Luckily, one test still failed on Linux so I could find the problem. However , it will be good to ensure that this is the same problem that caused failures on MacOS and the commit will be safe. That's why I asked for an additional check.

Alexei,

I had a chance to have a quick look into this on a borrowed Mac Book. Seems like the assertion which was related to the revert is not existent anymore, but a new assertion came in. Next week I'll have time to debug that and I'll come back to you with what I find.

Hi Gabor,
Thank you for digging into this! Unfortunately, I don't have any MacOS device and I cannot check my code for the compatibility.

Hey Alexey,

Here is what I found so far:
The new assertion is this:

Assertion failed: (ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD)), function ImportDeclContext

ToRD looks like this in code:

typedef struct __sFILE {
   // several fields here ...
} FILE;

And the AST:

CXXRecordDecl 0x7f87ce97d488 </Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/_stdio.h:126:9, col:16> col:16 struct __sFILE definition
|-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
| |-DefaultConstructor exists trivial needs_implicit
| |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| |-MoveConstructor exists simple trivial needs_implicit
| |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| |-MoveAssignment exists simple trivial needs_implicit
| `-Destructor simple irrelevant trivial needs_implicit
|-CXXRecordDecl 0x7f87d1886000 <col:9, col:16> col:16 implicit struct __sFILE
| `-<undeserialized declarations>
`-<undeserialized declarations>

I dug deeper and this is because ToRD is not equal with ToD->getDeclContext().
However, if I load the fields of ToRD and then dump it seems to be structurally equivalent with ToD->getDeclContext().
Both ToRD and ToD->getDeclContext() are part of a Decl chain with one element, this is true for FromRD and D->getDeclContext(). So I suspect some sort of lookup problem here: first we import the declContext during the import of a field, then when we do auto ImportedDC = import(cast<Decl>(FromDC)); where we fail to find the first imported DeclContext. I suspect that this is because typedef hides the real struct, I mean __sFILE seems to be not find-able by normal C lookup (ie. it is hidden).

Here is one workaround (I did not try it yet):
We could lazy initialize ToRD inside the for loop to be equal to the first 'ToD`'s decl context.

RecordDecl *ToRD = nullptr;
for (auto *D : FromRD->decls()) {
  if (isa<FieldDecl>(D) || isa<FriendDecl>(D)) {
    Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
    if (!ToRD)
        ToRD = ToD->getDeclContext();
    ToRD->removeDecl(ToD);
  }
}

I am afraid I can continue with the debug only next week.

Hi Gabor,
Thank you for the investigation! I'll try to take a look.

@a_sidorin

The below diff on top of your patch successfully handles the failure with the TestCModules.py LLDB testcase:

diff --git a/lib/AST/ASTImporter.cpp b/lib/AST/ASTImporter.cpp
index 05fec7f943..e6fb590025 100644
--- a/lib/AST/ASTImporter.cpp
+++ b/lib/AST/ASTImporter.cpp
@@ -1695,15 +1695,22 @@ ASTNodeImporter::ImportDeclContext(DeclContext *FromDC, bool ForceImport) {
   // LoadFieldsFromExternalStorage().
   auto ImportedDC = import(cast<Decl>(FromDC));
   assert(ImportedDC);
-  auto *ToRD = cast<RecordDecl>(*ImportedDC);
+  RecordDecl *ToRD = nullptr;
   for (auto *D : FromRD->decls()) {
     if (isa<FieldDecl>(D) || isa<FriendDecl>(D)) {
       Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
-      assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
-      ToRD->removeDecl(ToD);
+      if (!ToRD)
+        ToRD = cast<RecordDecl>(ToD->getDeclContext());
+      else
+        assert(ToRD == ToD->getDeclContext());
+      if(ToRD->containsDecl(ToD))
+        ToRD->removeDecl(ToD);
     }
   }

+  if (!ToRD)
+    return Error::success();
+
   if (!ToRD->hasExternalLexicalStorage())
     assert(ToRD->field_empty());
This comment was removed by martong.

Alexey, good news! I have finished the testing of the above patch applied on top of 174545. Neither is a regression on Linux nor on macOS. (ASTTests, check-clang-astmerge, check-clang-import, check-clang-analysis, check-lldb).

Hi Gabor, thank you a lot!
I have to say that even having your patch, I still don't understand why the patch fixes the problem (and what the problem is). As I see, the patch handles the cases where some fields or friends have DeclContext different from ToRD. Could you please provide examples so I can add them to the test suite or, at least, just investigate?

Alexey, simply put, the crux of the problem is this:
The imported decl context is different than what we get by calling the getDeclContext() on an imported field/friend of the "from" decl context.

With other words:
import(cast<Decl>(FromDC) gives us a different value than what we get from ToD->getDeclContext() inside the for loop which iterates over the fields/friends of FromDC.

This seems abnormal, because we would expect the two to be equal. This is why I suspect something related to an LLDB specific use case (and what's worse it happens only on macOS). In LLDB, I have seen they first do a minimal import then they require the definition with ASTImporter::importDefinition (but I am not sure if this is the real root cause here). I guess this is a scenario we don't have any unittest neither lit tests for in Clang. In an ideal world we should fix this inequivalence first and then could we apply this patch.

The workaround I provided is to use the decl context of the first imported field and use that as target decl context (and not use the decl context given by import(FromDC)).

Hope this helps

aprantl added inline comments.Mon, Dec 17, 8:34 AM
lib/AST/ASTImporter.cpp
1672

this else is redundant.

1696

Can you add a comment explaining why this function named ...OrNull never returns a nullptr?

1703

assert(ToRD->hasExternalLexicalStorage() || ToRD->field_empty());

1708

This is redundant with the next assertion.

1712

ditto