Page MenuHomePhabricator

[ASTImporter] Import default expression of param before creating the param.
ClosedPublic

Authored by balazske on Thu, Aug 1, 7:16 AM.

Details

Summary

The default expression of a parameter variable should be imported before
the parameter variable object is created. Otherwise the function is created
with an incomplete parameter variable (default argument is nullptr) and in
this intermediary state the expression is imported. This import can have
a reference to the incomplete parameter variable that causes crash.

Diff Detail

Event Timeline

balazske created this revision.Thu, Aug 1, 7:16 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
martong accepted this revision.Thu, Aug 1, 8:31 AM

LGTM, though I have some comments.

clang/test/Analysis/Inputs/ctu-other.cpp
135

Perhaps we could write Default instead of Def.

144

testImportOfIncompleteDefaultParm ?

This revision is now accepted and ready to land.Thu, Aug 1, 8:31 AM
shafik added inline comments.Thu, Aug 1, 9:47 AM
clang/lib/AST/ASTImporter.cpp
3892

This should be DefaultArg now?

clang/test/Analysis/Inputs/ctu-other.cpp
135

+1 to this and the name suggestion below.

balazske marked 2 inline comments as done.Fri, Aug 2, 12:23 AM
balazske added inline comments.
clang/lib/AST/ASTImporter.cpp
3892

I am not sure if it is always correct, specially in the hasUninstantiatedDefaultArg case. (The constructor calls setDefaultArg only, probably this was the reason to set the default arg afterwards in different ways that is not doable with the constructor.)

clang/test/Analysis/Inputs/ctu-other.cpp
144

The reason for this name was that the import is incomplete, not the the default parameter. (Exactly, before the fix, the import results in a ParmVarDecl that is for temporary time interval incomplete when the default expression is missing.) In this way testIncompleteImportOfDefaultParm can be better, or testImportWithIncompleteDefaultParm.

balazske updated this revision to Diff 213551.Tue, Aug 6, 2:21 AM

A new kind of solution, the previous does not work always.
Still no test for this new problem case (circular dependency between a ParmVarDecl and function of it and a CXXDefaultArgExpr that references to the function probably by UsedContext).

balazske updated this revision to Diff 213563.Tue, Aug 6, 3:09 AM

Small but important fix.

martong accepted this revision.Tue, Aug 6, 3:48 AM

Still looks good to me, other than some style nits.

clang/lib/AST/ASTImporter.cpp
3847

This line seems to be longer than 80 columns, forgot to run clang-format?

6964

This would be more appropriate I guess: "ParmVarDecl was not imported?"

clang/test/Analysis/Inputs/ctu-other.cpp
135

Please do not forget to rename.

Would it be an alternative solution if we imported the function parameters after we created the FunctionDecl? I guess it would be. Maybe that would result in a smaller change, what do you think?

balazske marked an inline comment as done.Thu, Aug 8, 7:30 AM
balazske added inline comments.
clang/test/Analysis/Inputs/ctu-other.cpp
135

I did not get response to the name of testDefParmIncompleteImport, I think testImportOfIncompleteDefaultParm is not good, probably testImportOfIncompleteDefaultParmDuringImport is better.

Yes importing the parameters should be doable after the create of the function. I am not sure if it fixes this problem, the parameter that has the default expression can still be encountered without the default expression set in it.

martong added inline comments.Sat, Aug 10, 1:44 AM
clang/test/Analysis/Inputs/ctu-other.cpp
135

Okay, so let's proceed with testImportOfIncompleteDefaultParmDuringImport.
Thanks!

balazske updated this revision to Diff 214531.Sat, Aug 10, 8:10 AM

Rename of variables and reformat.

balazske marked 7 inline comments as done.Sat, Aug 10, 8:17 AM
This revision was automatically updated to reflect the committed changes.