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