[DWARFVerifier] Allow simplified template names in debug_name

[fdeazeve]

Summary

LLDB can benefit from having the base name of functions (i.e. without any
template parameters) as an entry into accelerator tables pointing back in the
DIE for the corresponding function specialization. In fact, some LLDB
functionality is only possible when those entries are present.

The DWARFLinker has been adding such entries for a while now, both with
apple_names and with debug_names. However, this has two side effects:

1. Some LLDB functionality is only possible when dsym bundles are present (i.e.

the linker touched the debug info).

2. The DWARFVerifier doesn't accept debug_name sections created by the linker,

as such names are (usually) neither the AT_name nor the AT_linkage_name of the
DIE.

Based on recent discussion [1], and because the DWARF 5 spec says that:

A producer may choose to implement additional rules for what names are placed
in the index

This patch relaxes the checks on the verifier to allow for simplified template
names in the accelerator table. To do so, we move some helper functions from
DWARFLinker into the core lib debug info. This addresses the point 2) above.

This patch also enables addressing point 1) in the future, since the helper
function is now visible to other parts of LLVM.

[1]: https://github.com/llvm/llvm-project/issues/58362

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1361–1363

Could roll this in together

[fdeazeve]

Address review comments

[avl]

LGTM, with small suggestions inside.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1354–1355

probably use 3 here as initial size? As it may return three names now.

1363

nit: other places use emplace_back.

[fdeazeve]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1354–1355

Good call

1363

Other places have a const char*, which is why they emplace, not push. In the grand scheme of things, either will generate the same code, but I think the push here is the more expressive call: we are communicating that we already have a StringRef. Do you agree?

[avl]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1363

I am not sure that I follow... Other places might also use push_back even if they have const char* and vice versa this place can use emplace_back. It is just looks more uniformly if the single variant is used.

[fdeazeve]

Use SmallVector of size 3 instead of 2.
Use emplace back to match the other uses in getNames.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1363

Other places might also use push_back even if they have const char*

Right, that's true because the StringRef(const char*) constructor is not explicit, but it is not generally true for all types. If we push_back(const char*), the StringRef is being implicitly constructed on the call to push_back; when we emplace_back(const char*), the StringRef is constructed inside the implementation of emplace_back.

But as I mentioned previously, this discussion is largely irrelevant for a simple type like StringRef, so I don't feel strongly either way. I'll use emplace_back!

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1363

FWIW - +1 from me, that we should prefer push_back/non-direct init (X y = z; rather than X y(z);) whenever possible, because those forms are "less powerful" and make the code easier to read because as a reader you don't have to worry about more powerful operations happening when you see that syntax/function name. (eg: if I see push_back(ptr) I don't have to worry that that's taking ownership - if the contianer was over unique_ptrs then you'd have to pass emplace_back(ptr) and it'd be a clearer marker that memory ownership was happening)

[fdeazeve]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1363

I think given the trade-offs discussed here of consistency with other methods vs expressiveness of the intent behind {push/emplace}_back, I am going to revert to what I had previously and use push_back.

@dblaikie do you have other concerns with this patch?

[fdeazeve]

Use push_back again

[avl]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1363

This is a minor issue and I do not ask for doing this way.
Though, I want to clarify my point. In this case, using
emplace_back(const char*) does not indicate that ownership
is taken. emplace_back(const char*) and emplace_back(StringRef)
are equal in that sense. When we have emplace_back() and push_back()
intermixed it looks like there is an important difference
in their usages. My original request was to do things in the same
way. All emplace_back(as in original code) or all push_back.

I think the intention for using emplace_back here was caused by
the desire to not call two constructors: one in push_back parameters (StringRef(const char*))
and another copy constructor inside push_back when placing an item into the vector.
Having all methods to be emplace_back shows that intention.

Having all methods to be push_back would show the intention that David has mentioned: using "less powerful" forms.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1363

if other code is using emplace_back where push_back is valid, then I think we should switch that code to use push_back - patches welcome?

(an extra StringRef ctor/move doesn't seem enough to justify a difference - that's pretty trivial)

[fdeazeve]

Sorry, I was on vacation for the past few weeks.
Based on the discussion here, I will commit the patch as it is, and then we can address the push vs emplace consistency issue separately.

[fdeazeve]

Oops, this created a warning in some bots. Will fix

llvm/include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h

public:
   const_iterator begin() const { return NameIndices.begin(); }
   const_iterator end() const { return NameIndices.end(); }
 
   /// Return the Name Index covering the compile unit at CUOffset, or nullptr if
   /// there is no Name Index covering that unit.
   const NameIndex *getCUNameIndex(uint64_t CUOffset);
 };
 
+/// If `Name` is the name of a templated function that includes template
+/// parameters, returns a substring of `Name` containing no template
+/// parameters.
+/// E.g.: StripTemplateParameters("foo<int>") = "foo".
+std::optional<StringRef> StripTemplateParameters(StringRef Name);
+
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFACCELERATORTABLE_H

