I found a few cases where entries in the debug_line for a specific line of code have invalid entries (the address is outside of a code section or no section at all) and also valid entries. When this happens lldb might not set the breakpoint because the first line entry it will find in the line table might be the invalid one and since it's range is "invalid" no location is resolved. To get around this I changed the way we parse the line sequences to ignore those starting at an address under the first code segment.
Greg suggested to implement it this way so we don't need to check all sections for every line sequence.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
440 | I'm actually not 100% sure about this, can there be new code sections added after this? | |
lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-offsets.yaml | ||
520 ↗ | (On Diff #290049) | I generated this yaml with the code that is at the top, and then changed it to replicate the bug ,here's what the line table looks like: Address Line Column File ISA Discriminator Flags ------------------ ------ ------ ------ --- ------------- ------------- 0x0000000100000f80 1 0 1 0 0 is_stmt 0x0000000100000f84 2 5 1 0 0 is_stmt prologue_end 0x0000000100000f8b 2 5 1 0 0 is_stmt end_sequence 0x0000000100000f90 5 0 1 0 0 is_stmt 0x0000000100000f90 5 0 1 0 0 is_stmt end_sequence 0x000000000000000f 2 13 1 0 0 is_stmt prologue_end 0x0000000000000014 2 9 1 0 0 0x0000000000000017 3 12 1 0 0 is_stmt 0x000000000000001d 3 12 1 0 0 end_sequence Now that I think about it maybe I should also copy this into this file as a comment. |
lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-offsets.yaml | ||
---|---|---|
1 ↗ | (On Diff #290049) | I should rename this to test-invalid-addresses.yaml instead. |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
436–440 | This should be fine. Any symbol files that might get added after the fact really need to have matching sections or nothing will make sense. | |
1062 | Add a comment here about attempting to avoid line tables that should have been dead stripped. | |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h | ||
523 | A lengthy comment would be great here. Maybe something like: /// Many linkers will concatenate all available DWARF, even if parts of that DWARF /// should have been dead stripped. Some linkers will place tombstone values in for /// addresses that should have been dead stripped, with a value like -1 or -2. But many /// linkers will apply a relocation and attempt to set the value to zero. This is problematic /// in that we can end up with many addresses that are zero or close to zero due to there /// being an addend on the relocation whose base is at zero. Here we attempt to avoid /// addresses that are zero or close to zero by finding the first valid code address by looking /// at the sections and finding the first one that has read + execute permissions. This allows /// us to do a quick and cheap comparison against this address when parsing line tables and /// parsing functions where the DWARF should have been dead stripped. | |
lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-offsets.yaml | ||
520 ↗ | (On Diff #290049) | Would line table work just fine without your fix since the good address comes first? Or do we end up with multiple locations? I thought I remembered you thinking that it would pick the first one. Am I remembering correctly? |
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp | ||
74 ↗ | (On Diff #290049) | remove whitespace only changes please. Ditto for all whitespace only changes below. |
lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-offsets.yaml | ||
---|---|---|
520 ↗ | (On Diff #290049) | yeah, I was wrong, they end up being order by address, so the issue is there. |
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp | ||
---|---|---|
74 ↗ | (On Diff #290049) | Can I make an argument to allow this on diffs? Most repos have a no trailing whitespace rule and because of this most editors are configured to remote trailing whitespaces on save. I've also just checked this is the case for LLVM: https://llvm.org/docs/CodingStandards.html#whitespace I can either:
Doing 2) is pretty easy to me (but it also means everyone else has to do it), since LLVM code convention is to disallow trailing whitespaces I could split this into 2 diffs, one with the bug fix and another one with trailing whitespace removal only (so no one else has the same problem with this file in the future). I don't see what is the advantage of having 2 diffs (just more work) so that's why I'm making the case to allow removal of trailing whitespaces in the files of a diff. What do you think? |
This is fixing the same issue that I tried to fix with D84402, but then failed to get back to it when the discussion about the best approach started getting long. While I do have some reservations about this approach, it is definitive improvement than the status quo, so I won't start bikeshedding the design (but do see the inline comments)..
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
436 | This is really the _first_ text section (as listed in the section headers), is it not? But, I assume you really want the section with the lowest file address? The linkers (and other producers) will normally output the sections in ascending address order (because it's the simplest thing to do). However, I don't believe that is actually required by anything, and I /think/ it should be possible (with a moderately funky linker script) to produce a fully functional executable which does not have the sections listed in this order. | |
436–440 | Doing this in InitializeObject would be better. | |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h | ||
523 | To me, "should have been stripped" implies that the linkers are doing something wrong, as if they were required to parse and rewrite the dwarf in the input files. However, there is nothing in the DWARF or ELF specs that would support that, and the linkers are simply doing what the specs say, and what linkers have been doing since the dawn of time -- concatenate things. It would be better to just state the facts here, instead of passing judgement (or make an appearance of doing that). | |
lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-addresses.yaml | ||
1–8 ↗ | (On Diff #290112) | Is this the actual source code, or you've made some changes to it (like, to change the line table start address)? |
203–254 ↗ | (On Diff #290112) | I guess these (and debug_pub(types/names), and possible debug_aranges) are not really needed for this test. |
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp | ||
349–369 ↗ | (On Diff #290112) | I think this would be better as a shell test (if for nothing else, then because other line table tests are written that way). image lookup is pretty much a direct equivalent to ResolveSymbolContext, but its output is more readable, and its easier to fiddle with these kinds of tests. |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h | ||
---|---|---|
523 | ||
lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-addresses.yaml | ||
203–254 ↗ | (On Diff #290112) | I changed the line table by hand but forgot to add comments there. |
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp | ||
349–369 ↗ | (On Diff #290112) | I think what you describe is an end to end test but here I explicitly want to test the SymbolFileDWARF code because that's what I changed, it's more like a unit test. |
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp | ||
---|---|---|
349–369 ↗ | (On Diff #290112) | On most days I am a proponent of unit tests, so I feel slightly odd for writing this, but here it goes... I think you'll find that a lot of people in the llvm community (which now has a pretty big (but not as big as I'd like) intersection with the lldb community) are not very enthusiastic about (c++) unit tests. They concede they have their place, but are generally trying to avoid it. Some would rather create a brand new command line tool wrapping an api (and then test that via lit) than to write a unit test for it. Some effects of that can be seen in the design of the lldb-test tool. Although this approach, when taken to extremes, can be bad, I believe it has it's advantages, for several reasons:
Your points about the issues with lit tests are not untrue, but I wouldn't say they are more true for this test than they are for any other of our lit tests, so I don't see a need to treat this test specially. It's definitely true that the lit test will be more broad that this unit test, but it's still pretty far from a end-to-end test. A lot of our end-to-end tests for dwarf features start by compiling some c++ codes (hoping it produces the right dwarf), running the code, and then inspecting the running process. That's something I'm trying to change (not very successfully). Compared to that, "image lookup" is peanuts. Although you the fact that it's implemented by ResolveSymbolContext is kind of an implementation detail, I would say that if it was *not* implemented this way (and e.g. reimplemented some of that functionality) that we have bigger problems. |
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp | ||
---|---|---|
369 ↗ | (On Diff #290112) | I would rather deal with an C++ unit test any day. Trying to track down what set of convoluted command line commands reproduce some lit test is quite annoying and takes me a lot more time to debug. I think this test is targeted and tests what is needed. I would vote to keep this one over converting to a text dump test. My main reasoning is that it isn't possible to re-create a compilable test case that will survive any compiler that it used (past, present and future), and all symbol resolution is done bone using this call in all cases. When something goes wrong, very easy to compile the binary and debug. |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
436 | We do need to iterate over all sections and find the one with the lowest address, good catch. No sorting is required in object files. | |
440 | And doing the work in InitializeObject() is better. | |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h | ||
523 | Fine to reword this to not appear hostile toward the linkers. If any judgement should be passed, it should be on the DWARF format itself and how unfriendly it is to link! |
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp | ||
---|---|---|
369 ↗ | (On Diff #290112) | If we put this up for a vote, I think you'd be in the minority. :) I'm not sure what you find hard about reproducing a lit test -- the commands to do that get printed as a part of the test. And most of the time you don't need to run all the command to reproduce it -- running the last one suffices as the intermediate files are left over from the previous test run. I consider the leftover temporaries as one of the best aspects of this method. In this case, I could for example run llvm-dwarfdump on the intermediate object file to better understand the input that lldb gets. Note that I am not advocating changing the test input to c++ source. I think the yaml is just fine (if I was writing it, I would probably have made that an assembler file). I just meant changing the test method by prefixing the yaml with something like: # RUN: yaml2obj %s > %t # RUN: %lldb %t -b -o "image lookup -f main.cpp -l 2" | FileCheck %s # CHECK: LineEntry: {{.*}}main.cpp:2 # or something like that |
lldb/unittests/SymbolFile/DWARF/SymbolFileDWARFTests.cpp | ||
---|---|---|
369 ↗ | (On Diff #290112) |
If it is that easy, then sounds good. I was thinking you were asking him to add new functionality for dumping something. |
Addressed all comments:
- Moved initialization code to InitializeObject
- Moved to lit test
See inlined comments for full details.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
505 | Yes we need to check permissions. But we also have to watch out for sections that contain sections. For ELF, we have the PT_LOAD[N] sections that contain sections. On Mac, we have TEXT that contains sections, but the first section can be PAGEZERO (which has no read, no write, no execute). So the main question becomes: if a section contains subsections, do we ignore the top level section? I would prefer to see us do this. So this code should be factored into a lambda or function: bool SymbolFileDWARF::SetFirstCodeAddress(const SectionList §ion_list) { const uint32_t num_sections = section_list.GetSize(); for (uint32_t i = 0; i < num_sections; ++i) { SectionSP section_sp(section_list.GetSectionAtIndex(i)); if (sections_sp->GetChildren().GetSize() > 0) { SetFirstCodeAddress(sections_sp->GetChildren()); continue; } if (HasExecutePermissions(section_sp)) { const addr_t section_file_address = section_sp->GetFileAddress(); if (m_first_code_address > section_file_address) m_first_code_address = section_file_address; } } } And then this code will just look like: const SectionList *section_list = m_objfile_sp->GetModule()->GetSectionList(); if (section_list) SetFirstCodeAddress(*section_list); |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
505 | Sorry about this. I completely changed the original logic with my last update. I was checking the section type before and also going down the section hierarchy but not anymore. We should go through all sub sections just like I did in my original code, I was also checking if it was a code section, rather than permissions, it's a bit more abstract that way. |
lldb/test/Shell/SymbolFile/DWARF/line-entries-invalid-addresses.yaml | ||
---|---|---|
4 | This should be fine, this is what you get without the patch: (lldb) image lookup -f main.cpp -l 2 -v 1 match found in main.cpp:2 in /Users/aadsm/Projects/test-bad.obj: Address: () Summary: |
Just remove the braces as suggested in inline comments. Pavel?
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
502–504 | remove braces |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
502–504 | or even fold the declaration into the if condition. | |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h | ||
523 | How about this: DWARF does not provide a good way for traditional (concatenating) linkers to invalidate debug info describing dead-stripped code. These linkers will keep the debug info but resolve any addresses referring to such code as zero (BFD), a small positive integer (zero + relocation addend -- GOLD), or -1/-2 (LLD). Try to filter out this debug info by comparing it to the lowest code address in the module. | |
lldb/test/Shell/SymbolFile/DWARF/line-entries-invalid-addresses.yaml | ||
4 | True, but adding the check still wouldn't hurt, as one can imagine a bug that would cause the second main.cpp:2 entry (at address 0xf) to make its way here -- and that's what we're trying to avoid. |
- Fixed formatting issues
- Updated comment on the reason we need to do this
- Improved the lit test by also checking how many matches it found
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
502–504 | folding was not possible as clang-formatter didn't allow it. |
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | ||
---|---|---|
502–504 | I meant: if(const SectionList *section_list = m_objfile_sp->GetModule()->GetSectionList()) InitializeFirstCodeAddress(*section_list); We have plently of code like that, so clang-format should not have a problem. |
reverting because I'm not able to repro the fails, so need more time to figure it out.
A lengthy comment would be great here. Maybe something like: