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

Yuanfang Chen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 9 14:51:28 PDT 2019


ychen added inline comments.


================
Comment at: COFF/SymbolTable.cpp:93
 
+static std::pair<StringRef, uint32_t> symbolizeFileLine(const SectionChunk *c,
+                                                        uint32_t addr) {
----------------
mstorsjo wrote:
> ychen wrote:
> > mstorsjo wrote:
> > > ychen wrote:
> > > > This funtionality could be in coff::getFileLine() ?
> > > > 
> > > > Also, I have not idea why `coff::getFileLine` is replying on `{"", 0}` and `ExitOnError` instead of `Expected<>`.
> > > > 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.
> > >>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.
> >>> 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`?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67053/new/

https://reviews.llvm.org/D67053





More information about the llvm-commits mailing list