Page MenuHomePhabricator

Fix resolution of certain recursive types.
ClosedPublic

Authored by sas on Mar 23 2015, 12:46 PM.

Details

Summary

If a struct type S has a member T that has a member that is a function that
returns a typedef of S* the respective field would be duplicated, which caused
an assert down the line in RecordLayoutBuilder. This patch adds a check that
removes the possibility of trying to resolve the same type twice within the
same callstack.

This commit also adds unit tests for these failures.

Fixes https://llvm.org/bugs/show_bug.cgi?id=20486.

Patch by Cristian Hancila.

Diff Detail

Repository
rL LLVM

Event Timeline

sas updated this revision to Diff 22506.Mar 23 2015, 12:46 PM
sas retitled this revision from to Fix resolution of certain recursive types..
sas updated this object.
sas edited the test plan for this revision. (Show Details)
sas added a reviewer: clayborg.
sas added a subscriber: Unknown Object (MLST).
sas added a comment.Mar 23 2015, 12:48 PM

I'd like to add some unit tests for this patch but I don't know how to create one that has two separate source files (which is required to repro this bug). Does the unit test framework support that?

clayborg requested changes to this revision.Mar 23 2015, 1:29 PM
clayborg edited edge metadata.

See inline comments.

You can easily make a test case with two files, there are many that do it.

source/Symbol/ClangASTImporter.cpp
289–296 ↗(On Diff #22506)

Do we always want to set this to true? If we really just have a forward declaration in the DWARF "result" might be false. If we set this to true, it means anytime we have a forward declaration to a class, we might start and complete the class definition and say that is is a complete class that contains nothing. Then in another ClangASTContext we might have a full definition of the type, then we try to import both types into an expression ClangASTContext and it will fail saying there are two definitions. You should add any needed smarts to ComplateTagDecl() and not do anything inline like this.

This revision now requires changes to proceed.Mar 23 2015, 1:29 PM
spyffe added a subscriber: spyffe.Mar 23 2015, 1:34 PM

Stéphane,

please add me as a reviewer for this patch. I have an account, spyffe.

Sean

sas added a reviewer: spyffe.Mar 23 2015, 6:46 PM
sas added inline comments.Mar 24 2015, 3:23 PM
source/Symbol/ClangASTImporter.cpp
289–296 ↗(On Diff #22506)

This code follows what we have at lines 202-207 so I'm not sure if we're doing something wrong.

I'll factor the code in CompleteTagDecl as you suggested and let Sean comment on what's the right thing to do here.

sas updated this revision to Diff 22618.Mar 24 2015, 5:18 PM
sas edited edge metadata.

Add unit tests and address review comments.

sas updated this object.Mar 24 2015, 5:21 PM
sas edited the test plan for this revision. (Show Details)
sas edited edge metadata.
spyffe requested changes to this revision.Mar 30 2015, 2:17 PM
spyffe edited edge metadata.

This looks good, just make the RAII change so that everybody editing the code doesn't have to remember this.

source/Expression/ClangASTSource.cpp
483 ↗(On Diff #22618)

This should be done in a RAII way, with some object that deregisters context_decl when the function returns. That way we don't have to remember to explicitly deregister it wherever we return.

This revision now requires changes to proceed.Mar 30 2015, 2:17 PM
sas updated this revision to Diff 23198.Apr 2 2015, 6:51 PM
sas edited edge metadata.

Use a scoped class to do removals.

clayborg accepted this revision.Apr 3 2015, 9:21 AM
clayborg edited edge metadata.

I am ok as long as Sean Callanan is ok with this.

spyffe accepted this revision.Apr 8 2015, 1:39 PM
spyffe edited edge metadata.

Looks good! Thanks for the patch.

This revision is now accepted and ready to land.Apr 8 2015, 1:39 PM
sas added a comment.Apr 8 2015, 1:46 PM

Will submit this to trunk. Spoke with Sean about porting to release_36 as well after letting it sit for a couple of weeks on trunk, he's fine with it. Will do that too.

This revision was automatically updated to reflect the committed changes.