[Lldb-commits] [PATCH] D50304: [lldb] Fix thread step until to not set breakpoint(s) on incorrect line numbers

[Venkata Ramanaiah Nalamothu via Phabricator via lldb-commits]

RamNalamothu added a comment.

Thank you.

In D50304#3608740 <https://reviews.llvm.org/D50304#3608740>, @jingham wrote:

> In D50304#3592849 <https://reviews.llvm.org/D50304#3592849>, @RamNalamothu wrote:
>
>> In D50304#3586710 <https://reviews.llvm.org/D50304#3586710>, @jingham wrote:
>>
>>> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50304/new/

https://reviews.llvm.org/D50304