[DWARFVerifier] Allow ObjectiveC names in dwarf_debug tables

[fdeazeve]

Summary

ObjectiveC has its own extra accelerator table entries that are helpful for the
debugger. This patch relaxes the DWARFVerifier so that it accepts those in DWARF
5's debug_names.

[aprantl]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1366

I'm a little confused by this comment. "first" as opposed to when else?

[fdeazeve]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1366

As opposed to doing an Result.emplace_back(StrippedName), which would first ensure the vector has enough space, and then construct the std::string in-place from the (now possibly invalid) StringRef.

This is extremely subtle, but I will try to reword the comment

[fdeazeve]

Just to register in case there are any concerns with performance here, I built Clang in Debug, generated a dsym with dsymutil build_Debug/bin/clang --accelerator Dwarf and measured the time it takes to verify it: build_Release/bin/llvm-dwarfdump --verify build_Debug/bin/clang.dSYM

Baseline:
  Time (mean ± σ):     51.624 s ±  0.772 s    [User: 50.222 s, System: 1.381 s]
  Range (min … max):   50.867 s … 53.138 s    10 runs
With Patch:
  Time (mean ± σ):     50.941 s ±  0.090 s    [User: 49.561 s, System: 1.342 s]
  Range (min … max):   50.808 s … 51.075 s    10 runs

[fdeazeve]

This is exposing an issue with lldb/test/API/lang/objc/modules-objc-property/TestModulesObjCProperty.py.
This test compiles an ObjectiveC program and runs dsymutil on the binary.
By default, dsymutil verifies the output only if the input is valid.
With this patch, the input is now considered valid. So dsymutil verifies the output, which turns out to be invalid (for other reasons?)
If I don't apply this patch and force dsymutil to verify the output of that test, it also says the output is invalid.

build_Release/./bin/dsymutil"  -o "a.out.dSYM" "a.out"
Verifying .debug_abbrev...
Verifying .debug_info Unit Header Chain...
Verifying .debug_types Unit Header Chain...
Verifying non-dwo Units...
Verifying unit: 1 / 6, "_Builtin_stddef_max_align_t"
Verifying unit: 2 / 6, "Darwin"
Verifying unit: 3 / 6, "MachO"
Verifying unit: 4 / 6, "ObjectiveC"
Verifying unit: 5 / 6, "myModule"
Verifying unit: 6 / 6, "/Users/piovezan/workspace/llvm-project/worktrees/src1/lldb/test/API/lang/objc/modules-objc-property/main.m"
Verifying dwo Units...
Verifying .debug_line...
warning: .debug_line[0x00003b18].prologue.file_names[2] is a duplicate of file_names[0]
Verifying .debug_str_offsets...
error: .debug_str_offsets: contribution 0x0: index 0x0: invalid string offset *0x0 == 0x5C, is neither zero nor immediately following a null character
error: .debug_str_offsets: contribution 0x0: index 0x1: invalid string offset *0x4 == 0x5, is neither zero nor immediately following a null character
Verifying .debug_names...
error: output verification failed for arm64
make: *** [a.out.dSYM] Error 1

[fdeazeve]

Since this is DWARF5/ObjectiveC only, and since it resolves more failures than it creates, I'll go ahead and merge it.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1367

Technically, that'd be the behavior of this code even if it used * instead of ->str(), right? Since push_back takes a T&& parameter the StringRef->std::string implicit conversion would happen first, before the function call.

So I'm not sure the use of ->str() is sufficiently more explicit to make it clear what's going on here. Maybe the comment is sufficient and we could use *, or maybe it isn't, and a named temporary (std::string X = *y; func(std::move(X);) might be better, or an explicit cast (I guess that's no better than the ->str())?

I'm not feeling strongly about any of this - if the comment and the slightly different syntax feels like the right tradeoff to you folks, I won't stand in the way.

[fdeazeve]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1367

Oh, you are right; I could have done just * instead of ->str(), I totally missed that! I can probably simplify the comment a bit with that in mind.

I think the only important thing we emphasize with the comment is that we need to push instead of emplacing the StringRef, as this is a trap a well-intentioned developer could fall into during future code changes.

Personally I don't think the temporary would improve things a lot, as in the future someone could think "oh, let me remove this temporary that is just used once", not realizing it was done because of this subtle problem.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp

1367

Fair enough - probably just leave/rephrase the comment (to try to reduce @aprantl's confusion) and replace the ->str() with *.

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, 3> getNames(const DWARFDie &DIE,
+static SmallVector<std::string, 3> getNames(const DWARFDie &DIE,
                                             bool IncludeStrippedTemplateNames,
+                                            bool IncludeObjCNames = true,
                                             bool IncludeLinkageName = true) {
-  SmallVector<StringRef, 3> Result;
+  SmallVector<std::string, 3> Result;
   if (const char *Str = DIE.getShortName()) {
-    Result.emplace_back(Str);
+    StringRef Name(Str);
+    Result.emplace_back(Name);
     if (IncludeStrippedTemplateNames) {
       if (std::optional<StringRef> StrippedName =
               StripTemplateParameters(Result.back()))
-        Result.push_back(*StrippedName);
+        // Convert to std::string and push; emplacing the StringRef may trigger
+        // a vector resize which may destroy the StringRef memory.
+        Result.push_back(StrippedName->str());
+    }
+
+    if (IncludeObjCNames) {
+      if (std::optional<ObjCSelectorNames> ObjCNames =
+              getObjCNamesIfSelector(Name)) {
+        Result.emplace_back(ObjCNames->ClassName);
+        Result.emplace_back(ObjCNames->Selector);
+        if (ObjCNames->ClassNameNoCategory)
+          Result.emplace_back(*ObjCNames->ClassNameNoCategory);
+        if (ObjCNames->MethodNameNoCategory)
+          Result.push_back(std::move(*ObjCNames->MethodNameNoCategory));
+      }
     }
   } 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);
   }
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;
-  // We *allow* stripped template names as an extra entry into the template,
-  // but we don't *require* them to pass the completeness test.
+  // We *allow* stripped template names / ObjectiveC names as extra entries into
+  // the table, but we don't *require* them to pass the completeness test.
   auto IncludeStrippedTemplateNames = false;
-  auto EntryNames =
-      getNames(Die, IncludeStrippedTemplateNames, IncludeLinkageName);
+  auto IncludeObjCNames = false;
+  auto EntryNames = getNames(Die, IncludeStrippedTemplateNames,
+                             IncludeObjCNames, 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/objc.test

 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
 
+; Test DWARF 5 tables
+RUN: dsymutil --verify-dwarf=output --accelerator='Dwarf' -f -oso-prepend-path=%p/.. %p/../Inputs/objc.macho.x86_64 -o %t.d5
+RUN: llvm-dwarfdump -debug-names %t.d5 | FileCheck %s --check-prefix=D5
+
+D5: String: {{.*}} "method1:"
+D5: String: {{.*}} "method2:"
+D5: String: {{.*}} "A"
+D5: String: {{.*}} "-[A(Category) method2:]"
+D5: String: {{.*}} "-[A method1:]"
+D5: String: {{.*}} "-[Amethod2:]"
+
 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
 CHECK-NEXT: ]