[clang][TemplateBase] Add IsDefaulted bit to TemplateArgument

[Michael137]

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

[erichkeane]

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]

In D141826#4058866, @erichkeane wrote:

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

[erichkeane]

In D141826#4059073, @Michael137 wrote:

In D141826#4058866, @erichkeane wrote:

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.

[dblaikie]

In D141826#4059088, @erichkeane wrote:

In D141826#4059073, @Michael137 wrote:

In D141826#4058866, @erichkeane wrote:

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.

[Michael137]

• setIsDefaulted when constructing ClassTemplateSpecializationDecls
• Propagate IsDefaulted on serialization

[Michael137]

In D141826#4059088, @erichkeane wrote:

In D141826#4059073, @Michael137 wrote:

In D141826#4058866, @erichkeane wrote:

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)

[Michael137]

• Rename test

[dblaikie]

In D141826#4072609, @Michael137 wrote:

In D141826#4059088, @erichkeane wrote:

In D141826#4059073, @Michael137 wrote:

In D141826#4058866, @erichkeane wrote:

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.

[Michael137]

• Set IsDefaulted when checking template arguments in clang::Sema

[Michael137]

• Fix formatting

[Michael137]

• Don't need to pass flag to NewArgs

[Michael137]

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:

• https://reviews.llvm.org/D141827
• https://reviews.llvm.org/D142333

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

[Michael137]

• Remove redundant constructor call change

[bolshakov-a]

clang/lib/AST/ASTContext.cpp

6842 ↗

