This is an archive of the discontinued LLVM Phabricator instance.

[clang][TemplateBase] Add IsDefaulted bit to TemplateArgument
ClosedPublic

Authored by Michael137 on Jan 16 2023, 1:29 AM.

Details

Summary

Summary

This patch adds a IsDefaulted field to clang::TemplateArgument.

To prevent memory footprint increase we steal 1 bit from ArgKind.

Background

In LLDB we construct ASTs from debug-info and hand it to clang
to perform actions such as printing/formatting typenames.
Some debug formats, specifically DWARF, may only encode information
about class template instantiations, losing the structure of the generic
class definition. However, the clang::TypePrinter needs a properly
constructed ClassTemplateDecl with generic default argument decls
to be able to deduce whether a ClassTemplateSpecializationDecl was
instantiatiated with TemplateArguments that correspond to the
defaults. LLDB does know whether a particular template argument was
defaulted, but can't currently tell clang about it.

This patch allows LLDB to set the defaulted-ness of a TemplateArgument
and thus benefit more from clang::TypePrinter.

See discussion in https://reviews.llvm.org/D140423

Testing

  • TODO

Diff Detail

Event Timeline

Michael137 created this revision.Jan 16 2023, 1:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 1:29 AM
Michael137 requested review of this revision.Jan 16 2023, 1:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 1:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Michael137 edited the summary of this revision. (Show Details)Jan 16 2023, 1:46 AM
Michael137 edited the summary of this revision. (Show Details)Jan 16 2023, 1:52 AM

This seems innocuous enough/easy enough to use. I'd like a comment on the functions at least and types in TemplateBase.h to specify that this is for printing-policy only? Alternatively (and perhaps MUCH more appreciated) would be to make sure we mark the defaulted during AST generation as well.

Michael137 added a comment.EditedJan 17 2023, 7:36 AM

This seems innocuous enough/easy enough to use. I'd like a comment on the functions at least and types in TemplateBase.h to specify that this is for printing-policy only? Alternatively (and perhaps MUCH more appreciated) would be to make sure we mark the defaulted during AST generation as well.

I'll have a look at doing that

Are you suggesting we move the substitution check that the TypePrinter currently does to the point where we construct the specialisation decls? So the TypePrinter only needs to call TemplateArgument::getIsDefaulted? I like the sound of that.

Unfortunately we'd still need the setIsDefaulted because of the DWARF limitation in LLDB

This seems innocuous enough/easy enough to use. I'd like a comment on the functions at least and types in TemplateBase.h to specify that this is for printing-policy only? Alternatively (and perhaps MUCH more appreciated) would be to make sure we mark the defaulted during AST generation as well.

I'll have a look at doing that

Are you suggesting we do the substitution check that the TypePrinter currently does when constructing the specialisation decls? So the TypePrinter simply needs to check the TemplateArgument::getIsDefaulted? I like the sound of that.

Unfortunately we'd still need the setIsDefaulted because of the DWARF limitation in LLDB

Yes, thats my thought. Of course we'd still need the 'setIsDefaulted', but at least it would be something that coudl be generally useful.

This seems innocuous enough/easy enough to use. I'd like a comment on the functions at least and types in TemplateBase.h to specify that this is for printing-policy only? Alternatively (and perhaps MUCH more appreciated) would be to make sure we mark the defaulted during AST generation as well.

I'll have a look at doing that

Are you suggesting we do the substitution check that the TypePrinter currently does when constructing the specialisation decls? So the TypePrinter simply needs to check the TemplateArgument::getIsDefaulted? I like the sound of that.

Unfortunately we'd still need the setIsDefaulted because of the DWARF limitation in LLDB

Yes, thats my thought. Of course we'd still need the 'setIsDefaulted', but at least it would be something that coudl be generally useful.

Yeah, +1 for ensuring everything goes through the one solution (the IsDefaulted flag, in this case) - it'll make sure the flag support is well tested & is less likely to bitrot/break lldb's use case due to unrelated clang changes.

  • setIsDefaulted when constructing ClassTemplateSpecializationDecls
  • Propagate IsDefaulted on serialization
