This is an archive of the discontinued LLVM Phabricator instance.

[clang][ASTImporter] Improved handling of functions with auto return type.
ClosedPublic

Authored by balazske on Jul 13 2022, 6:58 AM.

Details

Summary

Avoid a crash if a function is imported that has auto return type that
references to a template with an expression-type of argument that
references into the function's body.
Fixes issue #56047

Diff Detail

Event Timeline

balazske created this revision.Jul 13 2022, 6:58 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
balazske requested review of this revision.Jul 13 2022, 6:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 6:58 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks, nice work!

clang/lib/AST/ASTImporter.cpp
3237
3237

The first call of getParents will create the parent map, via a full-blown AST visitation. I am concerned a bit about the additional performance overhead. Could you please run some measurements? (E.g. a CTU run on protobuf and bitcoin with our internal CI infra)

3251–3254

Should this be handled in a switch rather, perhaps with an llvm_unreachable at the default case? Just to make sure that no "kind" is left out.

3273–3277

Is it possible that T is both a RecordType and a TemplateSpecializationType at the same time? From the hierarchy this seems impossible (?)

clang/unittests/AST/ASTImporterTest.cpp
6323

Nice test!

balazske updated this revision to Diff 446057.Jul 20 2022, 12:19 AM
balazske marked 3 inline comments as done.

Small code NFC improvements.

balazske added inline comments.Jul 20 2022, 12:28 AM
clang/lib/AST/ASTImporter.cpp
3273–3277

The type dump shows that a RecordType is contained within the TemplateSpecializationType and it looks like that the get function handles this case. When the else is added the check of TemplateSpecializationType is skipped and the template argument expression X is not found (only integer constant 1).

TemplateSpecializationType 0x23fd8c0 'Tmpl<X>' sugar Tmpl
|-TemplateArgument expr
| `-ConstantExpr 0x23fd798 'int'
|   |-value: Int 1
|   `-ImplicitCastExpr 0x23fd780 'int' <LValueToRValue>
|     `-DeclRefExpr 0x23fd760 'const int' lvalue Var 0x23fd648 'X' 'const int' non_odr_use_constant
`-RecordType 0x23fd8a0 'struct Tmpl<1>'
  `-ClassTemplateSpecialization 0x23fd7b8 'Tmpl'
martong added inline comments.Jul 20 2022, 2:22 AM
clang/lib/AST/ASTImporter.cpp
3273–3277

Okay. I've dug deeper to understand how that is working. So, in the TemplateSpecializationType 'Tmpl<X>' is actually a "sugar" to the RecordType 'struct Tmpl<1>'.
And getAs<RecordType> will return with the desugared type:

// If this is a typedef for the type, strip the typedef off without
// losing all typedef information.
return cast<T>(getUnqualifiedDesugaredType());

Thus, to answer my own question, it is possible that T is both a RecordType and a TemplateSpecializationType at the same time!

martong accepted this revision.Jul 22 2022, 7:35 AM

LGTM

clang/lib/AST/ASTImporter.cpp
3237

The first call of getParents will create the parent map, via a full-blown AST visitation. I am concerned a bit about the additional performance overhead. Could you please run some measurements? (E.g. a CTU run on protobuf and bitcoin with our internal CI infra)

I've seen the measurement results, they look good.

We have errors in the CTU analysis of 'qtbase' and 'contour' even with the base line. The duration for the remaining 3 (xerces, protobuf, bincoin) looks good.

This revision is now accepted and ready to land.Jul 22 2022, 7:35 AM
This revision was landed with ongoing or failed builds.Jul 25 2022, 1:28 AM
This revision was automatically updated to reflect the committed changes.