[DebugInfo][NFC] Move ObjC Selector name handling to lib DebugInfo

[fdeazeve]

Summary

The DWARFLinker library has code to identify ObjC selector names, which is used
by the debug linker to generate accelerator table entries. In the future, we
would like the DWARF verifier to also have access to such code, so that it can
identify these names when verifying accelerator tables (e.g. debug_names).

This patch follows the same intent of D155723, where we also moved code
generating simplified template names.

Since this is moving code around and changing the log, we also replace raw
pointer manipulation with the more expressive
StringRef::{drop_front,take_front,...} methods.

We also change a test so that it verifies its output, and that requires having
dsymutil not write to stdout.

[fdeazeve]

Make a test run the verifier.

[fdeazeve]

Remove duplicate line

[aprantl]

Couple of minor nitpicks, otherwise this LGTM.

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

752

Why plural?

755

Is the = part necessary or will it be default constructed as empty like llvm::Optional?

757

If there are any comments to explain the latter two members, that would be nice.

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

992

maybe that's for a later patch, but this seems to be a complicated way of saying
consumeFront("-[")
split(" ")

[fdeazeve]

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

752

Because there are 4 such names. But to be honest, I'm not sure I understand what an objective C selector is; maybe just "ObjectiveCNames" would be better?

755

It is constructed empty like an llvm::Optional, so this = std::nullopt is not needed, I'll remove it.

757

I'll do some research to figure out what they actually are and add some comments

[fdeazeve]

Address review comments

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

992

Agreed. I will do a second patch after the base changes are good.

[aprantl]

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

752

An ObjC selector is basically the name of a method: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ObjectiveC/Chapters/ocSelectors.html

When you send a message (~ call a method) in ObjC, you pass the selctor to objc_msgSend and it will look up the function behind that selector and invoke it. So if it's all selector names, let's keep it that way; it would be the right terminology.

759

[aprantl]

In any case this LGTM with the minor editorial comments above incorporated.

[fdeazeve]

Address review comments

[fdeazeve]

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

752

Thank you for the pointers!

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

 };
 
 /// 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);
 
+struct ObjectiveCNames {
+  // For "-[A(Category) method:]", this would be "method:"
+  StringRef Selector;
+  // For "-[A(Category) method:]", this would be "A(category)"
+  StringRef ClassName;
+  // For "-[A(Category) method:]", this would be "A"
+  std::optional<StringRef> ClassNameNoCategory;
+  // For "-[A(Category) method:]", this would be "A method:"
+  std::optional<std::string> MethodNameNoCategory;
+};
+
+/// If `Name` is the AT_name of a DIE which refers to an Objective-C selector,
+/// returns an instance of ObjectiveCNames. The Selector and ClassName fields
+/// are guaranteed to be non-empty in the result.
+std::optional<ObjectiveCNames> getObjCNamesIfSelector(StringRef Name);
+
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFACCELERATORTABLE_H

llvm/lib/DWARFLinker/DWARFLinker.cpp

Linker.reportWarning("Unsupported attribute form " +
                              dwarf::FormEncodingString(AttrSpec.Form) +
                              " in cloneAttribute. Dropping.",
                          File, &InputDIE);
   }
 
   return 0;
 }
 