(On Diff #492626)

@Michael137, hasn't IsDefaulted parameter forgotten here?

clang/include/clang/AST/TemplateBase.h

enum ArgKind {
     /// in the Args struct.
     Pack
   };
 
 private:
   /// The kind of template argument we're storing.
 
   struct DA {
-    unsigned Kind;
+    unsigned Kind : 31;
+    unsigned IsDefaulted : 1;
     void *QT;
     ValueDecl *D;
   };
   struct I {
-    unsigned Kind;
+    unsigned Kind : 31;
+    unsigned IsDefaulted : 1;
     // We store a decomposed APSInt with the data allocated by ASTContext if
     // BitWidth > 64. The memory may be shared between multiple
     // TemplateArgument instances.
     unsigned BitWidth : 31;
     unsigned IsUnsigned : 1;
     union {
       /// Used to store the <= 64 bits integer value.
       uint64_t VAL;
 
       /// Used to store the >64 bits integer value.
       const uint64_t *pVal;
     };
     void *Type;
   };
   struct A {
-    unsigned Kind;
+    unsigned Kind : 31;
+    unsigned IsDefaulted : 1;
     unsigned NumArgs;
     const TemplateArgument *Args;
   };
   struct TA {
-    unsigned Kind;
+    unsigned Kind : 31;
+    unsigned IsDefaulted : 1;
     unsigned NumExpansions;
     void *Name;
   };
   struct TV {
-    unsigned Kind;
+    unsigned Kind : 31;
+    unsigned IsDefaulted : 1;
     uintptr_t V;
   };
   union {
     struct DA DeclArg;
     struct I Integer;
     struct A Args;
     struct TA TemplateArg;
     struct TV TypeOrValue;
   };
 
 public:
   /// Construct an empty, invalid template argument.
-  constexpr TemplateArgument() : TypeOrValue({Null, 0}) {}
+  constexpr TemplateArgument() : TypeOrValue({Null, /* IsDefaulted */ 0, 0}) {}
 
   /// Construct a template type argument.
   TemplateArgument(QualType T, bool isNullPtr = false) {
     TypeOrValue.Kind = isNullPtr ? NullPtr : Type;
+    TypeOrValue.IsDefaulted = false;
     TypeOrValue.V = reinterpret_cast<uintptr_t>(T.getAsOpaquePtr());
   }
 
   /// Construct a template argument that refers to a
   /// declaration, which is either an external declaration or a
   /// template declaration.
   TemplateArgument(ValueDecl *D, QualType QT) {
     assert(D && "Expected decl");
     DeclArg.Kind = Declaration;
+    DeclArg.IsDefaulted = false;
     DeclArg.QT = QT.getAsOpaquePtr();
     DeclArg.D = D;
   }
 
   /// Construct an integral constant template argument. The memory to
   /// store the value is allocated with Ctx.
   TemplateArgument(ASTContext &Ctx, const llvm::APSInt &Value, QualType Type);
 
public:
   /// This form of template argument is generally used for template template
   /// parameters. However, the template name could be a dependent template
   /// name that ends up being instantiated to a function template whose address
   /// is taken.
   ///
   /// \param Name The template name.
   TemplateArgument(TemplateName Name) {
     TemplateArg.Kind = Template;
+    TemplateArg.IsDefaulted = false;
     TemplateArg.Name = Name.getAsVoidPointer();
     TemplateArg.NumExpansions = 0;
   }
 
   /// Construct a template argument that is a template pack expansion.
   ///
   /// This form of template argument is generally used for template template
   /// parameters. However, the template name could be a dependent template
   /// name that ends up being instantiated to a function template whose address
   /// is taken.
   ///
   /// \param Name The template name.
   ///
   /// \param NumExpansions The number of expansions that will be generated by
   /// instantiating
   TemplateArgument(TemplateName Name, Optional<unsigned> NumExpansions) {
     TemplateArg.Kind = TemplateExpansion;
+    TemplateArg.IsDefaulted = false;
     TemplateArg.Name = Name.getAsVoidPointer();
     if (NumExpansions)
       TemplateArg.NumExpansions = *NumExpansions + 1;
     else
       TemplateArg.NumExpansions = 0;
   }
 
   /// Construct a template argument that is an expression.
   ///
   /// This form of template argument only occurs in template argument
   /// lists used for dependent types and for expression; it will not
   /// occur in a non-dependent, canonical template argument list.
   TemplateArgument(Expr *E) {
     TypeOrValue.Kind = Expression;
+    TypeOrValue.IsDefaulted = false;
     TypeOrValue.V = reinterpret_cast<uintptr_t>(E);
   }
 
   /// Construct a template argument that is a template argument pack.
   ///
   /// We assume that storage for the template arguments provided
   /// outlives the TemplateArgument itself.
   explicit TemplateArgument(ArrayRef<TemplateArgument> Args) {
     this->Args.Kind = Pack;
+    this->Args.IsDefaulted = false;
     this->Args.Args = Args.data();
     this->Args.NumArgs = Args.size();
   }
 
   TemplateArgument(TemplateName, bool) = delete;
 
   static TemplateArgument getEmptyPack() {
     return TemplateArgument(std::nullopt);
QualType getIntegralType() const {
     return QualType::getFromOpaquePtr(Integer.Type);
   }
 
   void setIntegralType(QualType T) {
     assert(getKind() == Integral && "Unexpected kind");
     Integer.Type = T.getAsOpaquePtr();
   }
 
+  void setIsDefaulted(bool v) { TypeOrValue.IsDefaulted = v; }
+
+  bool getIsDefaulted() const { return (bool)TypeOrValue.IsDefaulted; }
+
   /// If this is a non-type template argument, get its type. Otherwise,
   /// returns a null QualType.
   QualType getNonTypeTemplateArgumentType() const;
 
   /// Retrieve the template argument as an expression.
   Expr *getAsExpr() const {
     assert(getKind() == Expression && "Unexpected kind");
     return reinterpret_cast<Expr *>(TypeOrValue.V);

clang/lib/AST/TemplateBase.cpp

 
 //===----------------------------------------------------------------------===//
 // TemplateArgument Implementation
 //===----------------------------------------------------------------------===//
 
 TemplateArgument::TemplateArgument(ASTContext &Ctx, const llvm::APSInt &Value,
                                    QualType Type) {
   Integer.Kind = Integral;
+  Integer.IsDefaulted = false;
   // Copy the APSInt value into our decomposed form.
   Integer.BitWidth = Value.getBitWidth();
   Integer.IsUnsigned = Value.isUnsigned();
   // If the value is large, we have to get additional memory from the ASTContext
   unsigned NumWords = Value.getNumWords();
   if (NumWords > 1) {
     void *Mem = Ctx.Allocate(NumWords * sizeof(uint64_t));
     std::memcpy(Mem, Value.getRawData(), NumWords * sizeof(uint64_t));