Add support for Dwarf5 as emitted by gcc-11.2 #69
Conversation
Encapsulate dwarf-related PE sections into the PESection class. Remove some unused sections. Add helpers for common section operations. Move the context set via DIECursor::setContext to be static members of the class and add a debug context there. Add a standard method of enabling debug logging across the dwarf and PDB code.
DWARF5 has more contextual information that is associated with the compilation unit. As a preparation for using such information, carry it with the DIECursor and eliminate places where we're passing in the parent compilation unit. Also add the RDAddr helper to read a target-address according to the specification in the compilation unit.
Read the compilation unit header byte-by-byte rather than by casting the data to a structure. Add the currentBaseAddress contextual info to the compilation unit data. Move the LOCCursor into readDwarf.cpp. Implement a RangeCursor similar to the LOCCursor. Add more debug printing.
Handle the new DWARF5 compilation unit header and add new constants to dwarf.h. We still don't decode the new forms though.
Fill in Dwarf_InfoData for all DWARF5 forms excluding the new rnglists and loclists representations.
Add support for the new opcode-based format for location lists and range lists.
|
@dscho : This is the full dwarf5 PR. |
|
Thanks for your contribution. Now that the D compiler in the gcc suite will be uptodate with current D versions, this might also become interesting for the D community, again. I only had a cursory look so far, looks good. I'll have a closer look in the next couple of days. |
There was a problem hiding this comment.
Overall, it looks good.
I wish the "Refactor PESection" and the "more pre-DWARF5 refactoring" commits were split into more granular changes, to make it harder for bugs to hide.
The actual DWARF5 changes look good to me, but I've admittedly not gone through the corresponding parts of the DWARF5 specification to verify everything in detail.
| , debug_ranges(0), debug_ranges_length(0) | ||
| , codeSegment(0) | ||
| , linesSegment(-1) | ||
| , reloc(0), reloc_length(0) |
There was a problem hiding this comment.
As mentioned in #68 (review), I like to review PRs commit by commit, and if there is too much in one single commit, it is easy for bugs to hide. I did not find any bug in this, but I would not be surprised if I missed any because of the different goals of this commit: dropping debug_aranges and friends, refactoring PESections, adding new ones, dropping commented-out code, adding debugging, etc ;-)
There was a problem hiding this comment.
I understand your concern. I was mainly aiming here for a testable chunk which shouldn't introduce functional changes.
I have a different philosophy about code review: it's really only worthwhile for architectural analysis and for suggesting ways to improve the readability or understandability of code. For bugs, however, tests are the gold standard. Especially for code like this where an off-by-one error or subtle spec misreading can lead to wildly different results. I think we've seen over the decades that the "thousand eyes make bugs shallow" idea doesn't really work in practice.
src/readDwarf.h
Outdated
| DbgDwarfAttrRead = 0x200, | ||
| DbgDwarfLocLists = 0x400, | ||
| DbgDwarfRangeLists = 0x800, | ||
| DbgDwarfLines = 0x1000 |
There was a problem hiding this comment.
It would probably be a better idea to keep the original values and add the new ones to the end.
There was a problem hiding this comment.
I want to group the flags by theme, so all of the dwarf flags are next to each other, and all the pdb flags are next to each other too. In general, the higher numbered flags are much more verbose than the lower-numbeded flags.
The other thing to note about the DWARF debug printing is that it's meant to be used with the output of llvm-dwarfdump or objdump -W to see where we went wrong.
| PESection PEImage::* pSec; | ||
| }; | ||
|
|
||
| #define EXPANDSEC(name) constexpr SectionDescriptor sec_desc_##name { "." #name, &PEImage::name }; |
There was a problem hiding this comment.
cv2pdb is also compiled with Visual D, see https://ci.appveyor.com/project/rainers/visuald. The CI still uses VS2017, I'm not sure how well this works with constexpr. Can it be avoided?
There was a problem hiding this comment.
I just tested all the way back to VS2015 and this usage of constexpr is supported back there:
https://godbolt.org/z/349xjv44T
Do you still want me to remove it?
There was a problem hiding this comment.
Thanks for verifying that it works in older VS, too. No need to change it then.
Technically, this does not validate the DWARF5 parsing completely, but it looks for the `main` symbol to show up as expected when compiling with whatever GCC version MSYS2/Git for Windows uses (which is typically very close to the latest GCC version available). Signed-off-by: Johannes Schindelin <[email protected]>
Yes! Something like this should work: dscho@13a21e9 (it passed the build here)
Maybe we need to extend the GitHub workflow to also build with VS2017? The big problem here is that the VS2017 build agents will be removed in less than half a year: actions/runner-images#4312. |
I don' t think that's necessary. I'd rather update the appveyor build at some time or move the CI for Visual D to github, too. |
@neerajsi-msft how about fast-forwarding your branch to that commit? |
|
@dscho: Done. Thanks! |
|
Thanks! |
|
@rainers could I ask for a new tagged version with this? |
|
Available now: https://github.com/rainers/cv2pdb/releases/tag/v0.50 |
Thank you! |
This PR implements enough dwarf5 support to produce PDBs for git-for-windows.
Please see the individual commits for layered changes. I'd be happy to take another stab at re-layering if that would help drive this change to acceptance.