Page MenuHomePhabricator

[clang][AST][ASTImporter] Set record to complete during import of its members.
ClosedPublic

Authored by balazske on Dec 22 2021, 3:19 AM.

Details

Summary

At import of a member it may require that the record is already set to complete.
(For example 'computeDependence' at create of some Expr nodes.)
The record at this time may not be completely imported, the result of layout
calculations can be incorrect, but at least no crash occurs this way.

A good solution would be if fields of every encountered record are imported
before other members of all records. This is much more difficult to implement.

Diff Detail

Event Timeline

balazske created this revision.Dec 22 2021, 3:19 AM
balazske requested review of this revision.Dec 22 2021, 3:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2021, 3:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
shafik added inline comments.Jan 4 2022, 10:57 AM
clang/lib/AST/ASTImporter.cpp
2021

So DefinitionCompleterScopeExit will run To->setCompleteDefinition(false); after this function exits but this will be in effect during the import the base classes. I don't see how the tests you added hit that code.

clang/unittests/AST/ASTImporterTest.cpp
7486

The member function body should be considered complete-class context so the correct thing to do would be have all the fields laid out by this point.

balazske added inline comments.Jan 5 2022, 2:23 AM
clang/lib/AST/ASTImporter.cpp
2021

Should the test CompleteRecordBeforeImporting not do the import in minimal mode? The comment says minimal but in ASTImporter minimal mode is not set. The test will fail because this added line. But I think it should work to call the To->setCompleteDefinition here only in non-minimal mode.

2089

My fix was not correct. This line was added to make a test CompleteRecordBeforeImporting not fail but it makes the original fix not work. Something other is needed.

clang/unittests/AST/ASTImporterTest.cpp
7486

My initial plan was to import first all fields, then everything else. But it is possible that a field has reference to the same record before the import of all fields finishes (like in the second test) so this can not work in all cases (and it caused other test failures).

balazske added inline comments.Jan 5 2022, 2:25 AM
clang/lib/AST/ASTImporter.cpp
2021

And remove the later added line 2088 To->setCompleteDefinition(false);.

martong added inline comments.Jan 6 2022, 4:10 AM
clang/lib/AST/ASTImporter.cpp
2021

Should the test CompleteRecordBeforeImporting not do the import in minimal mode?

Yes, that should, however, I can confirm it probably does not (see the false arg below):

Importer.reset(Creator(ToAST->getASTContext(), ToAST->getFileManager(),
                       Unit->getASTContext(), Unit->getFileManager(), false,
                       SharedState));

So, first we should fix the test case CompleteRecordBeforeImporting to set up the ASTImporter in minimal mode.

Then, we should call the To->setCompleteDefinition here only in non-minimal mode as you suggests. Once again, an ugly branching because of the "minimal" mode, we should really get rid of that (and hope that Raphael patch evolves in D101950).

Ping.
I want to see opinion of @shafik (or others) about change of test CompleteRecordBeforeImporting (turn on minimal import mode in this test).

Adding Raphael @teemperor , he might have useful comments about the minimal mode.

I'm really sorry @martong , but I no longer work on LLDB (or the ASTImporter) and I'm not really in the loop regarding LLDB development.

(Having said that, I still happy to answer questions about my own patches/reviews that I did in the past in case there are questions about that)

@martong Ping
We do not have possibility currently to check all LLDB tests. I think we can commit this and observe the buildbots for failure.

(The current code does not work, the mentioned fixes must be applied first.)

@martong Ping
We do not have possibility currently to check all LLDB tests. I think we can commit this and observe the buildbots for failure.

I agree. Let's see if it works out. I don't think we shall wait for longer.
This should be already better than crashing time-to-time.
We can always revert if necessary.

Well, I've just recognized that the "Build Status" of this latest diff shows that it crashes.

balazske updated this revision to Diff 407822.Feb 11 2022, 3:00 AM

Fix of test failures.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 13 2022, 11:28 PM
This revision was automatically updated to reflect the committed changes.