I have created https://reviews.llvm.org/D50304.
BTW, I almost forgot the fact that the error message changes were already covered in my other patch https://reviews.llvm.org/D48865. On Fri, Aug 3, 2018 at 11:41 PM, Jim Ingham <jing...@apple.com> wrote: > Thanks! I look forward to the patch. > > Jim > > > > On Aug 2, 2018, at 8:56 PM, Venkata Ramanaiah Nalamothu < > ramana.venka...@gmail.com> wrote: > > > > Thanks Jim for the elaborate reply. > > > > 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. > > > > 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. > > > > - Ramana > > > > On Thu, Aug 2, 2018 at 10:56 PM, Jim Ingham <jing...@apple.com> wrote: > > 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: > > > > for (uint32_t line_number : line_numbers) { > > uint32_t start_idx_ptr = index_ptr; > > while (start_idx_ptr <= end_ptr) { > > LineEntry line_entry; > > const bool exact = false; > > start_idx_ptr = sc.comp_unit->FindLineEntry( > > start_idx_ptr, line_number, sc.comp_unit, exact, > &line_entry); > > if (start_idx_ptr == UINT32_MAX) > > break; > > > > addr_t address = > > line_entry.range.GetBaseAddress(). > GetLoadAddress(target); > > if (address != LLDB_INVALID_ADDRESS) { > > if (fun_addr_range.ContainsLoadAddress(address, target)) > > address_list.push_back(address); > > else > > all_in_function = false; > > } > > start_idx_ptr++; > > } > > } > > > > The problem is that in the while loop we set: > > > > const bool exact = false; > > > > 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. > > > > 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". > > > > 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. > > > > 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. > > > > Jim > > > > > > > > > On Aug 1, 2018, at 10:22 PM, Ramana <ramana.venka...@gmail.com> wrote: > > > > > > > > > On Thu, Aug 2, 2018 at 3:32 AM, Jim Ingham <jing...@apple.com> wrote: > > > > > > > > > > On Jul 24, 2018, at 9:05 PM, Ramana via lldb-dev < > lldb-dev@lists.llvm.org> wrote: > > > > > > > > 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. > > > > > > > > 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. > > > > > > Why do you think this is incorrect? > > > > > > The requirements for "thread until <line number>" are: > > > > > > a) If any code contributed by <line number> is executed before leaving > the function, stop > > > b) If you end up leaving the function w/o triggering (a), then stop > > > > > > Understood and no concerns on this. > > > > > > Correct or incorrect should be determined by how well the > implementation fits those requirements. > > > > > > 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. > > > > > > 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. > > > > > > Besides setting a few more breakpoints - which should be pretty cheap > - I don't see much downside to the way it is currently implemented. > > > > > > Anyway, why did this bother you? > > > > > > Jim > > > > > > 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. > > > > > > File name Line number Starting address > > > ./kernels.cpp:[++] > > > kernels.cpp 9 0xacc74d00 > > > kernels.cpp 12 0xacc74d00 > > > > kernels.cpp 14 0xacc74d40 > > > kernels.cpp 13 0xacc74dc0 > > > kernels.cpp 14 0xacc74e00 > > > kernels.cpp 25 0xacc74e80 > > > kernels.cpp 25 0xacc74ec0 > > > kernels.cpp 26 0xacc74f00 > > > kernels.cpp 26 0xacc74f40 > > > kernels.cpp 26 0xacc74f80 > > > kernels.cpp 17 0xacc74fc0 > > > kernels.cpp 18 0xacc75000 > > > kernels.cpp 18 0xacc75040 > > > kernels.cpp 19 0xacc75080 > > > kernels.cpp 27 0xacc750c0 > > > kernels.cpp 27 0xacc75140 > > > kernels.cpp 28 0xacc75180 > > > kernels.cpp 28 0xacc751c0 > > > kernels.cpp 29 0xacc75200 > > > kernels.cpp 29 0xacc75240 > > > kernels.cpp 30 0xacc75280 > > > > > > 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. > > > > > > GDBRemoteCommunicationClient::SendGDBStoppointTypePacket() add at > addr = 0xacc75280 > > > Thread::PushPlan(0x0xa48b38f0): "Stepping from address 0xacc74d00 > until we reach one of: > > > 0xacc75000 (bp: -4) > > > 0xacc75040 (bp: -5) > > > 0xacc75080 (bp: -6) > > > 0xacc750c0 (bp: -7) > > > 0xacc75140 (bp: -8) > > > 0xacc75180 (bp: -9) > > > 0xacc751c0 (bp: -10) > > > 0xacc75200 (bp: -11) > > > 0xacc75240 (bp: -12) > > > 0xacc75280 (bp: -13) > > > > > > 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)? > > > > > > 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? > > > > > > > > > > > > > > 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. > > > > > > > > If I my above understanding is correct and after discussing here on > how to handle the scattered code scenario, I will submit a patch. > > > > > > > > Regards, > > > > Venkata Ramanaiah > > > > _______________________________________________ > > > > lldb-dev mailing list > > > > lldb-dev@lists.llvm.org > > > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev > > > > > > > > > <kernels.cpp> > > > > > >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
