DO NOT SUBMIT: workaround for context-sensitive use of non-type-template-parameter integer suffixes

[dblaikie]

Summary

There's a nuanced check about when to use suffixes on these integer
non-type-template-parameters, but when rebuilding names for
-gsimple-template-names there isn't enough data in the DWARF to
determine when to use suffixes or not. So turn on suffixes always to
make it easy to match up names in llvm-dwarfdump --verify.

I /think/ if we correctly modelled auto non-type-template parameters
maybe we could put suffixes only on those. But there's also some logic
in Clang that puts the suffixes on overloaded functions - at least
that's what the parameter says (see D77598 and printTemplateArguments
"TemplOverloaded" parameter) - but I think maybe it's for anything that
/can/ be overloaded, not necessarily only the things that are overloaded
(the argument value is hardcoded at the various callsites, doesn't seem
to depend on overload resolution/searching for overloaded functions). So
maybe with "auto" modeled more accurately, and differentiating between
function templates (always using type suffixes there) and class/variable
templates (only using the suffix for "auto" types) we could correctly
use integer type suffixes only in the minimal set of cases.

But maybe that's all too much fuss and we could/should just put them
everywhere always?

(more context:

• https://reviews.llvm.org/D77598#inline-1057607
• https://groups.google.com/g/llvm-dev/c/ekLMllbLIZg/m/-dhJ0hO1AAAJ )

[dblaikie]

Ping

[aprantl]

I think this is fine.

[shafik]

I think this is fine as well.

[ldionne]

This broke libc++'s CI: https://buildkite.com/llvm-project/libcxx-ci/builds/6374#7569da95-c852-44f9-8b69-947245cf0b65

When you make a change to Clang AND the libc++ tests at the same time, you have to account for the fact that we support older versions of Clang. For example, here, you changed the pretty printer test, but we still run that test with Clang 12 and Clang 13, not only with Clang main. Since older Clangs don't contain your change, the test fails (and rightfully so). I'll add the necessary XFAILs, but please look out for these breakages since they are somewhat disruptive. Thanks!

[ldionne]

Trying to fix this in https://reviews.llvm.org/D113112.

[dblaikie]

In D111477#3106171, @ldionne wrote:

This broke libc++'s CI: https://buildkite.com/llvm-project/libcxx-ci/builds/6374#7569da95-c852-44f9-8b69-947245cf0b65

When you make a change to Clang AND the libc++ tests at the same time, you have to account for the fact that we support older versions of Clang. For example, here, you changed the pretty printer test, but we still run that test with Clang 12 and Clang 13, not only with Clang main. Since older Clangs don't contain your change, the test fails (and rightfully so). I'll add the necessary XFAILs, but please look out for these breakages since they are somewhat disruptive. Thanks!

Thanks for the context/fix - I'll try to keep it in mind.

[dblaikie]

Some offline post-commit feedback from @rsmith - Rename printing policy to match closer to the function it's tested in. Will think about that, check the function name, etc, and commit a patch in the next few days, hopefully.

[dblaikie]

Renamed in 6512098877c3a230bbd38bc81b14a4e5844739ff to AlwaysIncludeTypeForTemplateArgument

clang/include/clang/AST/DeclTemplate.h

SourceRange getSourceRange() const LLVM_READONLY {
     return SourceRange(TemplateLoc, RAngleLoc);
   }
 
   void print(raw_ostream &Out, const ASTContext &Context,
              bool OmitTemplateKW = false) const;
   void print(raw_ostream &Out, const ASTContext &Context,
              const PrintingPolicy &Policy, bool OmitTemplateKW = false) const;
 
-  static bool shouldIncludeTypeForArgument(const TemplateParameterList *TPL,
+  static bool shouldIncludeTypeForArgument(const PrintingPolicy &Policy,
+                                           const TemplateParameterList *TPL,
                                            unsigned Idx);
 };
 
 /// Stores a list of template parameters and the associated
 /// requires-clause (if any) for a TemplateDecl and its derived classes.
 /// Suitable for creating on the stack.
 template <size_t N, bool HasRequiresClause>
 class FixedSizeTemplateParameterListStorage

clang/include/clang/AST/PrettyPrinter.h

PrintingPolicy(const LangOptions &LO)
         Nullptr(LO.CPlusPlus11), Restrict(LO.C99), Alignof(LO.CPlusPlus11),
         UnderscoreAlignof(LO.C11), UseVoidForZeroParams(!LO.CPlusPlus),
         SplitTemplateClosers(!LO.CPlusPlus11), TerseOutput(false),
         PolishForDeclaration(false), Half(LO.Half),
         MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
         MSVCFormatting(false), ConstantsAsWritten(false),
         SuppressImplicitBase(false), FullyQualifiedName(false),
         PrintCanonicalTypes(false), PrintInjectedClassNameWithArguments(true),
-        UsePreferredNames(true) {}
+        UsePreferredNames(true), UseIntegerTypeSuffixesAlways(false) {}
 
   /// Adjust this printing policy for cases where it's known that we're
   /// printing C++ code (for instance, if AST dumping reaches a C++-only
   /// construct). This should not be used if a real LangOptions object is
   /// available.
   void adjustForCPlusPlus() {
     SuppressTagKeyword = true;
     Bool = true;
struct PrintingPolicy {
 
   /// Whether to print types as written or canonically.
   unsigned PrintCanonicalTypes : 1;
 
   /// Whether to print an InjectedClassNameType with template arguments or as
   /// written. When a template argument is unnamed, printing it results in
   /// invalid C++ code.
   unsigned PrintInjectedClassNameWithArguments : 1;
+
+  /// Whether to use C++ template preferred_name attributes when printing
+  /// templates.
   unsigned UsePreferredNames : 1;
 
+  /// Whether to use type suffixes (eg: 1U) on integral non-type template
+  /// parameters.
+  unsigned UseIntegerTypeSuffixesAlways : 1;
+
   /// Callbacks to use to allow the behavior of printing to be customized.
   const PrintingCallbacks *Callbacks = nullptr;
 };
 
 } // end namespace clang
 
 #endif

clang/lib/AST/DeclPrinter.cpp

void DeclPrinter::printTemplateArguments(ArrayRef<TemplateArgument> Args,
                                          bool TemplOverloaded) {
   Out << "<";
   for (size_t I = 0, E = Args.size(); I < E; ++I) {
     if (I)
       Out << ", ";
     if (TemplOverloaded || !Params)
       Args[I].print(Policy, Out, /*IncludeType*/ true);
     else
-      Args[I].print(
-          Policy, Out,
-          TemplateParameterList::shouldIncludeTypeForArgument(Params, I));
+      Args[I].print(Policy, Out,
+                    TemplateParameterList::shouldIncludeTypeForArgument(
+                        Policy, Params, I));
   }
   Out << ">";
 }
 
 void DeclPrinter::printTemplateArguments(ArrayRef<TemplateArgumentLoc> Args,
                                          const TemplateParameterList *Params,
                                          bool TemplOverloaded) {
   Out << "<";
   for (size_t I = 0, E = Args.size(); I < E; ++I) {
     if (I)
       Out << ", ";
     if (TemplOverloaded)
       Args[I].getArgument().print(Policy, Out, /*IncludeType*/ true);
     else
       Args[I].getArgument().print(
           Policy, Out,
-          TemplateParameterList::shouldIncludeTypeForArgument(Params, I));
+          TemplateParameterList::shouldIncludeTypeForArgument(Policy, Params,
+                                                              I));
   }
   Out << ">";
 }
 
 void DeclPrinter::VisitTemplateDecl(const TemplateDecl *D) {
   printTemplateParameters(D->getTemplateParameters());
 
   if (const TemplateTemplateParmDecl *TTP =

clang/lib/AST/DeclTemplate.cpp

if (HasRequiresClause)
     AC.push_back(getRequiresClause());
 }
 
 bool TemplateParameterList::hasAssociatedConstraints() const {
   return HasRequiresClause || HasConstrainedParameters;
 }
 
 bool TemplateParameterList::shouldIncludeTypeForArgument(
-    const TemplateParameterList *TPL, unsigned Idx) {
-  if (!TPL || Idx >= TPL->size())
+    const PrintingPolicy &Policy, const TemplateParameterList *TPL,
+    unsigned Idx) {
+  if (!TPL || Idx >= TPL->size() || Policy.UseIntegerTypeSuffixesAlways)
     return true;
   const NamedDecl *TemplParam = TPL->getParam(Idx);
   if (const auto *ParamValueDecl =
           dyn_cast<NonTypeTemplateParmDecl>(TemplParam))
     if (ParamValueDecl->getType()->getContainedDeducedType())
       return true;
   return false;
 }

clang/lib/AST/Expr.cpp

for (SpecsTy::reverse_iterator I = Specs.rbegin(), E = Specs.rend();
       const TemplateParameterList *Params
                   = (*I)->getSpecializedTemplate()->getTemplateParameters();
       const TemplateArgumentList &Args = (*I)->getTemplateArgs();
       assert(Params->size() == Args.size());
       for (unsigned i = 0, numParams = Params->size(); i != numParams; ++i) {
         StringRef Param = Params->getParam(i)->getName();
         if (Param.empty()) continue;
         TOut << Param << " = ";
-        Args.get(i).print(
-            Policy, TOut,
-            TemplateParameterList::shouldIncludeTypeForArgument(Params, i));
+        Args.get(i).print(Policy, TOut,
+                          TemplateParameterList::shouldIncludeTypeForArgument(
+                              Policy, Params, i));
         TOut << ", ";
       }
     }
 
     FunctionTemplateSpecializationInfo *FSI
                                           = FD->getTemplateSpecializationInfo();
     if (FSI && !FSI->isExplicitSpecialization()) {
       const TemplateParameterList* Params

clang/lib/AST/TypePrinter.cpp

if (Argument.getKind() == TemplateArgument::Pack) {
       if (Argument.pack_size() && !FirstArg)
         OS << Comma;
       printTo(ArgOS, Argument.getPackAsArray(), Policy, TPL,
               /*IsPack*/ true, ParmIndex);
     } else {
       if (!FirstArg)
         OS << Comma;
       // Tries to print the argument with location info if exists.
-      printArgument(
-          Arg, Policy, ArgOS,
-          TemplateParameterList::shouldIncludeTypeForArgument(TPL, ParmIndex));
+      printArgument(Arg, Policy, ArgOS,
+                    TemplateParameterList::shouldIncludeTypeForArgument(
+                        Policy, TPL, ParmIndex));
     }
     StringRef ArgString = ArgOS.str();
 
     // If this is the first argument and its string representation
     // begins with the global scope specifier ('::foo'), add a space
     // to avoid printing the diagraph '<:'.
     if (FirstArg && !ArgString.empty() && ArgString[0] == ':')
       OS << ' ';

clang/lib/CodeGen/CGDebugInfo.cpp

if (CGM.getCodeGenOpts().EmitCodeView) {
     // For DWARF, printing rules are underspecified.
     // SplitTemplateClosers yields better interop with GCC and GDB (PR46052).
     PP.SplitTemplateClosers = true;
   }
 
   PP.SuppressInlineNamespace = false;
   PP.PrintCanonicalTypes = true;
   PP.UsePreferredNames = false;
+  PP.UseIntegerTypeSuffixesAlways = true;
 
   // Apply -fdebug-prefix-map.
   PP.Callbacks = &PrintCB;
   return PP;
 }
 
 StringRef CGDebugInfo::getFunctionName(const FunctionDecl *FD) {
   return internString(GetName(FD));

clang/lib/Sema/SemaDeclCXX.cpp

static std::string printTemplateArgs(const PrintingPolicy &PrintingPolicy,
                                      const TemplateParameterList *Params) {
   SmallString<128> SS;
   llvm::raw_svector_ostream OS(SS);
   bool First = true;
   unsigned I = 0;
   for (auto &Arg : Args.arguments()) {
     if (!First)
       OS << ", ";
-    Arg.getArgument().print(
-        PrintingPolicy, OS,
-        TemplateParameterList::shouldIncludeTypeForArgument(Params, I));
+    Arg.getArgument().print(PrintingPolicy, OS,
+                            TemplateParameterList::shouldIncludeTypeForArgument(
+                                PrintingPolicy, Params, I));
     First = false;
     I++;
   }
   return std::string(OS.str());
 }
 
 static bool lookupStdTypeTraitMember(Sema &S, LookupResult &TraitMemberLookup,
                                      SourceLocation Loc, StringRef Trait,

clang/lib/Sema/SemaTemplate.cpp

for (unsigned I = 0, N = Params->size(); I != N; ++I) {
 
     if (const IdentifierInfo *Id = Params->getParam(I)->getIdentifier()) {
       Out << Id->getName();
     } else {
       Out << '$' << I;
     }
 
     Out << " = ";
-    Args[I].print(
-        getPrintingPolicy(), Out,
-        TemplateParameterList::shouldIncludeTypeForArgument(Params, I));
+    Args[I].print(getPrintingPolicy(), Out,
+                  TemplateParameterList::shouldIncludeTypeForArgument(
+                      getPrintingPolicy(), Params, I));
   }
 
   Out << ']';
   return std::string(Out.str());
 }
 
 void Sema::MarkAsLateParsedTemplate(FunctionDecl *FD, Decl *FnD,
                                     CachedTokens &Toks) {

clang/test/CodeGenCXX/debug-info-template.cpp

 void func();
 
 // CHECK: [[TCI]] = !DIGlobalVariableExpression(var: [[TCIV:.*]], expr: !DIExpression())
 // CHECK: [[TCIV]] = distinct !DIGlobalVariable(name: "tci",
 // CHECK-SAME:                                  type: ![[TCNESTED:[0-9]+]]
 // CHECK: ![[TCNESTED]] ={{.*}}!DICompositeType(tag: DW_TAG_structure_type, name: "nested",
 // CHECK-SAME:             scope: ![[TC:[0-9]+]],
 
-// CHECK: ![[TC]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "TC<unsigned int, 2, &glb, &foo::e, &foo::f, &foo::g, 1, 2, 3>"
+// CHECK: ![[TC]] = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "TC<unsigned int, 2U, &glb, &foo::e, &foo::f, &foo::g, 1, 2, 3>"
 // CHECK-SAME:                              templateParams: [[TCARGS:![0-9]*]]
 TC
 // CHECK: [[EMPTY:![0-9]*]] = !{}
 // CHECK: [[TCARGS]] = !{[[TCARG1:![0-9]*]], [[TCARG2:![0-9]*]], [[TCARG3:![0-9]*]], [[TCARG4:![0-9]*]], [[TCARG5:![0-9]*]], [[TCARG6:![0-9]*]], [[TCARG7:![0-9]*]]}
 // CHECK: [[TCARG1]] = !DITemplateTypeParameter(name: "T", type: [[UINT:![0-9]*]])
 // CHECK: [[UINT:![0-9]*]] = !DIBasicType(name: "unsigned int"
 < unsigned,
 // CHECK: [[TCARG2]] = !DITemplateValueParameter(type: [[UINT]], value: i32 2)

clang/test/Modules/lsv-debuginfo.cpp

 
 // ADT
 // CHECK: @__clang_ast =
 
 // B
 // CHECK: @__clang_ast =
 
 // This type isn't anchored anywhere, expect a full definition.
-// CHECK: !DICompositeType({{.*}}, name: "AlignedCharArray<4, 16>",
+// CHECK: !DICompositeType({{.*}}, name: "AlignedCharArray<4U, 16U>",
 // CHECK-SAME:             elements:
 
 // C
 // CHECK: @__clang_ast =
 
 // Here, too.
-// CHECK: !DICompositeType({{.*}}, name: "AlignedCharArray<4, 16>",
+// CHECK: !DICompositeType({{.*}}, name: "AlignedCharArray<4U, 16U>",
 // CHECK-SAME:             elements:
 
 #include <B/B.h>

libcxx/test/libcxx/gdb/gdb_pretty_printer_test.sh.cpp

void unique_ptr_test() {
 
   std::unique_ptr<int> this_is_null;
   ComparePrettyPrintToChars(std::move(this_is_null),
       R"(std::unique_ptr is nullptr)");
 }
 
 void bitset_test() {
   std::bitset<258> i_am_empty(0);
-  ComparePrettyPrintToChars(i_am_empty, "std::bitset<258>");
+  ComparePrettyPrintToChars(i_am_empty, "std::bitset<258ul>");
 
   std::bitset<0> very_empty;
-  ComparePrettyPrintToChars(very_empty, "std::bitset<0>");
+  ComparePrettyPrintToChars(very_empty, "std::bitset<0ul>");
 
   std::bitset<15> b_000001111111100(1020);
   ComparePrettyPrintToChars(b_000001111111100,
-      "std::bitset<15> = {[2] = 1, [3] = 1, [4] = 1, [5] = 1, [6] = 1, "
+      "std::bitset<15ul> = {[2] = 1, [3] = 1, [4] = 1, [5] = 1, [6] = 1, "
       "[7] = 1, [8] = 1, [9] = 1}");
 
   std::bitset<258> b_0_129_132(0);
   b_0_129_132[0] = true;
   b_0_129_132[129] = true;
   b_0_129_132[132] = true;
   ComparePrettyPrintToChars(b_0_129_132,
-      "std::bitset<258> = {[0] = 1, [129] = 1, [132] = 1}");
+      "std::bitset<258ul> = {[0] = 1, [129] = 1, [132] = 1}");
 }
 
 void list_test() {
   std::list<int> i_am_empty{};
   ComparePrettyPrintToChars(i_am_empty, "std::list is empty");
 
   std::list<int> one_two_three {1, 2, 3};
   ComparePrettyPrintToChars(one_two_three,