[Lldb-commits] [PATCH] D15407: When stepping in/over source lines, combine the addr ranges of all line table entries w/ same line num

[Jim Ingham via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

This looks good.  I have one worry.  If the plan is stepping through a range of line 10, and that code branches to:

0x120 - line 0
0x128 - line 11
0x130 - line 12

You will see that we are starting at line 0, so you will choose 11 as the NEW line number to match, extend the range to 0x130 and then step through that.  So we would have effectively stepped through lines 10 and 11.  There are actually times the stepping will do this - for instance if you jump into the middle of another line we step out of the line for you, since being mid-source line is disconcerting.

It is important not to stop the stepping at line 0, since being "nowhere" is even more disconcerting.  So consuming the line 0 range is important.  But somewhere you need to add the notion of "expected completion line" so that you don't extend past an initial line 0 if the following line is not the expected one.


================
Comment at: include/lldb/Symbol/LineEntry.h:143-154
@@ -142,1 +142,14 @@
 
+    //------------------------------------------------------------------
+    /// Give the range for this LineEntry + any additional LineEntries for
+    /// this same source line that are contiguous.
+    ///
+    /// A compiler may emit multiple line entries for a single source line,
+    /// e.g. to indicate subexpressions at different columns.  This method
+    /// will get the AddressRange for all of the LineEntries for this source 
+    /// line that are contiguous.
+    ///
+    /// @return
+    ///     The contiguous AddressRange for this source line.
+    //------------------------------------------------------------------
+    AddressRange
----------------
Do you want to mention the treatment of line number 0 here?  You also glom those into the extended line range, which isn't obvious from the name or comment.

================
Comment at: source/Target/Thread.cpp:1546-1547
@@ +1545,4 @@
+{
+    return QueueThreadPlanForStepOverRange (abort_other_plans, line_entry.GetSameLineContiguousAddressRange(), addr_context, stop_other_threads, step_out_avoids_code_withoug_debug_info);
+}
+
----------------
This should get broken up across more lines.

================
Comment at: source/Target/Thread.cpp:1583-1584
@@ +1582,4 @@
+{
+    return QueueThreadPlanForStepInRange (abort_other_plans, line_entry.GetSameLineContiguousAddressRange(), addr_context, step_in_target, stop_other_threads, step_in_avoids_code_without_debug_info, step_out_avoids_code_without_debug_info);
+}
+
----------------
This line is too long.


Repository:
  rL LLVM

http://reviews.llvm.org/D15407