<div dir="ltr"><div>I have created <a href="https://reviews.llvm.org/D50304">https://reviews.llvm.org/D50304</a>.</div><div><br></div><div>BTW, I almost forgot the fact that the error message changes were already covered in my other patch <a href="https://reviews.llvm.org/D48865">https://reviews.llvm.org/D48865</a>.<br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 3, 2018 at 11:41 PM, Jim Ingham <span dir="ltr"><<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks! I look forward to the patch.<br>
<span class="HOEnZb"><font color="#888888"><br>
Jim<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
> On Aug 2, 2018, at 8:56 PM, Venkata Ramanaiah Nalamothu <<a href="mailto:ramana.venkat83@gmail.com">ramana.venkat83@gmail.com</a>> wrote:<br>
> <br>
> Thanks Jim for the elaborate reply.<br>
> <br>
> I knew what is happening in below piece of code and also has a patch ready but now needs a bit of fine tuning based on your below comments. I just wanted to hear from you folks that my understanding is correct and I am not missing something.<br>
> <br>
> Also, the code around it needs modification of error messages to refer to 'thread->GetID()' instead of 'm_options.m_thread_idx'. I will create a review on phabricator with all these changes.<br>
> <br>
> - Ramana<br>
> <br>
> On Thu, Aug 2, 2018 at 10:56 PM, Jim Ingham <<a href="mailto:jingham@apple.com">jingham@apple.com</a>> wrote:<br>
> That's a bug. The code in CommandObjectThreadUntil needs to be a little more careful about sliding the line number to the "nearest source line that has code." It currently does:<br>
> <br>
> for (uint32_t line_number : line_numbers) {<br>
> uint32_t start_idx_ptr = index_ptr;<br>
> while (start_idx_ptr <= end_ptr) {<br>
> LineEntry line_entry;<br>
> const bool exact = false;<br>
> start_idx_ptr = sc.comp_unit->FindLineEntry(<br>
> start_idx_ptr, line_number, sc.comp_unit, exact, &line_entry);<br>
> if (start_idx_ptr == UINT32_MAX)<br>
> break;<br>
> <br>
> addr_t address =<br>
> line_entry.range.<wbr>GetBaseAddress().<wbr>GetLoadAddress(target);<br>
> if (address != LLDB_INVALID_ADDRESS) {<br>
> if (fun_addr_range.<wbr>ContainsLoadAddress(address, target))<br>
> address_list.push_back(<wbr>address);<br>
> else<br>
> all_in_function = false;<br>
> }<br>
> start_idx_ptr++;<br>
> }<br>
> }<br>
> <br>
> The problem is that in the while loop we set:<br>
> <br>
> const bool exact = false;<br>
> <br>
> Having exact == false through the whole loop means that all the other line table entries after the last exact match will match because from the index ptr on there are no more entries, so we slide it to the next one.<br>
> <br>
> In general it's not always easy to tell which lines will actually contribute code so lldb is lax about line number matching, and slides the breakpoint to the nearest subsequent line that has code when there's no exact match. That seems a good principle to follow here as well. So I don't want to just set exact to "true".<br>
> <br>
> I think the better behavior is to try FindLineEntry the first time with exact = false. If that picks up a different line from the one given, reset the line we are looking for to the found line. In either case, then go on to search for all the other instances of that line with exact = false. It might actually be handy to have another function on CompUnit like FindAllEntriesForLine that bundles up this behavior as it seems a generally useful one.<br>
> <br>
> If you want to give this a try, please do. Otherwise, file a bug and I'll get to it when I have a chance.<br>
> <br>
> Jim<br>
> <br>
> <br>
> <br>
> > On Aug 1, 2018, at 10:22 PM, Ramana <<a href="mailto:ramana.venkat83@gmail.com">ramana.venkat83@gmail.com</a>> wrote:<br>
> > <br>
> > <br>
> > On Thu, Aug 2, 2018 at 3:32 AM, Jim Ingham <<a href="mailto:jingham@apple.com">jingham@apple.com</a>> wrote:<br>
> > <br>
> > <br>
> > > On Jul 24, 2018, at 9:05 PM, Ramana via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org">lldb-dev@lists.llvm.org</a>> wrote:<br>
> > > <br>
> > > On the subject line, the ToT lldb (see code around CommandObjectThread.cpp:1230) sets the breakpoint on the first exact matching line of 'line-number' or the closest line number > 'line-number' i.e. the best match.<br>
> > > <br>
> > > And along with that, starting from the above exact/best matching line number index in the line table, the breakpoints are also being set on every other line number available in the line table in the current function scope. This latter part, I believe, is incorrect.<br>
> > <br>
> > Why do you think this is incorrect? <br>
> > <br>
> > The requirements for "thread until <line number>" are:<br>
> > <br>
> > a) If any code contributed by <line number> is executed before leaving the function, stop<br>
> > b) If you end up leaving the function w/o triggering (a), then stop<br>
> > <br>
> > Understood and no concerns on this.<br>
> > <br>
> > Correct or incorrect should be determined by how well the implementation fits those requirements.<br>
> > <br>
> > There isn't currently a reliable indication from the debug information or line tables that "line N will always be entered starting with the block at 0x123". So you can't tell without doing control flow analysis, which if any of the separate entries in the line table for the same line will get hit in the course of executing the function. So the safest thing to do is to set breakpoints on them all. <br>
> > <br>
> > From the above, I understand that we have to do this when the debug line table has more than one entry for a particular source line. And this is what I referred to as "machine code for one single source line is scattered across" in my previous mail. Thanks for sharing why we had to do that.<br>
> > <br>
> > Besides setting a few more breakpoints - which should be pretty cheap - I don't see much downside to the way it is currently implemented.<br>
> > <br>
> > Anyway, why did this bother you?<br>
> > <br>
> > Jim<br>
> > <br>
> > However, I am concerned about the below 'thread until' behaviour. For the attached test case (kernels.cpp - OpenCL code), following is the debug line table generated by the compiler.<br>
> > <br>
> > File name Line number Starting address<br>
> > ./kernels.cpp:[++]<br>
> > kernels.cpp 9 0xacc74d00<br>
> > kernels.cpp 12 0xacc74d00 <br>
> > kernels.cpp 14 0xacc74d40<br>
> > kernels.cpp 13 0xacc74dc0<br>
> > kernels.cpp 14 0xacc74e00<br>
> > kernels.cpp 25 0xacc74e80<br>
> > kernels.cpp 25 0xacc74ec0<br>
> > kernels.cpp 26 0xacc74f00<br>
> > kernels.cpp 26 0xacc74f40<br>
> > kernels.cpp 26 0xacc74f80<br>
> > kernels.cpp 17 0xacc74fc0<br>
> > kernels.cpp 18 0xacc75000<br>
> > kernels.cpp 18 0xacc75040<br>
> > kernels.cpp 19 0xacc75080<br>
> > kernels.cpp 27 0xacc750c0<br>
> > kernels.cpp 27 0xacc75140<br>
> > kernels.cpp 28 0xacc75180<br>
> > kernels.cpp 28 0xacc751c0<br>
> > kernels.cpp 29 0xacc75200<br>
> > kernels.cpp 29 0xacc75240<br>
> > kernels.cpp 30 0xacc75280<br>
> > <br>
> > With the ToT lldb, when I am at line 12 (0xacc74d00), if I say 'thread until 18', the lldb log gives me the following w.r.t breakpoints.<br>
> > <br>
> > GDBRemoteCommunicationClient::<wbr>SendGDBStoppointTypePacket() add at addr = 0xacc75280<br>
> > Thread::PushPlan(0x0xa48b38f0)<wbr>: "Stepping from address 0xacc74d00 until we reach one of:<br>
> > 0xacc75000 (bp: -4)<br>
> > 0xacc75040 (bp: -5)<br>
> > 0xacc75080 (bp: -6)<br>
> > 0xacc750c0 (bp: -7)<br>
> > 0xacc75140 (bp: -8)<br>
> > 0xacc75180 (bp: -9)<br>
> > 0xacc751c0 (bp: -10)<br>
> > 0xacc75200 (bp: -11)<br>
> > 0xacc75240 (bp: -12)<br>
> > 0xacc75280 (bp: -13)<br>
> > <br>
> > Setting two breakpoints for line number 18 i.e. at 0xacc75000 and 0xacc75040 is understandable from your above reasoning and since we are anyway setting a breakpoint at the end of the function (line 30 - 0xacc75280), is it necessary to set the breakpoints on line numbers 19, 27, 28, 29 as well i.e. at 0xacc75080 (line 19), 0xacc750c0 (line 27), 0xacc75140 (line 27), 0xacc75180 (line 28), 0xacc751c0 (line 28), 0xacc75200 (line 29), 0xacc75240 (line 29)? <br>
> > <br>
> > The latter part i.e. setting breakpoints on 19, 27, 28, 29 as well is what I think is incorrect. Am I missing something here?<br>
> > <br>
> > <br>
> > > <br>
> > > What, I think, should happen is we just set only one breakpoint on the first exact/best match for the given 'line-number' and another on the return address from the current frame. And this leaves us with one special case where the machine code for one single source line is scattered across (aka scheduled across) and I do not know what is the expected behaviour in this case.<br>
> > > <br>
> > > If I my above understanding is correct and after discussing here on how to handle the scattered code scenario, I will submit a patch.<br>
> > > <br>
> > > Regards,<br>
> > > Venkata Ramanaiah<br>
> > > ______________________________<wbr>_________________<br>
> > > lldb-dev mailing list<br>
> > > <a href="mailto:lldb-dev@lists.llvm.org">lldb-dev@lists.llvm.org</a><br>
> > > <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/lldb-dev</a><br>
> > <br>
> > <br>
> > <kernels.cpp><br>
> <br>
> <br>
<br>
</div></div></blockquote></div><br></div>