-static bool isObjCSelector(StringRef Name) {
-  return Name.size() > 2 && (Name[0] == '-' || Name[0] == '+') &&
-         (Name[1] == '[');
-}
-
 void DWARFLinker::DIECloner::addObjCAccelerator(CompileUnit &Unit,
                                                 const DIE *Die,
                                                 DwarfStringPoolEntryRef Name,
                                                 OffsetsStringPool &StringPool,
                                                 bool SkipPubSection) {
-  assert(isObjCSelector(Name.getString()) && "not an objc selector");
-  // Objective C method or class function.
-  // "- [Class(Category) selector :withArg ...]"
-  StringRef ClassNameStart(Name.getString().drop_front(2));
-  size_t FirstSpace = ClassNameStart.find(' ');
-  if (FirstSpace == StringRef::npos)
-    return;
-
-  StringRef SelectorStart(ClassNameStart.data() + FirstSpace + 1);
-  if (!SelectorStart.size())
-    return;
-
-  StringRef Selector(SelectorStart.data(), SelectorStart.size() - 1);
-  Unit.addNameAccelerator(Die, StringPool.getEntry(Selector), SkipPubSection);
-
-  // Add an entry for the class name that points to this
-  // method/class function.
-  StringRef ClassName(ClassNameStart.data(), FirstSpace);
-  Unit.addObjCAccelerator(Die, StringPool.getEntry(ClassName), SkipPubSection);
-
-  if (ClassName[ClassName.size() - 1] == ')') {
-    size_t OpenParens = ClassName.find('(');
-    if (OpenParens != StringRef::npos) {
-      StringRef ClassNameNoCategory(ClassName.data(), OpenParens);
-      Unit.addObjCAccelerator(Die, StringPool.getEntry(ClassNameNoCategory),
-                              SkipPubSection);
-
-      std::string MethodNameNoCategory(Name.getString().data(), OpenParens + 2);
-      // FIXME: The missing space here may be a bug, but
-      //        dsymutil-classic also does it this way.
-      MethodNameNoCategory.append(std::string(SelectorStart));
-      Unit.addNameAccelerator(Die, StringPool.getEntry(MethodNameNoCategory),
-                              SkipPubSection);
-    }
-  }
+  std::optional<ObjectiveCNames> Names =
+      getObjCNamesIfSelector(Name.getString());
+  if (!Names)
+    return;
+  Unit.addNameAccelerator(Die, StringPool.getEntry(Names->Selector),
+  Unit.addObjCAccelerator(Die, StringPool.getEntry(Names->ClassName),
+  if (Names->ClassNameNoCategory)
+    Unit.addObjCAccelerator(
+        Die, StringPool.getEntry(*Names->ClassNameNoCategory), SkipPubSection);
+  if (Names->MethodNameNoCategory)
+    Unit.addNameAccelerator(
+        Die, StringPool.getEntry(*Names->MethodNameNoCategory), SkipPubSection);
 }
 
 static bool
 shouldSkipAttribute(bool Update,
                     DWARFAbbreviationDeclaration::AttributeSpec AttrSpec,
                     bool SkipPC) {
   switch (AttrSpec.Attr) {
   default:
if (AttrInfo.MangledName && AttrInfo.MangledName != AttrInfo.Name)
                               Tag == dwarf::DW_TAG_inlined_subroutine);
     if (AttrInfo.Name) {
       if (AttrInfo.NameWithoutTemplate)
         Unit.addNameAccelerator(Die, AttrInfo.NameWithoutTemplate,
                                 /* SkipPubSection */ true);
       Unit.addNameAccelerator(Die, AttrInfo.Name,
                               Tag == dwarf::DW_TAG_inlined_subroutine);
     }
-    if (AttrInfo.Name && isObjCSelector(AttrInfo.Name.getString()))
+    if (AttrInfo.Name)
       addObjCAccelerator(Unit, Die, AttrInfo.Name, DebugStrPool,
                          /* SkipPubSection =*/true);
 
   } else if (Tag == dwarf::DW_TAG_namespace) {
     if (!AttrInfo.Name)
       AttrInfo.Name = DebugStrPool.getEntry("(anonymous namespace)");
     Unit.addNamespaceAccelerator(Die, AttrInfo.Name);
   } else if (Tag == dwarf::DW_TAG_imported_declaration && AttrInfo.Name) {

llvm/lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

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);
 }
 
+static bool isObjCSelector(StringRef Name) {
+  return Name.size() > 2 && (Name[0] == '-' || Name[0] == '+') &&
+         (Name[1] == '[');
+}
+
+std::optional<ObjectiveCNames> llvm::getObjCNamesIfSelector(StringRef Name) {
+  if (!isObjCSelector(Name))
+    return std::nullopt;
+  // "-[Atom setMass:]"
+  StringRef ClassNameStart(Name.drop_front(2));
+  size_t FirstSpace = ClassNameStart.find(' ');
+  if (FirstSpace == StringRef::npos)
+    return std::nullopt;
+
+  StringRef SelectorStart = ClassNameStart.drop_front(FirstSpace + 1);
+  if (!SelectorStart.size())
+    return std::nullopt;
+
+  ObjectiveCNames Ans;
+  Ans.ClassName = ClassNameStart.take_front(FirstSpace);
+  Ans.Selector = SelectorStart.drop_back(); // drop ']';
+
+  // "-[Class(Category) selector :withArg ...]"
+  if (Ans.ClassName.back() == ')') {
+    size_t OpenParens = Ans.ClassName.find('(');
+    if (OpenParens != StringRef::npos) {
+      Ans.ClassNameNoCategory = Ans.ClassName.take_front(OpenParens);
+
+      Ans.MethodNameNoCategory = Name.take_front(OpenParens + 2);
+      // FIXME: The missing space here may be a bug, but dsymutil-classic also
+      // does it this way.
+      append_range(*Ans.MethodNameNoCategory, SelectorStart);
+    }
+  }
+  return Ans;
+}
+
 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("<=>"))

llvm/test/tools/dsymutil/X86/objc.test

-RUN: dsymutil -f -oso-prepend-path=%p/.. %p/../Inputs/objc.macho.x86_64 -o - \
-RUN:   | llvm-dwarfdump -apple-types -apple-objc - | FileCheck %s
+RUN: dsymutil --verify-dwarf=output -f -oso-prepend-path=%p/.. %p/../Inputs/objc.macho.x86_64 -o %t.d4
+RUN: llvm-dwarfdump -apple-types -apple-objc %t.d4 | FileCheck %s
 
 CHECK: .apple_types contents:
 CHECK: String: 0x00000066 "A"
 CHECK-NEXT: Data 0 [
 CHECK-NEXT:   Atom[0]: 0x0000012d
 CHECK-NEXT:   Atom[1]: 0x0013 (DW_TAG_structure_type)
 CHECK-NEXT:   Atom[2]: 0x02
 CHECK-NEXT:   Atom[3]: 0x0b87b15a