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

Sun Sep 8 22:45:24 PDT 2019

MaskRay added a comment.

In D67053#1662315 <https://reviews.llvm.org/D67053#1662315>, @mstorsjo wrote:

> In D67053#1662274 <https://reviews.llvm.org/D67053#1662274>, @MaskRay wrote:
>
> > In D67053#1662244 <https://reviews.llvm.org/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.

================
Comment at: test/COFF/undefined-symbol-dwarf.s:109
+	.byte	1                       # Abbrev [1] 0xb:0xb0 DW_TAG_compile_unit
+	.byte   0                       # DW_AT_producer
+	.short	4                       # DW_AT_language
----------------
Convert tabs to spaces so they will align regardless of the tabstop setting (e.g. in phabricator it is nicely rendered)

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

https://reviews.llvm.org/D67053