[LLD] [COFF] Resolve source locations for undefined references using dwarf

[mstorsjo]

Summary

This fixes PR42407.

[MaskRay]

test/COFF/undefined-symbol-dwarf.s

16

clang -S output is verbose. Many directives in this file are not significant and they should be deleted to improve readability. The relocations are not resolved. Does that cause any problem?

[mstorsjo]

test/COFF/undefined-symbol-dwarf.s

16

clang -S output is verbose. Many directives in this file are not significant and they should be deleted to improve readability.

Yes, I guess I could trim out some. This is also built with -O0 to make sure the inline function ends up in a separate comdat (.section .text$_ZN1A5afuncEv below) to test that resolving references in different sections work, but I guess I could use some optimization with -fno-inline or something similar to keep them separate.

However, I'm a little weary of doing too much manual cleanups of it, if it would make the debug info mismatch the actual code. I'm not very proficient in DWARF so I don't know for sure exactly what needs to match what here, and which of the debug sections could be removed without breaking it.

The relocations are not resolved. Does that cause any problem?

Um, what do you mean? The errors about undefined symbols? That's the whole point of the test; to have undefined references, that we try to resolve back to source line numbers using the debug info.

[mstorsjo]

Shrunk the test file a little by generating it using -Os -fno-exceptions -fno-unwind-tables -fno-inline.

[ruiu]

Looks good but I'll defer to MaskRay to give an LGTM

[mstorsjo]

In D67053#1654236, @ruiu wrote:

Looks good but I'll defer to MaskRay to give an LGTM

Ping @MaskRay

[MaskRay]

test/COFF/undefined-symbol-dwarf.s

16

Can you take a look at test/ELF/debug-line-str.s and test/ELF/debug-line-obj.s? They are still a bit verbose in my opinion but is much conciser than this file..

.debug* sections (DWARF) in object files contains relocations (e.g. .secrel32 .Linfo_string1). They have to be resolved so that they can be symbolized by lib/DebugInfo/Symbolize. In the ELF port of lld, ELF/DWARF.cpp provides the relocation resolver functionality.

Does COFF need a counterpart? To be honest I know very little about COFF. I'll make some investigation into this. Meanwhile, can you test this patch on some larger programs, DLLs with multiple object files, etc?

[mstorsjo]

test/COFF/undefined-symbol-dwarf.s

16

There are a few directives left I could strip out, and apparently it's ok to remove most of the .debug_str section, and some comments. With that I don't think I would be too far off from the ELF debug-line tests.

My test case has two text sections, just like debug-line-obj.s, but I guess I could try to transcribe that one to COFF for a smaller testcase of the same situation.