Michael137 added a comment.EditedJan 23 2023, 1:40 AM

This seems innocuous enough/easy enough to use. I'd like a comment on the functions at least and types in TemplateBase.h to specify that this is for printing-policy only? Alternatively (and perhaps MUCH more appreciated) would be to make sure we mark the defaulted during AST generation as well.

I'll have a look at doing that

Are you suggesting we do the substitution check that the TypePrinter currently does when constructing the specialisation decls? So the TypePrinter simply needs to check the TemplateArgument::getIsDefaulted? I like the sound of that.

Unfortunately we'd still need the setIsDefaulted because of the DWARF limitation in LLDB

Yes, thats my thought. Of course we'd still need the 'setIsDefaulted', but at least it would be something that coudl be generally useful.

So I've been having a go at this. The latest diff sets the bit when creating ClassTemplateSpecializationDecls. However, we end up having to copy the ArrayRef<TemplateArgument> twice (because of the current assumptions that a TemplateArgumentList is immutable, which seems like a nice property to maintain). That seems iffy.

I also had to add the flag to the TemplateArgument tablegen description to support deserialized ClassTemplateSpecializationDecl.

I've been playing around with setting the flag at construction of TemplateArguments instead. Not sure how much cleaner that's going to be because so far I've not found a good single point of entry (been looking at CheckTemplateArgumentList in SemaTemplate)

  • Rename test

This seems innocuous enough/easy enough to use. I'd like a comment on the functions at least and types in TemplateBase.h to specify that this is for printing-policy only? Alternatively (and perhaps MUCH more appreciated) would be to make sure we mark the defaulted during AST generation as well.

I'll have a look at doing that

Are you suggesting we do the substitution check that the TypePrinter currently does when constructing the specialisation decls? So the TypePrinter simply needs to check the TemplateArgument::getIsDefaulted? I like the sound of that.

Unfortunately we'd still need the setIsDefaulted because of the DWARF limitation in LLDB

Yes, thats my thought. Of course we'd still need the 'setIsDefaulted', but at least it would be something that coudl be generally useful.

So I've been having a go at this. The latest diff sets the bit when creating ClassTemplateSpecializationDecls. However, we end up having to copy the ArrayRef<TemplateArgument> twice (because of the current assumptions that a TemplateArgumentList is immutable, which seems like a nice property to maintain). That seems iffy.

I also had to add the flag to the TemplateArgument tablegen description to support deserialized ClassTemplateSpecializationDecl.

I've been playing around with setting the flag at construction of TemplateArguments instead. Not sure how much cleaner that's going to be because so far I've not found a good single point of entry (been looking at CheckTemplateArgumentList in SemaTemplate)

Yep, I think that'd be the idea, if there's somewhere to do it - rather than updating/copying it on every specialization.

  • Set IsDefaulted when checking template arguments in clang::Sema
  • Fix formatting
  • Don't need to pass flag to NewArgs
Michael137 retitled this revision from [WIP][clang][TemplateBase] Add IsDefaulted bit to TemplateArgument to [clang][TemplateBase] Add IsDefaulted bit to TemplateArgument.Jan 25 2023, 10:57 AM

Latest diff now sets the IsDefaulted flag from Sema::CheckTemplateArgumentList, which is the best point I could find for creation of TemplateArgument which are fed into the various specialization decls (and later on the TypePrinter).

The review description has some additional info about the changes.

As a result of moving the isSubstitutedDefaultArgument higher up into Sema, we can simplify the other remaining callers:

Still unsure if changing the TemplateArgument constructors is cleaner than simply calling setIsDefaulted from the tablegen creators but I'm open to suggestions.

  • Remove redundant constructor call change
erichkeane accepted this revision.Jan 26 2023, 6:58 AM
This revision is now accepted and ready to land.Jan 26 2023, 6:58 AM
This revision was landed with ongoing or failed builds.Jan 26 2023, 6:35 PM
This revision was automatically updated to reflect the committed changes.
bolshakov-a added inline comments.
clang/lib/AST/ASTContext.cpp
6842

@Michael137, hasn't IsDefaulted parameter forgotten here?