Page MenuHomePhabricator

[lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers
ClosedPublic

Authored by RamNalamothu on Aug 5 2018, 3:53 AM.

Details

Summary

The requirements for "thread until <line number>" are:

a) If any code contributed by <line number> or the nearest subsequent of <line number> is executed before leaving the function, stop
b) If you end up leaving the function w/o triggering (a), then stop

In case of (a), since the <line number> may have multiple entries in the line table and the compiler might have scheduled/moved the relevant code across, and the lldb does not know the control flow, set breakpoints on all the line table entries of best match of <line number> i.e. exact or the nearest subsequent line.

Along with the above, currently, CommandObjectThreadUntil is also setting the breakpoints on all the subsequent line numbers after the best match and this latter part is wrong.

This issue is discussed at http://lists.llvm.org/pipermail/lldb-dev/2018-August/013979.html.

In fact, currently TestStepUntil.py is not actually testing step until scenarios and test_missing_one test fails without this patch if tests are made to run. Fixed the test as well.

Diff Detail

Event Timeline

ramana-nvr created this revision.Aug 5 2018, 3:53 AM

This implementation forces an extra lookup when the line number slides. For instance, if there were only one line match, but it was not exact, you're going to look up the exact address, fail, then look it up with exact = false.

Wouldn't it be more efficient to look with exact = false first, then reset the line you are looking for to whatever you got back, and continue with the exact=true search you do for the other line numbers.

ramana-nvr updated this revision to Diff 159517.Aug 7 2018, 8:41 AM

Yes, updated the patch accordingly.

jingham accepted this revision.Aug 7 2018, 10:04 AM

That looks good. Thanks for cleaning this up!

This revision is now accepted and ready to land.Aug 7 2018, 10:04 AM

I do not have the commit permission. Could someone help commit this patch?

davide added a subscriber: davide.Oct 3 2018, 10:33 PM

I'll commit this for you, but I might ask if you can try adding a test first?

