Page MenuHomePhabricator

[ASTImporter] Fix infinite recursion on function import with struct definition in parameters
ClosedPublic

Authored by gerazo on Jun 8 2018, 8:12 AM.

Details

Summary

Importing a function having a struct definition in the parameter list causes a crash in the importer via infinite recursion. This patch avoids the crash and reports such functions as not supported. Unit tests make sure that normal struct definitions inside function bodies work normally on the other hand and LLDB-like type imports also do.

Diff Detail

Repository
rL LLVM

Event Timeline

gerazo created this revision.Jun 8 2018, 8:12 AM

Problem: This change interferes with https://reviews.llvm.org/D47445. Probably that should be committed, it is approved already.

Problem: This change interferes with https://reviews.llvm.org/D47445. Probably that should be committed, it is approved already.

Ok. I'll wait for the other thing to be committed and I will rework this immediately. Thanks for telling.

gerazo updated this revision to Diff 151533.Jun 15 2018, 10:26 AM

Updated to not conflict with and use the stuff implemented in https://reviews.llvm.org/D47445 (so became a bit smaller)
Now, it is ready for a review. Enjoy!

martong accepted this revision.Jun 19 2018, 3:16 AM

LGTM, just found some minor things.

unittests/AST/ASTImporterTest.cpp
174 ↗(On Diff #151533)

Now that the Importer is moved under the TU struct it is not that trivial so I think we should add a few more comments to it.
Something like:

Represents a "From" translation unit and holds an importer object which we use to import from this translation unit.

Also there is an invariant which should be forced: the "To" and the "From" context of the Importer must remain the same throughout the life time of any TU object. We can achieve that with an assert in lazyInitImporter.

(What's more, the different objects all should have the same "To" contexts, but that could be checked only in the TestBase and looks more difficult, so for this patch we could skip that. )

190 ↗(On Diff #151533)

I think, if there is an existing Importer then we should assert whether that has a ToASTwhich is equal to the parameter.

This revision is now accepted and ready to land.Jun 19 2018, 3:16 AM
gerazo updated this revision to Diff 152055.Jun 20 2018, 4:17 AM
gerazo marked 2 inline comments as done.

Added @martong 's suggestions.

gerazo added a comment.Jul 4 2018, 6:35 AM

@a.sidorin what do you think?

martong retitled this revision from [ASTmporter] Fix infinite recursion on function import with struct definition in parameters to [ASTImporter] Fix infinite recursion on function import with struct definition in parameters.Jul 10 2018, 3:26 AM

Ping.

a.sidorin accepted this revision.Jul 11 2018, 3:18 AM

Hello Zoltán,

Sorry for the delay. I think the patch is fine, just some minor nits inline.

unittests/AST/ASTImporterTest.cpp
234 ↗(On Diff #152055)

Can we move the side effect into import()?

1049 ↗(On Diff #152055)

EXPECT_FALSE?

gerazo marked 2 inline comments as done.Jul 11 2018, 7:06 AM
gerazo added inline comments.
unittests/AST/ASTImporterTest.cpp
234 ↗(On Diff #152055)

Sure, thank you.

gerazo updated this revision to Diff 154991.Jul 11 2018, 7:08 AM
gerazo marked an inline comment as done.

Minor fixes for Aleksei's comments.

@gerazo, Do you have commit rights, or should I help with the commit?

@martong I don't have commit rights. Thanks for your help in advance.

This revision was automatically updated to reflect the committed changes.