Add DW_AT_linkage_name for DW_TAG_labels

[dimitry]

Summary

Since DW_AT_name is not a symbol name for DW_TAG_label DIEs
add DW_AT_linkage_name with original symbol name.

Bug: https://bugs.llvm.org/show_bug.cgi?id=37470

[davide]

This needs a testcase.

[aprantl]

I'm not sure that stripping the leading _ unconditionally is correct either. This code exists to emit DWARF for labels in assembler source code and I'm assuming that this patch will break that use-case.

[aprantl]

It would be good to add both a testcase for your intended use-case and one with an assembler label (if that doesn't exist already).

[dimitry]

Interestingly the test specifically checks if underscore is removed. Looks like this was very much intentional. But I still do not understand why.. It makes the DW_AT_name of the label differ from elf symbol name (in .dynsym section).

[dblaikie]

The goal would be for the name to match what the user wrote - not
necessarily what the symbol name is.

[dimitry]

It looks like in our case it ended up being not what the user wrote.. and this is especially confusing because libc and exit symbol too (and __exit, which gets converted to _exit...

[probinson]

If there's a difference between the in-source name and the exposed-to-linker name, the in-source name should be provided to DW_AT_name and the exposed-to-linker name should be in DW_AT_linkage_name. Don't try to overload DW_AT_name with any kind of decoration imposed for linking purposes.

[dimitry]

Added DW_AT_linkage_name instead of keeping underscore in DW_AT_name.

[dimitry]

git clang-format --style=file

[dblaikie]

Is this correct? What if the user has a label called "_foo"?

I would've thought this '_' handling thing was specifically a OSX/iOS thing (from what I've seen in its handling in LLVM in other places) - so I'm surprised it isn't conditionalized in some way?

[dimitry]

In D46878#1099526, @aprantl wrote:

It would be good to add both a testcase for your intended use-case and one with an assembler label (if that doesn't exist already).

I am not sure if I understand - my intended use-case here is in fact an assembler label (a .glob label). Can you please give me a pointer to another use-case? I am not very familiar with llvm code base.

[dimitry]

ping

[aprantl]

I think this looks good now.

[dimitry]

There is still a question about which fix is the correct one. This one or https://reviews.llvm.org/D47104. To reiterate which use case requires clang to remove leading underscores in DW_AT_name?

[dimitry]

ping?

[JDevlieghere]

Based on @echristo's comment in D47104 it feels like this is the preferable way to move forward with this.

[echristo]

LGTM. Thanks!

-eric

[JohnReagan]

lib/MC/MCDwarf.cpp

562

Uh, DW_AT_linkage_name isn't intended to be used with DW_TAG_labels. It is really meant to provide the mangled names with entries, etc. From DWARF4 spec

"Debugging information entries to which DW_AT_linkage_name may apply include: DW_TAG_common_block, DW_TAG_constant, DW_TAG_entry_point, DW_TAG_subprogram and DW_TAG_variable."

DWARF5 dropped that wording, but you can still infer that list from Appendix A, etc.

I can see that we already emit DW_AT_prototyped for DW_TAG_labels (also extra to the standard).

There are other debuggers who consume this stuff. We're about to start work on the OpenVMS debugger to bring it up past DWARF3 and I would have assumed (incorrectly) that nobody would emit these DW_AT_ attributes for labels.

Is there a document somewhere that tracks these extra-standard extensions?

[aprantl]

lib/MC/MCDwarf.cpp

562

DWARF5 dropped that wording, but you can still infer that list from Appendix A, etc.

I wonder if the information in Appendix A (which is non-normative) isn't an oversight. Might be worth bringing this up on the dwarf-discuss mailing list.

Is there a document somewhere that tracks these extra-standard extensions?

Not that I'm aware of. Generally LLVM tends to be fairly permissive about emitting extra dwarf attributes based on that consumers can always ignore extra attributes they don't understand.

[probinson]

lib/MC/MCDwarf.cpp

562

Appendix A is really a best-effort list, not definitive (which is explicitly stated in the opening text of the appendix). DWARF rarely mandates anything, and permissively allows extra attributes not specified in the normative text to appear in a DIE. Any reader needs to be ready for that sort of thing.
In the case at hand, it seems there are situations where the label's name in the source text is not the same as the name exposed to the linker, and that's pretty much exactly what DW_AT_linkage_name is for. I might prefer that the extra attribute appear only when needed, but we don't make a huge effort to support the best possible debug info for assembler source (which is the case here).

[JohnReagan]

lib/MC/MCDwarf.cpp

562

Got it. Thanks.

I do agree about omitting it if the linkage_name is the same as the name since that reduces the number of references to the string table. Given your lightning talk and the reduction of string references that led to the changes for DWARF5, no sense making a DWARF4 (or earlier) user pay that extra cost. You do end up with needing two Abbrevs however.

I still have a question about the use of DW_AT_prototyped (the normative description is in the "Miscellaneous Subprogram Properties" section) and what it means on a label but I'll go dig for when that was included and the rationale behind it.

include/llvm/MC/MCDwarf.h

 };
 
 // When generating dwarf for assembly source files this is the info that is
 // needed to be gathered for each symbol that will have a dwarf label.
 class MCGenDwarfLabelEntry {
 private:
   // Name of the symbol without a leading underbar, if any.
   StringRef Name;
+  // The original symbol name.
+  StringRef LinkageName;
   // The dwarf file number this symbol is in.
   unsigned FileNumber;
   // The line number this symbol is at.
   unsigned LineNumber;
   // The low_pc for the dwarf label is taken from this symbol.
   MCSymbol *Label;
 
 public:
-  MCGenDwarfLabelEntry(StringRef name, unsigned fileNumber, unsigned lineNumber,
+  MCGenDwarfLabelEntry(StringRef name, StringRef linkageName,
+                       unsigned fileNumber, unsigned lineNumber,
                        MCSymbol *label)
-      : Name(name), FileNumber(fileNumber), LineNumber(lineNumber),
-        Label(label) {}
+      : Name(name), LinkageName(linkageName), FileNumber(fileNumber),
+        LineNumber(lineNumber), Label(label) {}
 
   StringRef getName() const { return Name; }
+  StringRef getLinkageName() const { return LinkageName; }
   unsigned getFileNumber() const { return FileNumber; }
   unsigned getLineNumber() const { return LineNumber; }
   MCSymbol *getLabel() const { return Label; }
 
   // This is called when label is created when we are generating dwarf for
   // assembly source files.
   static void Make(MCSymbol *Symbol, MCStreamer *MCOS, SourceMgr &SrcMgr,
                    SMLoc &Loc);

lib/MC/MCDwarf.cpp

static void EmitGenDwarfAbbrev(MCStreamer *MCOS) {
   EmitAbbrev(MCOS, dwarf::DW_AT_language, dwarf::DW_FORM_data2);
   EmitAbbrev(MCOS, 0, 0);
 
   // DW_TAG_label DIE abbrev (2).
   MCOS->EmitULEB128IntValue(2);
   MCOS->EmitULEB128IntValue(dwarf::DW_TAG_label);
   MCOS->EmitIntValue(dwarf::DW_CHILDREN_yes, 1);
   EmitAbbrev(MCOS, dwarf::DW_AT_name, dwarf::DW_FORM_string);
+  EmitAbbrev(MCOS, dwarf::DW_AT_linkage_name, dwarf::DW_FORM_string);
   EmitAbbrev(MCOS, dwarf::DW_AT_decl_file, dwarf::DW_FORM_data4);
   EmitAbbrev(MCOS, dwarf::DW_AT_decl_line, dwarf::DW_FORM_data4);
   EmitAbbrev(MCOS, dwarf::DW_AT_low_pc, dwarf::DW_FORM_addr);
   EmitAbbrev(MCOS, dwarf::DW_AT_prototyped, dwarf::DW_FORM_flag);
   EmitAbbrev(MCOS, 0, 0);
 
   // DW_TAG_unspecified_parameters DIE abbrev (3).
   MCOS->EmitULEB128IntValue(3);
static void EmitGenDwarfInfo(MCStreamer *MCOS,
   for (const auto &Entry : Entries) {
     // The DW_TAG_label DIE abbrev (2).
     MCOS->EmitULEB128IntValue(2);
 
     // AT_name, of the label without any leading underbar.
     MCOS->EmitBytes(Entry.getName());
     MCOS->EmitIntValue(0, 1); // NULL byte to terminate the string.
 
+    // AT_linkage_name - the actual symbol name
+    MCOS->EmitBytes(Entry.getLinkageName());
+    MCOS->EmitIntValue(0, 1); // NULL byte to terminate the string.
+
     // AT_decl_file, index into the file table.
     MCOS->EmitIntValue(Entry.getFileNumber(), 4);
 
     // AT_decl_line, source line number.
     MCOS->EmitIntValue(Entry.getLineNumber(), 4);
 
     // AT_low_pc, start address of the label.
     const MCExpr *AT_low_pc = MCSymbolRefExpr::create(Entry.getLabel(),
void MCGenDwarfLabelEntry::Make(MCSymbol *Symbol, MCStreamer *MCOS,
   // We won't create dwarf labels for symbols in sections that we are not
   // generating debug info for.
   if (!context.getGenDwarfSectionSyms().count(MCOS->getCurrentSectionOnly()))
     return;
 
   // The dwarf label's name does not have the symbol name's leading
   // underbar if any.
   StringRef Name = Symbol->getName();
+  StringRef LinkageName = Symbol->getName();
   if (Name.startswith("_"))
     Name = Name.substr(1, Name.size()-1);
 
   // Get the dwarf file number to be used for the dwarf label.
   unsigned FileNumber = context.getGenDwarfFileNumber();
 
   // Finding the line number is the expensive part which is why we just don't
   // pass it in as for some symbols we won't create a dwarf label.
   unsigned CurBuffer = SrcMgr.FindBufferContainingLoc(Loc);
   unsigned LineNumber = SrcMgr.FindLineNumber(Loc, CurBuffer);
 
   // We create a temporary symbol for use for the AT_high_pc and AT_low_pc
   // values so that they don't have things like an ARM thumb bit from the
   // original symbol. So when used they won't get a low bit set after
   // relocation.
   MCSymbol *Label = context.createTempSymbol();
   MCOS->EmitLabel(Label);
 
   // Create and entry for the info and add it to the other entries.
   MCOS->getContext().addMCGenDwarfLabelEntry(
-      MCGenDwarfLabelEntry(Name, FileNumber, LineNumber, Label));
+      MCGenDwarfLabelEntry(Name, LinkageName, FileNumber, LineNumber, Label));
 }
 
 static int getDataAlignmentFactor(MCStreamer &streamer) {
   MCContext &context = streamer.getContext();
   const MCAsmInfo *asmInfo = context.getAsmInfo();
   int size = asmInfo->getCalleeSaveStackSlotSize();
   if (asmInfo->isStackGrowthDirectionUp())
     return size;

test/MC/MachO/gen-dwarf.s

 // We don't check the file name as it is a temp directory
 // CHECK:    DW_AT_name [DW_FORM_string]
 // We don't check the DW_AT_comp_dir which is the current working directory
 // CHECK:    DW_AT_producer [DW_FORM_string]  ("llvm-mc (based on {{.*}})")
 // CHECK:    DW_AT_language [DW_FORM_data2] (DW_LANG_Mips_Assembler)
 
 // CHECK:    DW_TAG_label [2] *
 // CHECK:      DW_AT_name [DW_FORM_string]  ("bar")
+// CHECK:      DW_AT_linkage_name [DW_FORM_string]  ("_bar")
 // CHECK:      DW_AT_decl_file [DW_FORM_data4]  ([[FILE:".*gen-dwarf.s"]])
 // CHECK:      DW_AT_decl_line [DW_FORM_data4]  (5)
 // CHECK:      DW_AT_low_pc [DW_FORM_addr]  (0x0000000000000000)
 // CHECK:      DW_AT_prototyped [DW_FORM_flag]  (0x00)
 
 // CHECK:      DW_TAG_unspecified_parameters [3]  
 
 // CHECK:      NULL
 
 // CHECK:    DW_TAG_label [2] *
 // CHECK:      DW_AT_name [DW_FORM_string]  ("foo")
+// CHECK:      DW_AT_linkage_name [DW_FORM_string]  ("_foo")
 // CHECK:      DW_AT_decl_file [DW_FORM_data4]  ([[FILE]])
 // CHECK:      DW_AT_decl_line [DW_FORM_data4]  (9)
 // CHECK:      DW_AT_low_pc [DW_FORM_addr]  (0x0000000000000007)
 // CHECK:      DW_AT_prototyped [DW_FORM_flag]  (0x00)
 
 // CHECK:      DW_TAG_unspecified_parameters [3]  
 
 // CHECK:      NULL
 
 // CHECK:    DW_TAG_label [2] *
 // CHECK:      DW_AT_name [DW_FORM_string]  ("baz")
+// CHECK:      DW_AT_linkage_name [DW_FORM_string]  ("_baz")
 // CHECK:      DW_AT_decl_file [DW_FORM_data4]  ([[FILE]])
 // CHECK:      DW_AT_decl_line [DW_FORM_data4]  (10)
 // CHECK:      DW_AT_low_pc [DW_FORM_addr]  (0x0000000000000007)
 // CHECK:      DW_AT_prototyped [DW_FORM_flag]  (0x00)
 
 // CHECK:      DW_TAG_unspecified_parameters [3]  
 
 // CHECK:      NULL