[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. Jenkins is green: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31743/ The previous build caught up the change, but http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31742/ was stopped, perhaps a test got stucked. Repository: rL LLVM CHANGES

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

This revision was automatically updated to reflect the committed changes. Closed by commit rL366997: [ASTImporter] Reorder fields after structure import is finished (authored by martong, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

shafik accepted this revision. shafik added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100 ___ cfe-commits mailing list

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:5244 +} + + a_sidorin wrote: > A redundant newline? Yes, I deleted that. And moved the test case closer the the last `ImportDecl` test case. Repository: rG LLVM Github

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong updated this revision to Diff 211314. martong marked 5 inline comments as done. martong added a comment. - Address Alexei's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100 Files:

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Gabor, Thank you again for working on this patch. I think it can be committed after minor stylish issues are fixed. Comment at: clang/lib/AST/ASTImporter.cpp:1677

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. @a_sidorin, @shafik This is a polite Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100 ___ cfe-commits mailing list

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong updated this revision to Diff 210861. martong added a comment. - Further simplify by removing the last for loop Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100 Files:

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong updated this revision to Diff 210813. martong added a comment. - Rebase to master - Some refactor is done mostly because since D63603 ([ASTImporter] Propagate error from ImportDeclContext) we may not imported successfully all decls of a DC. - Made the

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added inline comments. Comment at: lib/AST/ASTImporter.cpp:1643 +if (!ImportedOrErr) { + // For RecordDecls, failed import of a field will break the layout of the + // structure. Handle it as an error. shafik wrote: > What cases are failed

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong updated this revision to Diff 199629. martong marked 13 inline comments as done. martong added a comment. - Address Shafik's comments and update assertions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

shafik added a comment. Done with first round. Comment at: lib/AST/ASTImporter.cpp:1643 +if (!ImportedOrErr) { + // For RecordDecls, failed import of a field will break the layout of the + // structure. Handle it as an error. What cases are

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. Thank you for digging into this! Feel free to ask me if you encounter any problems with the patch. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong commandeered this revision. martong edited reviewers, added: a_sidorin; removed: martong. martong added a comment. This revision now requires review to proceed. Herald added subscribers: gamesh411, dkrupp. @a_sidorin Aleksei, If you don't mind, I am going to investigate this further with

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong updated this revision to Diff 189830. martong edited the summary of this revision. martong added a comment. Herald added a project: clang. Rebase to master. There was a conflict in the tests, in ASTImporterTest.cpp. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

aprantl added inline comments. Comment at: lib/AST/ASTImporter.cpp:1672 // FIXME: Handle this case somehow better. - consumeError(ImportedOrErr.takeError()); + else +consumeError(ImportedOrErr.takeError()); this else is redundant.

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. 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(FromDC)` gives us a different value than

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. 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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. 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). Repository: rC Clang CHANGES SINCE

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. There is another failure on macOS, which is not there on Linux. This is present with the 174545 patch id (even before applying my fix for TestCModules). $ ninja check-clang-astmerge Testing Time: 0.63s Failing Tests

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. @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 +++

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. Hi Gabor, Thank you for the investigation! I'll try to take a look. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100 ___ cfe-commits mailing list

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. 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 ... }

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D44100/new/ https://reviews.llvm.org/D44100

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. 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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. 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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

aprantl added a comment. In D44100#1302478 , @a_sidorin wrote: > 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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. 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. Repository: rC Clang

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

davide added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D44100

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin updated this revision to Diff 174545. a_sidorin added a comment. Hi @davide and @shafik, Could you please check the updated version of the patch? Repository: rC Clang https://reviews.llvm.org/D44100 Files: lib/AST/ASTImporter.cpp unittests/AST/ASTImporterTest.cpp Index:

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

shafik added a comment. I am happy to discuss and review a revised version of this change. Repository: rC Clang https://reviews.llvm.org/D44100 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

davide added a comment. In https://reviews.llvm.org/D44100#1282157, @a.sidorin wrote: > 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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

davide added a comment. No worries :) l In https://reviews.llvm.org/D44100#1282148, @martong wrote: > 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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a.sidorin added a comment. 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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. 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:

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. Oops. Thank you Davide! Repository: rC Clang https://reviews.llvm.org/D44100 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

davide added subscribers: shafik, davide. davide added a comment. 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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

This revision was automatically updated to reflect the committed changes. Closed by commit rC345545: [ASTImporter] Reorder fields after structure import is finished (authored by a.sidorin, committed by ). Changed prior to commit: https://reviews.llvm.org/D44100?vs=160477=171578#toc

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D44100 ___ cfe-commits

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D44100

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong accepted this revision. martong added inline comments. This revision is now accepted and ready to land. Comment at: lib/AST/ASTImporter.cpp:1317 + for (auto *D : FromRD->decls()) { +Decl *ToD = Importer.GetAlreadyImportedOrNull(D); +assert(ToRD ==

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin marked an inline comment as done. a_sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:1317 + for (auto *D : FromRD->decls()) { +Decl *ToD = Importer.GetAlreadyImportedOrNull(D); +assert(ToRD == ToD->getDeclContext() &&

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added inline comments. Comment at: lib/AST/ASTImporter.cpp:1317 + for (auto *D : FromRD->decls()) { +Decl *ToD = Importer.GetAlreadyImportedOrNull(D); +assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD)); Is it sure that `ToD` will

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin marked 2 inline comments as done. a_sidorin added inline comments. Comment at: lib/AST/ASTImporter.cpp:1029 + + RecordDecl *ToRD = cast(Importer.Import(cast(FromDC))); + martong wrote: > Can't we just import the `FromRD`, why we need that cast at the

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a_sidorin updated this revision to Diff 160477. a_sidorin edited the summary of this revision. a_sidorin added a comment. All declarations are reordered now, not only fields. Also some review comments were addressed. Repository: rC Clang https://reviews.llvm.org/D44100 Files:

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. Ping Repository: rC Clang https://reviews.llvm.org/D44100 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. Hi Aleksei, I am OK with this, I just have a little concern about friend declarations. Since https://reviews.llvm.org/D32947 ( [ASTImporter] FriendDecl importing improvements ) records' structural equivalency depends on the order of their friend declarations. Anyway,

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a.sidorin added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D44100 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

martong added a comment. 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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a.sidorin added a comment. 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.

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

xazax.hun added a comment. 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.

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

a.sidorin created this revision. a.sidorin added reviewers: xazax.hun, szepet, jingham. Herald added subscribers: martong, rnkovacs. - 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