llvm/lib/DWARFLinker/DWARFLinker.cpp

 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/CodeGen/NonRelocatableStringpool.h"
 #include "llvm/DWARFLinker/DWARFLinkerDeclContext.h"
 #include "llvm/DWARFLinker/DWARFStreamer.h"
 #include "llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h"
+#include "llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFDataExtractor.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugLine.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugMacro.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugRangeList.h"
 #include "llvm/DebugInfo/DWARF/DWARFDie.h"
 #include "llvm/DebugInfo/DWARF/DWARFExpression.h"
 #include "llvm/DebugInfo/DWARF/DWARFFormValue.h"
static bool isTypeTag(uint16_t Tag) {
   }
   return false;
 }
 
 AddressesMap::~AddressesMap() = default;
 
 DwarfEmitter::~DwarfEmitter() = default;
 
-static std::optional<StringRef> StripTemplateParameters(StringRef Name) {
-  // We are looking for template parameters to strip from Name. e.g.
-  //
-  //  operator<<B>
-  //
-  // We look for > at the end but if it does not contain any < then we
-  // have something like operator>>. We check for the operator<=> case.
-  if (!Name.endswith(">") || Name.count("<") == 0 || Name.endswith("<=>"))
-    return {};
-
-  // How many < until we have the start of the template parameters.
-  size_t NumLeftAnglesToSkip = 1;
-
-  // If we have operator<=> then we need to skip its < as well.
-  NumLeftAnglesToSkip += Name.count("<=>");
-
-  size_t RightAngleCount = Name.count('>');
-  size_t LeftAngleCount = Name.count('<');
-
-  // If we have more < than > we have operator< or operator<<
-  // we to account for their < as well.
-  if (LeftAngleCount > RightAngleCount)
-    NumLeftAnglesToSkip += LeftAngleCount - RightAngleCount;
-
-  size_t StartOfTemplate = 0;
-  while (NumLeftAnglesToSkip--)
-    StartOfTemplate = Name.find('<', StartOfTemplate) + 1;
-
-  return Name.substr(0, StartOfTemplate - 1);
-}
-
 bool DWARFLinker::DIECloner::getDIENames(const DWARFDie &Die,
                                          AttributesInfo &Info,
                                          OffsetsStringPool &StringPool,
                                          bool StripTemplate) {
   // This function will be called on DIEs having low_pcs and
   // ranges. As getting the name might be more expansive, filter out
   // blocks directly.
   if (Die.getTag() == dwarf::DW_TAG_lexical_block)

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

DWARFDebugNames::getCUNameIndex(uint64_t CUOffset) {
   if (CUToNameIndex.size() == 0 && NameIndices.size() > 0) {
     for (const auto &NI : *this) {
       for (uint32_t CU = 0; CU < NI.getCUCount(); ++CU)
         CUToNameIndex.try_emplace(NI.getCUOffset(CU), &NI);
     }
   }
   return CUToNameIndex.lookup(CUOffset);
 }
+
+std::optional<StringRef> llvm::StripTemplateParameters(StringRef Name) {
+  // We are looking for template parameters to strip from Name. e.g.
+  //
+  //  operator<<B>
+  //
+  // We look for > at the end but if it does not contain any < then we
+  // have something like operator>>. We check for the operator<=> case.
+  if (!Name.endswith(">") || Name.count("<") == 0 || Name.endswith("<=>"))
+    return {};
+
+  // How many < until we have the start of the template parameters.
+  size_t NumLeftAnglesToSkip = 1;
+
+  // If we have operator<=> then we need to skip its < as well.
+  NumLeftAnglesToSkip += Name.count("<=>");
+
+  size_t RightAngleCount = Name.count('>');
+  size_t LeftAngleCount = Name.count('<');
+
+  // If we have more < than > we have operator< or operator<<
+  // we to account for their < as well.
+  if (LeftAngleCount > RightAngleCount)
+    NumLeftAnglesToSkip += LeftAngleCount - RightAngleCount;
+
+  size_t StartOfTemplate = 0;
+  while (NumLeftAnglesToSkip--)
+    StartOfTemplate = Name.find('<', StartOfTemplate) + 1;
+
+  return Name.substr(0, StartOfTemplate - 1);
+}

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

if (!Attributes.count(dwarf::DW_IDX_die_offset)) {
           "NameIndex @ {0:x}: Abbreviation {1:x} has no {2} attribute.\n",
           NI.getUnitOffset(), Abbrev.Code, dwarf::DW_IDX_die_offset);
       ++NumErrors;
     }
   }
   return NumErrors;
 }
 
