Page MenuHomePhabricator

Check if debug line sequences are starting after the first code segment
ClosedPublic

Authored by aadsm on Sep 4 2020, 4:58 PM.

Details

Summary

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.

Diff Detail

Event Timeline

aadsm created this revision.Sep 4 2020, 4:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2020, 4:58 PM
aadsm requested review of this revision.Sep 4 2020, 4:58 PM
aadsm added inline comments.Sep 4 2020, 5:00 PM
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.

aadsm added inline comments.
lldb/unittests/SymbolFile/DWARF/Inputs/test-invalid-offsets.yaml
1 ↗(On Diff #290049)

I should rename this to test-invalid-addresses.yaml instead.

clayborg requested changes to this revision.Sep 4 2020, 5:22 PM
clayborg added inline comments.
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.

This revision now requires changes to proceed.Sep 4 2020, 5:22 PM
aadsm added inline comments.Sep 4 2020, 6:33 PM
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.

aadsm added inline comments.Sep 5 2020, 4:23 PM
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:

  1. git add -i (which is a pain), or
  2. configure my editor to not remove trailing white spaces for this project (which I guess it should be fine since clang-format takes care of it in the end)

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?

aadsm updated this revision to Diff 290112.Sep 5 2020, 4:53 PM

Address comments

clayborg accepted this revision.Sep 14 2020, 3:38 PM
This revision is now accepted and ready to land.Sep 14 2020, 3:38 PM

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.

aadsm added inline comments.Sep 20 2020, 2:48 PM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
523

@clayborg can you clarify this comment like @labath suggests?

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.
I would prefer to stay away from an end to end test for this particular change because 1) knowing that image lookup calls ResolveSymbolContext is an implementation detail 2) The output of image lookup could change over time, 3) Other bugs unrelated to this could be introduced that will make image lookup show a different content as well and fail the test.

labath added inline comments.Sep 29 2020, 6:59 AM
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:

  • changing the test does not require recompilation
  • due to more people using it, more people are aware of how these tests work (easier for them to inspect/modify it)
  • for similar reasons, the infrastructure supporting these tests is better. E.g., for the lit test, I can choose whether to generate the test binary via yaml2obj, llvm-mc, clang, or something completely different. unit tests only have yaml2obj available and for a long time it was implemented by shelling out to the external binary.

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.

clayborg added inline comments.Sep 29 2020, 10:41 AM
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.

clayborg added inline comments.Sep 29 2020, 10:46 AM
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!

labath added inline comments.Sep 30 2020, 7:37 AM
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
clayborg added inline comments.Sep 30 2020, 2:36 PM
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

If it is that easy, then sounds good. I was thinking you were asking him to add new functionality for dumping something.

aadsm updated this revision to Diff 298906.Oct 18 2020, 4:47 PM

Addressed all comments:

  • Moved initialization code to InitializeObject
  • Moved to lit test
labath added inline comments.Oct 19 2020, 3:01 AM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
505

Shouldn't this be also checking the permissions?

lldb/test/Shell/SymbolFile/DWARF/line-entries-invalid-addresses.yaml
4

I think you also need to check for the "1 matches found", otherwise this will succeed even without the patch.

clayborg requested changes to this revision.Oct 19 2020, 3:13 PM

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 &section_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);
This revision now requires changes to proceed.Oct 19 2020, 3:13 PM
aadsm added inline comments.Oct 19 2020, 3:29 PM
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.

aadsm added inline comments.Oct 22 2020, 6:10 PM
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:
aadsm updated this revision to Diff 300131.Oct 22 2020, 6:20 PM

Check all child sections and makes sure the section is an actual code section.

clayborg accepted this revision.Oct 26 2020, 2:23 PM

Just remove the braces as suggested in inline comments. Pavel?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
502–504

remove braces

This revision is now accepted and ready to land.Oct 26 2020, 2:23 PM
labath accepted this revision.Oct 27 2020, 2:55 AM
labath added inline comments.
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.

aadsm updated this revision to Diff 301450.Oct 28 2020, 2:59 PM
  • 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
aadsm added inline comments.Nov 1 2020, 8:46 AM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
502–504

folding was not possible as clang-formatter didn't allow it.

labath added inline comments.Nov 2 2020, 1:01 AM
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.

aadsm added a comment.Nov 9 2020, 11:46 AM

reverting because I'm not able to repro the fails, so need more time to figure it out.