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