[PATCH] D134169: [LLD][COFF] Improve symbol table info for import thunk

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo accepted this revision.
mstorsjo added a comment.
This revision is now accepted and ready to land.

In D134169#3799064 <https://reviews.llvm.org/D134169#3799064>, @alvinhochun wrote:

> In D134169#3799044 <https://reviews.llvm.org/D134169#3799044>, @mstorsjo wrote:
>
>> My initial reaction here is that I wonder when you'd want to look at these symbols in "normal" application level debugging. But on the other hand, I presume we're already inconsistently adding some `__imp_` symbols to the symbol table, but not all of them, so maybe it's just for the best to add all of them. Additionally, if it makes things more consistent, and is useful for lower level debugging (like when figuring out which part of the toolchain is misbehaving), then it's probably good to add. So yeah, +1 from me to this too.
>
> It was more for the sake of completeness, and also because ld.bfd seems to do the same.
>
> The real reason which led me to add this symbol is: When I was using LLDB to inspect binaries without the import thunk being marked as functions, calls into the import thunk do not have any comments, and disassembling the import thunk shows a bogus label with symbol+offset (from another import symbol). Although marking the import thunk symbol as function already solves the biggest issue, it just annoys me knowing that LLDB would still show the wrong label for the jump inside the import thunk when not having the __imp_ symbol. (There is another way to fix this, which is to have LLDB load symbols from the import table, but that's more complicated.)

Oh, that's a great reason for adding them indeed. Yes, for stepping through things like thunks, having it show the right symbol name can be very valuable.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134169