-static SmallVector<StringRef, 2> getNames(const DWARFDie &DIE,
+static SmallVector<StringRef, 3> getNames(const DWARFDie &DIE,
+                                          bool IncludeStrippedTemplateNames,
                                           bool IncludeLinkageName = true) {
   SmallVector<StringRef, 2> Result;
-  if (const char *Str = DIE.getShortName())
+  if (const char *Str = DIE.getShortName()) {
     Result.emplace_back(Str);
-  else if (DIE.getTag() == dwarf::DW_TAG_namespace)
+    if (IncludeStrippedTemplateNames) {
+      if (std::optional<StringRef> StrippedName =
+              StripTemplateParameters(Result.back()))
+        Result.push_back(*StrippedName);
+    }
+  } else if (DIE.getTag() == dwarf::DW_TAG_namespace)
     Result.emplace_back("(anonymous namespace)");
 
   if (IncludeLinkageName) {
     if (const char *Str = DIE.getLinkageName())
       Result.emplace_back(Str);
   }
 
   return Result;
for (; EntryOr; ++NumEntries, EntryID = NextEntryID,
     if (DIE.getTag() != EntryOr->tag()) {
       error() << formatv("Name Index @ {0:x}: Entry @ {1:x}: mismatched Tag of "
                          "DIE @ {2:x}: index - {3}; debug_info - {4}.\n",
                          NI.getUnitOffset(), EntryID, DIEOffset, EntryOr->tag(),
                          DIE.getTag());
       ++NumErrors;
     }
 
-    auto EntryNames = getNames(DIE);
+    // We allow an extra name for functions: their name without any template
+    // parameters.
+    auto IncludeStrippedTemplateNames =
+        DIE.getTag() == DW_TAG_subprogram ||
+        DIE.getTag() == DW_TAG_inlined_subroutine;
+    auto EntryNames = getNames(DIE, IncludeStrippedTemplateNames);
     if (!is_contained(EntryNames, Str)) {
       error() << formatv("Name Index @ {0:x}: Entry @ {1:x}: mismatched Name "
                          "of DIE @ {2:x}: index - {3}; debug_info - {4}.\n",
                          NI.getUnitOffset(), EntryID, DIEOffset, Str,
                          make_range(EntryNames.begin(), EntryNames.end()));
       ++NumErrors;
     }
   }
unsigned DWARFVerifier::verifyNameIndexCompleteness(
   // attribute are included with the name “(anonymous namespace)”.
   // All other debugging information entries without a DW_AT_name attribute
   // are excluded."
   // "If a subprogram or inlined subroutine is included, and has a
   // DW_AT_linkage_name attribute, there will be an additional index entry for
   // the linkage name."
   auto IncludeLinkageName = Die.getTag() == DW_TAG_subprogram ||
                             Die.getTag() == DW_TAG_inlined_subroutine;
-  auto EntryNames = getNames(Die, IncludeLinkageName);
+  // We *allow* stripped template names as an extra entry into the template,
+  // but we don't *require* them to pass the completeness test.
+  auto IncludeStrippedTemplateNames = false;
+  auto EntryNames =
+      getNames(Die, IncludeStrippedTemplateNames, IncludeLinkageName);
   if (EntryNames.empty())
     return 0;
 
   // We deviate from the specification here, which says:
   // "The name index must contain an entry for each debugging information entry
   // that defines a named subprogram, label, variable, type, or namespace,
   // subject to ..."
   // Explicitly exclude all TAGs that we know shouldn't be indexed.

llvm/test/tools/dsymutil/X86/dwarf5-accel.test

 ## This test checks that DIE name with stripped template parameters
 ## is stored into .debug_name section.
 
 ## cat dwarf5-accel.cpp
 ##
 ## template<typename A> void foo() {};
 ##
 ## int main ( void ) {
 ##   foo<char>();
 ##   return 0;
 ## }
 
 ## $ clang -gdwarf-5 dwarf5-accel.cpp -c -o dwarf5-accel.o
 
 #RUN: dsymutil -accelerator=Dwarf -oso-prepend-path %p/Inputs -y %s -o %t.dSYM
-#COM: Uncomment next line when dwarfdump will handle stripped template names.
-#COM: llvm-dwarfdump --verify  %t.dSYM | FileCheck %s --check-prefix VERIFY
+#RUN: llvm-dwarfdump --verify  %t.dSYM | FileCheck %s --check-prefix VERIFY
 #RUN: llvm-dwarfdump -a --verbose  %t.dSYM | FileCheck %s
 
-#COM: Uncomment next line when dwarfdump will handle stripped template names.
-#COM: #VERIFY: No errors.
+#VERIFY: No errors.
 
 #CHECK: .debug_names
 #CHECK: "foo"
 #CHECK: _Z3fooIcEvv
 #CHECK: "foo<char>"
 
 ---
 triple:          'x86_64-apple-darwin'
 objects:
   - filename:        'dwarf5-accel.o'
     timestamp:       1676048242
     symbols:
       - { sym: _main, objAddr: 0x0000000000000000, binAddr: 0x0000000100000AB0, size: 0x00000008 }
       - { sym: __Z3fooIcEvv, objAddr: 0x0000000000000020, binAddr: 0x0000000100000BB0, size: 0x00000008 }