I tested it on a larg DLL (with lots of object files) and seems to work fine there as well. (I don't see how the number of object files would matter though as only one object file is given to Symbolizer at a time.)

However it doesn't seem to work with debug info generated by GCC, so I guess I need to look into what's differing there.

The libObject RelocationResolver.cpp seems to be involved to some part for resolving relocations at least, but I'm not sure if that's enough for handling it in general, or if something like what is done for ELF is needed here.

[mstorsjo]

test/COFF/undefined-symbol-dwarf.s

16

However it doesn't seem to work with debug info generated by GCC, so I guess I need to look into what's differing there.

This turned out to just be me missing to pass the mingw flag to the linker when I tested it; it seems to work just as well with dwarf debug info from GCC.

.debug* sections (DWARF) in object files contains relocations (e.g. .secrel32 .Linfo_string1).
They have to be resolved so that they can be symbolized by lib/DebugInfo/Symbolize

As far as I can see when reading through the code, this is supposed to just work, thanks to getRelocationResolver(Obj), which for COFF handles .secrel32 relocations.

[mstorsjo]

Reduced the size of the test case by manually cutting out unneeded parts, similar to some of the ELF tests.

[MaskRay]

In D67053#1662244, @mstorsjo wrote:

Reduced the size of the test case by manually cutting out unneeded parts, similar to some of the ELF tests.

The updated test looks good now. Have you had success with larger executables now?

[mstorsjo]

In D67053#1662274, @MaskRay wrote:

In D67053#1662244, @mstorsjo wrote:

Reduced the size of the test case by manually cutting out unneeded parts, similar to some of the ELF tests.

The updated test looks good now. Have you had success with larger executables now?

Yes, it works fine in large test cases as well.

Do you have an opinion on whether I should keep the LLVMSymbolizer object around between calls?

And would it be better to use symbolizeInlinedCode instead of symbolizeCode? (That requires a small patch to the symbolizer, to allow that one to operate on an in-memory object file.)

[MaskRay]

In D67053#1662315, @mstorsjo wrote:

In D67053#1662274, @MaskRay wrote:

In D67053#1662244, @mstorsjo wrote:

Reduced the size of the test case by manually cutting out unneeded parts, similar to some of the ELF tests.

The updated test looks good now. Have you had success with larger executables now?

Yes, it works fine in large test cases as well.

Nice.

Do you have an opinion on whether I should keep the LLVMSymbolizer object around between calls?

This is the error path and we don't expect this code to be called many times so its contribution to the overall time may be negligible. Keep the same LLVMSymbolizer will benefit when you symbolize the same object multiple times. If reusing LLVMSymbolizer does not take too many lines, you can do that.

And would it be better to use symbolizeInlinedCode instead of symbolizeCode? (That requires a small patch to the symbolizer, to allow that one to operate on an in-memory object file.)

In the diagnostics I think we just need one line, so symbolizeCode should be sufficient. symbolizeInlinedCode can give you multiple frames but I'm not sure how to pretty print them.

test/COFF/undefined-symbol-dwarf.s

109

Convert tabs to spaces so they will align regardless of the tabstop setting (e.g. in phabricator it is nicely rendered)

[mstorsjo]

In D67053#1662604, @MaskRay wrote:

In D67053#1662315, @mstorsjo wrote:

Do you have an opinion on whether I should keep the LLVMSymbolizer object around between calls?

This is the error path and we don't expect this code to be called many times so its contribution to the overall time may be negligible. Keep the same LLVMSymbolizer will benefit when you symbolize the same object multiple times. If reusing LLVMSymbolizer does not take too many lines, you can do that.

@ruiu - Do you have any suggestion on where to store it, to have it reused in multiple calls here? I could add it as an std::unique_ptr in the config object, but I think that might end up requiring including the symbolizer header in the config header.

And would it be better to use symbolizeInlinedCode instead of symbolizeCode? (That requires a small patch to the symbolizer, to allow that one to operate on an in-memory object file.)

In the diagnostics I think we just need one line, so symbolizeCode should be sufficient. symbolizeInlinedCode can give you multiple frames but I'm not sure how to pretty print them.

Ok, thanks for the info!

test/COFF/undefined-symbol-dwarf.s

109

Ok, will do

[mstorsjo]

Reduced the test case further, converted it to use spaces instead of tabs. (A few manual edits had given some parts get mixed indentation in the middle of lines before.) Keeping the symbolizer around in the global config object (without std::unique_ptr, as it's allocated by the arena allocator with make<LLVMSymbolizer>()).

[ychen]

COFF/SymbolTable.cpp

93

This funtionality could be in coff::getFileLine() ?

Also, I have not idea why coff::getFileLine is replying on {"", 0} and ExitOnError instead of Expected<>.

[mstorsjo]

COFF/SymbolTable.cpp

93

This funtionality could be in coff::getFileLine() ?

Well, despite the file name, it should maybe be named getPDBFileLine() or something like that; it's defined in PDB.cpp and specific to that format.

Also, I have not idea why coff::getFileLine is replying on {"", 0} and ExitOnError instead of Expected<>.

That's a good point, but also feels orthogonal to this patch.

[ychen]

COFF/SymbolTable.cpp

93

This funtionality could be in coff::getFileLine() ?

Well, despite the file name, it should maybe be named getPDBFileLine() or something like that; it's defined in PDB.cpp and specific to that format.

I'm not sure if coff::getFileLine is meant to be PDB specific since symbolizing a COFF file should be debug-format agnostic and getSymbolLocations is not in a PDB specific path in 3108802f1696a3635d7e1edcc3fbe523381ed5df. It more looks like a missing case for ELF.
And it does look odd to be in PDB.h if we add ELF support.

Also, I have not idea why coff::getFileLine is replying on {"", 0} and ExitOnError instead of Expected<>.

That's a good point, but also feels orthogonal to this patch.

Agree. Considering it is just a LLD internal API, it is not a major issue.

[mstorsjo]

COFF/SymbolTable.cpp

93

This funtionality could be in coff::getFileLine() ?

Well, despite the file name, it should maybe be named getPDBFileLine() or something like that; it's defined in PDB.cpp and specific to that format.

I'm not sure if coff::getFileLine is meant to be PDB specific since symbolizing a COFF file should be debug-format agnostic and getSymbolLocations is not in a PDB specific path in 3108802f1696a3635d7e1edcc3fbe523381ed5df. It more looks like a missing case for ELF.
And it does look odd to be in PDB.h if we add ELF support.

Um, what? Add ELF support for what, where?

Currently the COFF linker in lld only supports symbolizing with PDB debug info. This patch adds support for DWARF debug info as well.

Maybe it would be clearer if this patch also renamed getFileLine to getPDBFileLine (that function is not part of any public API anywhere, it's just an internal function within the lld COFF linker), changed the name of the new symbolizeFileLine to getDwarfFileLine and added a wrapper frontend getFileLine which will call getPDBFileLine and getDwarfFileLine?

[ychen]

COFF/SymbolTable.cpp

93

Sorry for the typo. I meant DWARF.

Maybe it would be clearer if this patch also renamed getFileLine to getPDBFileLine (that function is not part of any public API anywhere, it's just an internal function within the lld COFF linker), changed the name of the new symbolizeFileLine to getDwarfFileLine and added a wrapper frontend getFileLine which will call getPDBFileLine and getDwarfFileLine?

This sounds great to me.

[mstorsjo]

Renamed the existing functions to clarify that they operate on codeview debug info only.

@ruiu - as @MaskRay is ok with the form of the test case now, can you give this the formal approval?

[mstorsjo]

Ping @ruiu

[mstorsjo]

In D67053#1665676, @mstorsjo wrote:

Ping @ruiu

Ping again

[mstorsjo]

@ruiu @MaskRay - can either of you approve this one? @ruiu earlier said it was ok with him, and deferred the review to @MaskRay, who hasn't commented further after I fixed what was suggested before.

COFF/CMakeLists.txt

add_lld_library(lldCOFF
   DebugInfoMSF
   DebugInfoPDB
   LibDriver
   LTO
   MC
   Object
   Option
   Support
+  Symbolize
   WindowsManifest
 
   LINK_LIBS
   lldCommon
   ${LLVM_PTHREAD_LIB}
 
   DEPENDS
   COFFOptionsTableGen
   ${tablegen_deps}
   )

COFF/SymbolTable.cpp

 #include "Config.h"
 #include "Driver.h"
 #include "LTO.h"
 #include "PDB.h"
 #include "Symbols.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Timer.h"
+#include "llvm/DebugInfo/Symbolize/Symbolize.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/Object/WindowsMachineFlag.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include <utility>
 
 using namespace llvm;
 
static std::vector<std::string> getSymbolLocations(BitcodeFile *file) {
   std::string res("\n>>> referenced by ");
   StringRef source = file->obj->getSourceFileName();
   if (!source.empty())
     res += source.str() + "\n>>>               ";
   res += toString(file);
   return {res};
 }
 
+static std::pair<StringRef, uint32_t> symbolizeFileLine(const SectionChunk *c,
+                                                        uint32_t addr) {
+  symbolize::LLVMSymbolizer symbolizer;
+  Expected<DILineInfo> expectedLineInfo = symbolizer.symbolizeCode(
+      *c->file->getCOFFObj(), {addr, c->getSectionNumber() - 1});
+  if (!expectedLineInfo)
+    return {"", 0};
+  const DILineInfo &lineInfo = *expectedLineInfo;
+  if (lineInfo.FileName == DILineInfo::BadString)
+    return {"", 0};
+  return {saver.save(lineInfo.FileName), lineInfo.Line};
+}
+
 // Given a file and the index of a symbol in that file, returns a description
 // of all references to that symbol from that file. If no debug information is
 // available, returns just the name of the file, else one string per actual
 // reference as described in the debug info.
 std::vector<std::string> getSymbolLocations(ObjFile *file, uint32_t symIndex) {
   struct Location {
     Symbol *sym;
     std::pair<StringRef, uint32_t> fileLine;
   };
   std::vector<Location> locations;
 
   for (Chunk *c : file->getChunks()) {
     auto *sc = dyn_cast<SectionChunk>(c);
     if (!sc)
       continue;
     for (const coff_relocation &r : sc->getRelocs()) {
       if (r.SymbolTableIndex != symIndex)
         continue;
       std::pair<StringRef, uint32_t> fileLine =
           getFileLine(sc, r.VirtualAddress);
+      if (fileLine.first.empty() && config->mingw)
+        fileLine = symbolizeFileLine(sc, r.VirtualAddress);
       Symbol *sym = getSymbol(sc, r.VirtualAddress);
       if (!fileLine.first.empty() || sym)
         locations.push_back({sym, fileLine});
     }
   }
 
   if (locations.empty())
     return std::vector<std::string>({"\n>>> referenced by " + toString(file)});

test/COFF/undefined-symbol-dwarf.s

+# REQUIRES: x86
+# RUN: llvm-mc -triple=x86_64-windows-gnu -filetype=obj -o %t.o %s
+# RUN: not lld-link /lldmingw /out:%t.exe %t.o /entry:entry 2>&1 | FileCheck %s
+
+# CHECK: error: undefined symbol: bar()
+# CHECK-NEXT: >>> referenced by /path/to/src/undef.cpp:17
+# CHECK-NEXT: >>>               {{.*}}.o:(entry)
+# CHECK-EMPTY:
+# CHECK-NEXT: error: undefined symbol: foo()
+# CHECK-NEXT: >>> referenced by /path/to/src/undef.cpp:7
+# CHECK-NEXT: >>>               {{.*}}.o:(A::afunc())
+# CHECK-EMPTY:
+# CHECK-NEXT: error: undefined symbol: baz
+# CHECK-NEXT: >>> referenced by {{.*}}.o:(myptr)
+
+  .text
+  .file "undef.cpp"
+  .file 1 "/path/to/src" "undef.cpp"
+  .globl  entry                   # -- Begin function entry
+entry:                                  # @entry
+.Lfunc_begin0:
+  .loc  1 14 0                  # undef.cpp:14:0
+  subq  $40, %rsp
+.Ltmp0:
+  leaq  32(%rsp), %rcx
+.Ltmp1:
+  .loc  1 16 4 prologue_end     # undef.cpp:16:4
+  callq _ZN1A5afuncEv
+  .loc  1 17 2                  # undef.cpp:17:2
+  callq _Z3barv
+  .loc  1 18 1                  # undef.cpp:18:1
+  addq  $40, %rsp
+  retq
+.Ltmp2:
+.Lfunc_end0:
+
+  .def   _ZN1A5afuncEv;
+  .scl  2;
+  .type 32;
+  .endef
+  .section  .text$_ZN1A5afuncEv,"xr",discard,_ZN1A5afuncEv
+  .globl  _ZN1A5afuncEv           # -- Begin function _ZN1A5afuncEv
+  .p2align  1, 0x90
+_ZN1A5afuncEv:                          # @_ZN1A5afuncEv
+.Lfunc_begin1:
+  .loc  1 6 0                   # undef.cpp:6:0
+  .loc  1 7 3 prologue_end      # undef.cpp:7:3
+  jmp _Z3foov                 # TAILCALL
+.Ltmp3:
+.Lfunc_end1:
+
+  .data
+  .globl  myptr                   # @myptr
+  .p2align  3
+myptr:
+  .quad baz
+
+  .section  .debug_str,"dr"
+.Linfo_string0:
+  .asciz  "myptr"                 # string offset=0
+  .section  .debug_abbrev,"dr"
+.Lsection_abbrev:
+  .byte 1                       # Abbreviation Code
+  .byte 17                      # DW_TAG_compile_unit
+  .byte 1                       # DW_CHILDREN_yes
+  .byte 37                      # DW_AT_producer
+  .byte 37                      # DW_FORM_strx1
+  .byte 19                      # DW_AT_language
+  .byte 5                       # DW_FORM_data2
+  .byte 3                       # DW_AT_name
+  .byte 37                      # DW_FORM_strx1
+  .byte 16                      # DW_AT_stmt_list
+  .byte 23                      # DW_FORM_sec_offset
+  .byte 27                      # DW_AT_comp_dir
+  .byte 37                      # DW_FORM_strx1
+  .byte 17                      # DW_AT_low_pc
+  .byte 1                       # DW_FORM_addr
+  .byte 85                      # DW_AT_ranges
+  .byte 23                      # DW_FORM_sec_offset
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 2                       # Abbreviation Code
+  .byte 52                      # DW_TAG_variable
+  .byte 0                       # DW_CHILDREN_no
+  .byte 3                       # DW_AT_name
+  .byte 14                      # DW_FORM_strp
+  .byte 73                      # DW_AT_type
+  .byte 19                      # DW_FORM_ref4
+  .byte 63                      # DW_AT_external
+  .byte 25                      # DW_FORM_flag_present
+  .byte 58                      # DW_AT_decl_file
+  .byte 11                      # DW_FORM_data1
+  .byte 59                      # DW_AT_decl_line
+  .byte 11                      # DW_FORM_data1
+  .byte 2                       # DW_AT_location
+  .byte 24                      # DW_FORM_exprloc
+  .byte 0                       # EOM(1)
+  .byte 0                       # EOM(2)
+  .byte 0                       # EOM(3)
+  .section  .debug_info,"dr"
+.Lsection_info:
+.Lcu_begin0:
+  .long .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+  .short  4                       # DWARF version number
+  .secrel32 .Lsection_abbrev # Offset Into Abbrev. Section
+  .byte 8                       # Address Size (in bytes)
+  .byte 1                       # Abbrev [1] 0xb:0xb0 DW_TAG_compile_unit
+  .byte   0                       # DW_AT_producer
+  .short  4                       # DW_AT_language
+  .byte   0                       # DW_AT_name
+  .secrel32 .Lline_table_start0 # DW_AT_stmt_list
+  .byte   0                       # DW_AT_comp_dir
+  .quad 0                       # DW_AT_low_pc
+  .secrel32 .Ldebug_ranges0 # DW_AT_ranges
+  .byte 2                       # Abbrev [2] 0x2a:0x15 DW_TAG_variable
+  .secrel32 .Linfo_string0  # DW_AT_name
+  .long 63                      # DW_AT_type
+                                        # DW_AT_external
+  .byte 1                       # DW_AT_decl_file
+  .byte 12                      # DW_AT_decl_line
+  .byte 9                       # DW_AT_location
+  .byte 3
+  .quad myptr
+  .byte 0                       # End Of Children Mark
+.Ldebug_info_end0:
+  .section  .debug_ranges,"dr"
+.Ldebug_range:
+.Ldebug_ranges0:
+  .quad .Lfunc_begin0
+  .quad .Lfunc_end0
+  .quad .Lfunc_begin1
+  .quad .Lfunc_end1
+  .quad 0
+  .quad 0
+
+  .section  .debug_line,"dr"
+.Lline_table_start0: