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

[Yuanfang Chen via Phabricator via llvm-commits]

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:
> > 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.


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

https://reviews.llvm.org/D67053