[DWARF5][COFF] Fix wrong relocation in .debug_line #83773
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-debuginfo Author: None (timoh-ba) ChangesDwarf 5 allows separating filenames from .debug_line into a separate .debug_line_str section. The strings are referenced relative to the start of the .debug_line_str section. Previously, on COFF, the relocation information instead caused offsets to be relocated to the base address of the COFF-File. This lead to wrong offsets in linked COFF files which caused the debugger to be unable to find the correct source files. This patch fixes this problem by making the offsets relative to the start of the .debug_line_str section instead. There should be no changes for ELF-Files as everything seems to be working there. I'm not sure if we should add a test case for this and I don't know how you'd write one - the problem only occurs in linked .dll-files containing dwarf 5 debugging information. The wrong offsets in the files causes missing file names of the source files there. I tested this change manually and it seems to work for dlls and not break elf files, though someone with more experience in llvm should probably check if this the right way of achieving this. Full diff: https://github.com/llvm/llvm-project/pull/83773.diff 1 Files Affected:
diff --git a/llvm/lib/MC/MCDwarf.cpp b/llvm/lib/MC/MCDwarf.cpp
index d0face9140de66..2ee0c3eb27b92e 100644
--- a/llvm/lib/MC/MCDwarf.cpp
+++ b/llvm/lib/MC/MCDwarf.cpp
@@ -360,7 +360,12 @@ void MCDwarfLineStr::emitRef(MCStreamer *MCOS, StringRef Path) {
size_t Offset = addString(Path);
if (UseRelocs) {
MCContext &Ctx = MCOS->getContext();
- MCOS->emitValue(makeStartPlusIntExpr(Ctx, *LineStrLabel, Offset), RefSize);
+ if (Ctx.getAsmInfo()->needsDwarfSectionOffsetDirective()) {
+ MCOS->emitCOFFSecRel32(LineStrLabel, Offset);
+ } else {
+ MCOS->emitValue(makeStartPlusIntExpr(Ctx, *LineStrLabel, Offset),
+ RefSize);
+ }
} else
MCOS->emitIntValue(Offset, RefSize);
}
|
|
I don't know enough about the COFF side of DWARF (maybe @zmodem would have some pointers to folks more versed in this) - but as for a test case. I'd expect some sort of objdump tool on the resulting COFF Object and observing the different kind of relocation would be suitable. (if I were reviewing this in more detail (other reviewers might already be familiar with the context and not need this explained) I'd want to know more/understand why the old code is still in a fallback path/how the code is valid in that case & what that case ( |
|
+@mstorsjo for DWARF-in-COFF |
|
I'm not very familiar with DWARF overall, or what DWARF 5 brings, but some extra context: When the overall switch to DWARF 5 was made, the Windows targets stuck to DWARF 4, see 9c62728 and 6cf64b2. From what I remember, neither GDB nor LLDB worked with the DWARF 5 output. I guess this fix is the missing key here, although from what I remember, the breakage might have been bigger than just not finding the right souce files. It'd be interesting to know if GDB and LLDB work with our DWARF 5 output after this change. We've had to do similar fixes for COFF before, for using the right kind of relative relocations - see e.g. 47046f0. Thus, my take on this is that the fix sounds reasonable and plausible, but I don't have all the nitty gritty details in mind right now. |
That is actually a good question, I based this commit around other uses of The other code is still in fallback because it seems to be the path for ELF files and everything was working there when I tested it so I didn't want to break anything. Maybe we can always use emitCOFFSecRel32, but the function name suggests that it won't work on ELF :).
There is an overview at https://dwarfstd.org/dwarf5std.html which lists the major & breaking changes, one of which being:
Which can be seen in MCDwarfLineTableHeader::Emit calling MCDwarfLineTableHeader::emitV5FileDirTables for version 5. This calls MCDwarfLineStr::emitRef which is where I put my change. Edit: I just researched ELF files a bit and it seems like all relocations for shared objects are always section-relative, which explains why everything is working there; PE files have absolute & relative relocations though and we emitted an absolute one previously. |
|
This needs a test. https://maskray.me/blog/2021-08-08-toolchain-testing#i-dont-know-where-to-add-a-test
If you are still unsure, edit the description to include some C example so that folks can suggest how to craft a test. I believe the issue (object file format related) is better demonstrated as code. |
Ah, fair enough - if there's similar code elsewhere I'm more or less happy to hand-wave/not think about it too much harder. But yeah, some test coverage would be good. |
|
FYI, I tested this patch in my rudimentary llvm-mingw smoke test setup for LLDB, and there are at least other issues present still after this (not sure if they are on the LLDB side or code generation side though). I have a small test script that does a couple simple tests and verifies that things generally work: https://github.com/mstorsjo/llvm-mingw/blob/master/run-lldb-tests.sh To test the behaviour, I applied a patch that switched Clang to use DWARF 5 for the MinGW target: mstorsjo/llvm-mingw@2b41137 That obviously causes these tests to fail: https://github.com/mstorsjo/llvm-mingw/actions/runs/8169849153/job/22344878316 To try out this PR, I added this patch on top of the previous one, mstorsjo/llvm-mingw@7dae451, and reran the tests, but they still fail in the same way: https://github.com/mstorsjo/llvm-mingw/actions/runs/8169885454/job/22345201730 |
You're right, some tests would be good, I'll add one; though it will probably early next week when I'll get around to it. If you really wanted to test this you'd have to compile code, link it & inspect the dwarf information - though with LLVM's test system it's probably easier to just verify that the relocations in the object file are correct. What I don't like about that is that it assumes that section-relative relocations are indeed the correct choice here instead of verifying that they are. We can test if we emit section relative relocs, but not if section relative relocs are in fact correct.
That's a pity, I haven't tested it with lldb, just looked at the dwarf information using llvm-dwarfdump & libdwarfs dwarfdump and it looked good. |
Indeed, but that's how most of LLVMs tests work - they test one detail in isolation (which allows catching regressions in simple tests on any platform by just executing these code generation tools). Details like whether the big picture works or not, which often requires a full working toolchain, is left for higher level integration tests, as those normally are harder to run (requiring to have suitable target headers and libraries etc, and be running on the right platform above all). |
|
We do have cross-project-tests as a place to put that kind of end-to-end test. |
Dwarf 5 allows separating filenames from .debug_line into a separate .debug_line_str section. The strings are referenced relative to the start of the .debug_line_str section. Previously, on COFF, the relocation information instead caused offsets to be relocated to the base address of the COFF-File. This lead to wrong offsets in linked COFF files which caused the debugger to be unable to find the correct source files. This patch fixes this problem by making the offsets relative to the start of the .debug_line_str section instead. There should be no changes for ELF-Files as everything seems to be working there. A test is also added to ensure that the correct relocation entries are emitted. See also llvm#83773
timoh-ba
force-pushed
the
timoh/fix_dwarf_line_str_reloc
branch
from
d6357ee
to
37570d0
Compare
|
I added a small test to verify that llvm emits the correct relocation entries. As this is my first time writing a llvm test, I wasn't 100% sure what I was doing - please verify that this correct and tell if its not :) |
llvm/test/MC/COFF/dwarf5lineinfo.s
Outdated
| .Linfo_string: | ||
| .Linfo_string1: | ||
| .secrel32 .Linfo_string1 |
There was a problem hiding this comment.
Do you need these 3 lines? (They don't look like they contribute to the relocations, but I don't know how COFF works.)
There was a problem hiding this comment.
You're right, I tried around a bit and I can actually remove a lot more lines, will push the changes now
Dwarf 5 allows separating filenames from .debug_line into a separate .debug_line_str section. The strings are referenced relative to the start of the .debug_line_str section. Previously, on COFF, the relocation information instead caused offsets to be relocated to the base address of the COFF-File. This lead to wrong offsets in linked COFF files which caused the debugger to be unable to find the correct source files. This patch fixes this problem by making the offsets relative to the start of the .debug_line_str section instead. There should be no changes for ELF-Files as everything seems to be working there. A test is also added to ensure that the correct relocation entries are emitted. See also llvm#83773
timoh-ba
force-pushed
the
timoh/fix_dwarf_line_str_reloc
branch
from
37570d0
to
2709aff
Compare
|
ping Can this be merged now? Or is there anything left I should change before? |
|
|
||
| main: | ||
| .file 0 "/" "test.c" | ||
| .loc 0 1 0 |
There was a problem hiding this comment.
Just to be clear - if I understand correctly, the distinction between DWARF 4 and 5 gets activated by these directives here, with DWARF 4, I see that the first parameter to these directives is 1 instead of 0. Is that right?
There was a problem hiding this comment.
Well, you're asking a really good question there. I tested around a bit and you seem to be correct about the difference between dwarf4 & dwarf5 here. I tried to find some llvm documentation about this but couldn't find anything, I'll probably need to look through the assembly parser to find out what each of those fields mean? I got the assembly out of clang, I haven't written it myself.
There was a problem hiding this comment.
DWARF 5 defined file number 0 as the main source file. DWARF 4 doesn't have that, file numbers start at 1 and there isn't a distinguished "main" file. LMK if you need more detail.
There was a problem hiding this comment.
Thank you, very interesting
Dwarf 5 allows separating filenames from .debug_line into a separate .debug_line_str section. The strings are referenced relative to the start of the .debug_line_str section. Previously, on COFF, the relocation information instead caused offsets to be relocated to the base address of the COFF-File. This lead to wrong offsets in linked COFF (PE) files which caused the debugger to be unable to find the correct source files. This patch fixes this problem by making the offsets relative to the start of the .debug_line_str section instead. There should be no changes for ELF-Files as everything seems to be working there. A test is also added to ensure that the correct relocation entries are emitted. See also llvm#83773
timoh-ba
force-pushed
the
timoh/fix_dwarf_line_str_reloc
branch
from
2709aff
to
926be17
Compare
|
Sidenote: I also changed the commit message title to better reflect the change |
There was a problem hiding this comment.
FWIW, I'm ok with this change, so let's approve it. If nobody else has any objections to it, we can probably merge it tomorrow or so.
Still, as mentioned elsewhere, DWARF5 doesn't work properly for full debugging on mingw, but I guess this is one incremental step towards making it work.
|
@timoh-ba Btw, before merging: Please turn off Keep my email addresses private setting in your account. See LLVM Discourse for more information. |
Okay, I turned it off, I hope it worked? Also I noticed that I linked this PR using its github link, from the git log of llvm I see that some people do it that way, some tag it using the number and a #. Should I change it or is this not important? |
Yes, seems so. Thanks!
If the PR is merged via the web UI, the PR number gets added automatically at the end of the subject line (although I can edit it before it gets finalized). If some people merge their PRs by pushing their commits manually, they might link to the PR review discussion manually by mentioning it in the commit message. But our PRs are merged via "squash and merge", and we have it configured to default the commit message to the PR subject + initial comment. So if updating the commit message, it's good to update the PR subject and description as well, so it gets right in the end. In this case, I guess the message from the commit is the most up to date and correct one? I can copypaste it from there when merging it, and I can leave out redundant manual reference to the PR. |
Yes it is 😄
Yes, that would be perfect. The PR subject - and original commit message - were more related to the effect of the change (fix it) than to what changed (make relocs section relative), that's why I changed it. I think the latter is more important for a reader. |
|
@timoh-ba Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…m#83773) Dwarf 5 allows separating filenames from .debug_line into a separate .debug_line_str section. The strings are referenced relative to the start of the .debug_line_str section. Previously, on COFF, the relocation information instead caused offsets to be relocated to the base address of the COFF-File. This lead to wrong offsets in linked COFF (PE) files which caused the debugger to be unable to find the correct source files. This patch fixes this problem by making the offsets relative to the start of the .debug_line_str section instead. There should be no changes for ELF-Files as everything seems to be working there. A test is also added to ensure that the correct relocation entries are emitted.
Dwarf 5 allows separating filenames from .debug_line into a separate .debug_line_str section. The strings are referenced relative to the start of the .debug_line_str section. Previously, on COFF, the relocation information instead caused offsets to be relocated to the base address of the COFF-File. This lead to wrong offsets in linked COFF files which caused the debugger to be unable to find the correct source files.
This patch fixes this problem by making the offsets relative to the start of the .debug_line_str section instead. There should be no changes for ELF-Files as everything seems to be working there.
I'm not sure if we should add a test case for this and I don't know how you'd write one - the problem only occurs in linked .dll-files containing dwarf 5 debugging information. The wrong offsets in the files causes missing file names of the source files there. I tested this change manually and it seems to work for dlls and not break elf files, though someone with more experience in llvm should probably check if this the right way of achieving this.