Page MenuHomePhabricator

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

Authored by mstorsjo on Sep 1 2019, 2:34 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Sep 1 2019, 2:34 PM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added a subscriber: MaskRay.Sep 1 2019, 7:11 PM
MaskRay added inline comments.
test/COFF/undefined-symbol-dwarf.s
15 ↗(On Diff #218274)

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 marked an inline comment as done.Sep 1 2019, 10:52 PM
mstorsjo added inline comments.
test/COFF/undefined-symbol-dwarf.s
15 ↗(On Diff #218274)

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 updated this revision to Diff 218291.Sep 2 2019, 12:11 AM

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

ruiu added a comment.Sep 2 2019, 12:43 AM

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

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

Ping @MaskRay

MaskRay added inline comments.Sep 2 2019, 9:36 PM
test/COFF/undefined-symbol-dwarf.s
15 ↗(On Diff #218274)

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 marked an inline comment as done.Sep 3 2019, 5:42 AM
mstorsjo added inline comments.
test/COFF/undefined-symbol-dwarf.s
15 ↗(On Diff #218274)

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.

jdoerfert resigned from this revision.Sep 4 2019, 8:40 AM
mstorsjo marked an inline comment as done.Sep 7 2019, 2:05 PM
mstorsjo added inline comments.
test/COFF/undefined-symbol-dwarf.s
15 ↗(On Diff #218274)

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 updated this revision to Diff 219251.Sep 7 2019, 2:07 PM

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

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?

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

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 ↗(On Diff #219251)

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

mstorsjo marked an inline comment as done.Sep 8 2019, 11:19 PM

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 ↗(On Diff #219251)

Ok, will do

mstorsjo updated this revision to Diff 219289.Sep 9 2019, 12:16 AM
mstorsjo marked 3 inline comments as done.
mstorsjo edited the summary of this revision. (Show Details)

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 added inline comments.Sep 9 2019, 11:30 AM
COFF/SymbolTable.cpp
93 ↗(On Diff #219289)

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 marked an inline comment as done.Sep 9 2019, 11:47 AM
mstorsjo added inline comments.
COFF/SymbolTable.cpp
93 ↗(On Diff #219289)

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 added inline comments.Sep 9 2019, 1:06 PM
COFF/SymbolTable.cpp
93 ↗(On Diff #219289)

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 marked an inline comment as done.Sep 9 2019, 1:20 PM
mstorsjo added inline comments.
COFF/SymbolTable.cpp
93 ↗(On Diff #219289)

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 added inline comments.Sep 9 2019, 2:50 PM
COFF/SymbolTable.cpp
93 ↗(On Diff #219289)

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 updated this revision to Diff 219478.Sep 9 2019, 11:47 PM

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?

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

MaskRay accepted this revision.Sep 25 2019, 3:00 AM
This revision is now accepted and ready to land.Sep 25 2019, 3:00 AM
This revision was automatically updated to reflect the committed changes.