Page MenuHomePhabricator

Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton
ClosedPublic

Authored by shafik on Dec 11 2019, 3:50 PM.

Details

Summary

This fix was motivated by a crashes in expression parsing during code generation in which we had a RecordDecl that had incomplete FieldDecl. During code generation when computing the layout for the RecordDecl we crash because we have several incomplete FieldDecl.

This fixes the issue by assuring that during ImportDefinition(...) for a RecordDecl we also import the definitions for each FieldDecl.

Diff Detail

Event Timeline

shafik created this revision.Dec 11 2019, 3:50 PM

I wonder if we have a way to fix this from with LLDB. Having Clang code that is only tested in LLDB is always a bit weird.

Otherwise the idea itself LGTM, thanks for working on this (and reducing the test case to that!)

clang/lib/AST/ASTImporter.cpp
1739

*completing

lldb/packages/Python/lldbsuite/test/commands/expression/codegen-crash-typedefdecl-not-in_declcontext/main.cpp
28

You might want to explicitly call clang-format on this example as there are few indentation errors (this line, the f() declaration, line 20, etc.). We disabled clang-format for tests so that may require some workarounds (temporarily copying out of tree is the easiest probably)

Thanks for the patch! It look almost good to me, but I have a comment about the error handling.

clang/lib/AST/ASTImporter.cpp
1758

Rather than just simply returning with the Error, I think we should append this error to ChildErrors and go on with the next field, as this is what the original code did.
I am thinking about something like this:

if (Err && AccumulateChildErrors)
  ChildErrors =  joinErrors(std::move(ChildErrors), Err);
else
  consumeError(Err);

And perhaps line 1713 could be just a simple else {

Just one more thing, maybe that is too overkill, but I think on a long term we could benefit from a unittest for this case. You could create a test similar to LLDBLookupTest in ASTImporterTest.cpp. In that Fixture we use Minimal import and the regular lookup (that is the exact case in LLDB).
In the test itself we could call ImportDefinition on a struct that has fields with record types. And then we could assert that all field's type's have complete types.

I wonder if we have a way to fix this from with LLDB. Having Clang code that is only tested in LLDB is always a bit weird.

Otherwise the idea itself LGTM, thanks for working on this (and reducing the test case to that!)

I spent a while trying to come up w/ a way to reproduce this in clang-import-test but perhaps I can get something working using LLDBLookupTest like @martong suggested.

Another bug report for this: https://bugs.llvm.org/show_bug.cgi?id=44331 Please close when landing this.

shafik updated this revision to Diff 234619.Dec 18 2019, 2:04 PM
shafik marked 3 inline comments as done.
  • Fix typo
  • Adjust error handing
  • clang-format test
martong accepted this revision.Dec 19 2019, 2:25 AM
This revision is now accepted and ready to land.Dec 19 2019, 2:25 AM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 19 2019, 11:22 AM