[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

174–176

s/auto/StringRef/

178

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

180

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

184

Can this operator occur more than once?

191

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

199

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.

199

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

200

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

184

No

191

I clarified the comment (hopefully)

199

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

200

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

122

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.

134

I still think that Skip would be more meaningful than Count

151

Please clang format your patch before landing.

175

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;
   // Don't track interfaces that are part of the SDK.
   StringRef SysRoot = dwarf::toStringRef(DIE.find(dwarf::DW_AT_LLVM_sysroot));
   if (SysRoot.empty())
     SysRoot = CU.getSysRoot();
   if (!SysRoot.empty() && Path.startswith(SysRoot))
     return;

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>"