[dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators

[shafik]

Summary

Currently dsymutil when generating accelerator tables will attempt to strip the template parameters from names for subroutines. For some overload operators which contain < in their names e.g. operator< the current method ends up stripping the operator name as well, so we just end up with the name operator in the table for each case.

[JDevlieghere]

llvm/lib/DWARFLinker/DWARFLinker.cpp

175–177

s/auto/StringRef/

179

This comment adds more confusion than it clarifies. The angle brackets being balanced doesn't really matter as this point. I'd say something like:

To strip the template we need to find at least one left angle bracket

181

Please follow the LLVM coding style. This applies to all variables. Also I'd rename this to LeftAngleBracketsToSkip.

185

Can this operator occur more than once?

192

The comment says not balanced, but you use > instead of !=. Does that matter?

200

Given that we always strip the end, can we simplify the algorithm to something like

size_t RightAngleCount = name.count('>');
size_t LeftAngleCount = name.count('<');
if (RightAngleCount != LeftAngleCount) {
	size_t AngleDifference = std::abs(RightAngleCount - LeftAngleCount);
	while (AngleDifference) {
		name = name.take_front(name.rfind('<'));
	}
}

That way we don't have to hardcode the <=> stuff.

200

Forgot to decrement AngleDifference: while (AngleDifference--) {

201

We should extract this logic in a static function, something like StringRef stripTemplateFromName(StringRef Str);.

llvm/test/tools/dsymutil/X86/template_operators.test

24

Please test operator<=> as well.

[shafik]

• Addressing comments
• Fixing bug, I was setting NameWithoutTemplate incorrectly in some cases.

[shafik]

llvm/lib/DWARFLinker/DWARFLinker.cpp

185

No

192

I clarified the comment (hopefully)

200

As discussed in person I am ignoring this comment.

llvm/test/tools/dsymutil/X86/template_operators.test

24

Good call, I caught a bug where I was setting NameWithoutTemplate incorrectly in this case.

[shafik]

llvm/lib/DWARFLinker/DWARFLinker.cpp

201

Just realized I missed this comment, I will extract this out in the next update.

[shafik]

Factored out template parameter stripping into its own function.

[JDevlieghere]

llvm/lib/DWARFLinker/DWARFLinker.cpp

123

For consistency with the other free functions in the cpp file, can you just make it static? This seems to be the default in llvm.

135

I still think that Skip would be more meaningful than Count

152

Please clang format your patch before landing.

176

You can move this into the if:

if (Optional<StringRef> StrippedName = StripTemplateParameters(Name))

[shafik]

Addressing comment

• Converting function to static Vs anon namespace
• Updating naming
• moving assignment into if
• clang-format

[JDevlieghere]

Thanks. LGTM

llvm/lib/DWARFLinker/DWARFLinker.cpp

static bool isTypeTag(uint16_t Tag) {
   }
   return false;
 }
 
 AddressesMap::~AddressesMap() {}
 
 DwarfEmitter::~DwarfEmitter() {}
 
+static 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)
     return false;
 
   // FIXME: a bit wasteful as the first getName might return the
   // short name.
   if (!Info.MangledName)
     if (const char *MangledName = Die.getName(DINameKind::LinkageName))
       Info.MangledName = StringPool.getEntry(MangledName);
 
   if (!Info.Name)
     if (const char *Name = Die.getName(DINameKind::ShortName))
       Info.Name = StringPool.getEntry(Name);
 
   if (StripTemplate && Info.Name && Info.MangledName != Info.Name) {
-    // FIXME: dsymutil compatibility. This is wrong for operator<
-    auto Split = Info.Name.getString().split('<');
-    if (!Split.second.empty())
-      Info.NameWithoutTemplate = StringPool.getEntry(Split.first);
+    StringRef Name = Info.Name.getString();
+    if (Optional<StringRef> StrippedName = StripTemplateParameters(Name))
+      Info.NameWithoutTemplate = StringPool.getEntry(*StrippedName);
   }
 
   return Info.Name || Info.MangledName;
 }
 
 /// Resolve the relative path to a build artifact referenced by DWARF by
 /// applying DW_AT_comp_dir.
 static void resolveRelativeObjectPath(SmallVectorImpl<char> &Buf, DWARFDie CU) {
   sys::path::append(Buf, dwarf::toString(CU.find(dwarf::DW_AT_comp_dir), ""));
 }
 
 /// Collect references to parseable Swift interfaces in imported
 /// DW_TAG_module blocks.
 static void analyzeImportedModule(
     const DWARFDie &DIE, CompileUnit &CU,
     swiftInterfacesMap *ParseableSwiftInterfaces,
     std::function<void(const Twine &, const DWARFDie &)> ReportWarning) {
   if (CU.getLanguage() != dwarf::DW_LANG_Swift)
     return;
 
   if (!ParseableSwiftInterfaces)
     return;
 
   StringRef Path = dwarf::toStringRef(DIE.find(dwarf::DW_AT_LLVM_include_path));
   if (!Path.endswith(".swiftinterface"))
     return;
   if (Optional<DWARFFormValue> Val = DIE.find(dwarf::DW_AT_name))
     if (Optional<const char *> Name = Val->getAsCString()) {
       auto &Entry = (*ParseableSwiftInterfaces)[*Name];
       // The prepend path is applied later when copying.
       DWARFDie CUDie = CU.getOrigUnit().getUnitDIE();
       SmallString<128> ResolvedPath;

llvm/test/tools/dsymutil/X86/template_operators.test

+RUN: dsymutil -oso-prepend-path %p/../Inputs %p/../Inputs/private/tmp/templated_operators/template_operators -o %t.apple.dSYM
+
+RUN: llvm-dwarfdump -apple-names %t.apple.dSYM | FileCheck %s -check-prefix=NAMES
+
+The test was compiled from a single source:
+$ cat template_operators.cpp
+template <typename T>
+bool operator<(const T&, const T&){
+ return true;
+}
+
+template <typename T>
+bool operator>(const T&, const T&){
+ return true;
+}
+
+template <typename T>
+bool operator<<(const T&, const T&){
+ return true;
+}
+
+template <typename T>
+bool operator>>(const T&, const T&){
+ return true;
+}
+
+template <typename T>
+bool operator==(const T&, const T&){
+ return true;
+}
+
+struct B{};
+
+struct D{};
+
+template <typename T>
+bool operator<=>(const T&,const T&){ return true;}
+
+int main() {
+  B b1;
+  B b2;
+  D d1;
+  D d2;
+
+   return b1 < b2 && b1 > b2 && b1 << b2 && b1 >> b2 && b1 == b2 && d1 <=> d2;
+}
+$ clang++ -std=c++2a -g template_operators.cpp -c -o template_operators.o
+$ clang template_operators.o -o template_operators
+
+
+NAMES-DAG: "operator<"
+NAMES-DAG: "operator<<B>"
+NAMES-DAG: "operator>"
+NAMES-DAG: "operator><B>"
+NAMES-DAG: "operator<<"
+NAMES-DAG: "operator<<<B>"
+NAMES-DAG: "operator>>"
+NAMES-DAG: "operator>><B>"
+NAMES-DAG: "operator<=>"
+NAMES-DAG: "operator<=><D>"
+NAMES-DAG: "operator=="
+NAMES-DAG: "operator==<B>"