RamNalamothu commandeered this revision.May 28 2022, 8:03 PM
RamNalamothu added a reviewer: ramana-nvr.
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
RamNalamothu retitled this revision from [lldb] CommandObjectThreadUntil should set breakpoint at either on exact or the nearest subsequent line number but not on all the subsequent line numbers to [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers.
RamNalamothu edited the summary of this revision. (Show Details)

Added the test coverage.

By the way, I am surprized that it looks like no else landed into this issue since the time I got into it during Aug/2018.

Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 10:04 AM

Does this look good now to land?

@jingham looks like you missed to notice this one as well.

For some reason I'm not getting mail notifications for review changes, sorry about that.

This is certainly better than the original implementation. Among other things, if we find an exact match, we really shouldn't be doing any more inexact matches, so after the first exact hit it should have switched exact to true.

But if you had line tables laid out like (this is in increasing order of address:

10
20
30
10
16

and you did "thread until 15", this would find the inexact match at 20, switch to an exact match for line 20 and find no other matches. But the gap between 10 & 16 in the line table is maybe an even more plausible place to put the line 15 until breakpoint, so maybe we did want to throw a breakpoint there as well?

Regular breakpoint setting has to move inexact breakpoints in much the same way. The code in the BreakpointResolverFileLine::SearchCallback ends up calling CompileUnit::ResolveSymbolContext to get the "best" inexact match. Maybe it would be better to not do this by hand here in the Until command, but reuse the code that we use to move breakpoints more generally?

For some reason I'm not getting mail notifications for review changes, sorry about that.

This is certainly better than the original implementation. Among other things, if we find an exact match, we really shouldn't be doing any more inexact matches, so after the first exact hit it should have switched exact to true.

But if you had line tables laid out like (this is in increasing order of address:

10
20
30
10
16

and you did "thread until 15", this would find the inexact match at 20, switch to an exact match for line 20 and find no other matches. But the gap between 10 & 16 in the line table is maybe an even more plausible place to put the line 15 until breakpoint, so maybe we did want to throw a breakpoint there as well?

@jingham
Nope, with the current patch, we would find the inexact match at 16.

Regular breakpoint setting has to move inexact breakpoints in much the same way. The code in the BreakpointResolverFileLine::SearchCallback ends up calling CompileUnit::ResolveSymbolContext to get the "best" inexact match. Maybe it would be better to not do this by hand here in the Until command, but reuse the code that we use to move break points more generally?

Be it CompileUnit::ResolveSymbolContext or CompileUnit::FindLineEntry, we end up calling LineTable::FindLineEntryIndexByFileIndex which finds "Exact match always wins. Otherwise try to find the closest line > the desired line." when we pass exact = false to it.
Given that and we anyway have to extract address list, from what we get out of CompileUnit::ResolveSymbolContext, for the thread until thread plan to work with, probably the additional overhead to use CompileUnit::ResolveSymbolContext here does not make sense. Or am I missing something?

@jingham

Sorry for spamming. But I am not sure if you have received my previous messages.

jingham accepted this revision.Jun 24 2022, 10:46 AM

For some reason I'm not getting mail notifications for review changes, sorry about that.

This is certainly better than the original implementation. Among other things, if we find an exact match, we really shouldn't be doing any more inexact matches, so after the first exact hit it should have switched exact to true.

But if you had line tables laid out like (this is in increasing order of address:

10
20
30
10
16

and you did "thread until 15", this would find the inexact match at 20, switch to an exact match for line 20 and find no other matches. But the gap between 10 & 16 in the line table is maybe an even more plausible place to put the line 15 until breakpoint, so maybe we did want to throw a breakpoint there as well?

@jingham
Nope, with the current patch, we would find the inexact match at 16.

Ah, my bad. FindLineEntry does end up in FindLineEntryByFileIndexImpl which does the closest match.

Regular breakpoint setting has to move inexact breakpoints in much the same way. The code in the BreakpointResolverFileLine::SearchCallback ends up calling CompileUnit::ResolveSymbolContext to get the "best" inexact match. Maybe it would be better to not do this by hand here in the Until command, but reuse the code that we use to move break points more generally?

Be it CompileUnit::ResolveSymbolContext or CompileUnit::FindLineEntry, we end up calling LineTable::FindLineEntryIndexByFileIndex which finds "Exact match always wins. Otherwise try to find the closest line > the desired line." when we pass exact = false to it.
Given that and we anyway have to extract address list, from what we get out of CompileUnit::ResolveSymbolContext, for the thread until thread plan to work with, probably the additional overhead to use CompileUnit::ResolveSymbolContext here does not make sense. Or am I missing something?

Finding these addresses happens once in response to a user command, so I highly doubt that the overhead matters one way or the other. Both end up doing the closest match (I misremembered where that was done) which is the main thing. ResolveSymbolContext does more work to handle inlined files, but there's no way to do "thread until" from a main CU into inlined source so that doesn't matter.

Thank you.

For some reason I'm not getting mail notifications for review changes, sorry about that.

This is certainly better than the original implementation. Among other things, if we find an exact match, we really shouldn't be doing any more inexact matches, so after the first exact hit it should have switched exact to true.

But if you had line tables laid out like (this is in increasing order of address:

10
20
30
10
16

and you did "thread until 15", this would find the inexact match at 20, switch to an exact match for line 20 and find no other matches. But the gap between 10 & 16 in the line table is maybe an even more plausible place to put the line 15 until breakpoint, so maybe we did want to throw a breakpoint there as well?

@jingham
Nope, with the current patch, we would find the inexact match at 16.

Ah, my bad. FindLineEntry does end up in FindLineEntryByFileIndexImpl which does the closest match.

Regular breakpoint setting has to move inexact breakpoints in much the same way. The code in the BreakpointResolverFileLine::SearchCallback ends up calling CompileUnit::ResolveSymbolContext to get the "best" inexact match. Maybe it would be better to not do this by hand here in the Until command, but reuse the code that we use to move break points more generally?

Be it CompileUnit::ResolveSymbolContext or CompileUnit::FindLineEntry, we end up calling LineTable::FindLineEntryIndexByFileIndex which finds "Exact match always wins. Otherwise try to find the closest line > the desired line." when we pass exact = false to it.
Given that and we anyway have to extract address list, from what we get out of CompileUnit::ResolveSymbolContext, for the thread until thread plan to work with, probably the additional overhead to use CompileUnit::ResolveSymbolContext here does not make sense. Or am I missing something?

Finding these addresses happens once in response to a user command, so I highly doubt that the overhead matters one way or the other. Both end up doing the closest match (I misremembered where that was done) which is the main thing. ResolveSymbolContext does more work to handle inlined files, but there's no way to do "thread until" from a main CU into inlined source so that doesn't matter.

Make sense.

FYI this caused a test breakage at lldb-aarch64-ubuntu, https://lab.llvm.org/buildbot/#/builders/96/builds/25128. I have reverted this in fe6db8d03ff16a65f57af24d2cb04f489e2e9b0c

FYI this caused a test breakage at lldb-aarch64-ubuntu, https://lab.llvm.org/buildbot/#/builders/96/builds/25128. I have reverted this in fe6db8d03ff16a65f57af24d2cb04f489e2e9b0c

Thank you.

Interestingly, the test is not failing for x86-64 and arm but fails for only aarch64. I don't have an `aarch64' system and need to find an alternative way to get access to the system.

The problem is most likely due to the arm64 version having slightly different line number sequences due to different instruction scheduling. You might be able to gain some insight just by comparing the debug line content, without actually running it.

It might be helpful to make the test more hermetic by removing the calls to printf (a library function coming from the environment).

RamNalamothu reopened this revision.Jul 1 2022, 9:50 AM
This revision is now accepted and ready to land.Jul 1 2022, 9:50 AM

Make the test case insensitive to compiler optimizations.

I am not fully versed on the patch, but generally speaking, if you're reasonably confident that your change fixed the origianl problem, you can resubmit the patch (and watch the bot for problems). If you still don't know what's the issue, then we may need to find someone with an arm machine.

then we may need to find someone with an arm machine.

I tried this patch on AArch64 and did not get any new failures. You can go ahead and reland. If there's still issues I can help fix them.

then we may need to find someone with an arm machine.

I tried this patch on AArch64 and did not get any new failures. You can go ahead and reland. If there's still issues I can help fix them.

Thank you very much @DavidSpickett.