[Lldb-commits] [PATCH] D52851: Adding support to step into the callable wrapped by libc++ std::function

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Just a couple of trivial requests, mostly about comments...



================
Comment at: packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:27-28
+
+    @add_test_categories(["libc++"])
+    def test(self):
+        """Test that std::function as defined by libc++ is correctly printed by LLDB"""
----------------
davide wrote:
> Is this affected by the debug info variant used? (i.e. dSYM/gmodules/DWARF/dwo). 
> If not, this could be a `NO_DEBUG_INFO` test
I don't think this test will be sensitive to those differences, so this probably can be a NO_DEBUG_INFO test.


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:45-61
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        break_in_main = target.BreakpointCreateBySourceRegex('// Set break point at this line.', self.main_source_spec)
+        self.assertTrue(break_in_main, VALID_BREAKPOINT)
+
+        self.process = target.LaunchSimple(
----------------
Most of this setup (excepting the line_number calls) can be done in one line using lldbutils.run_to_source_breakpoint.


================
Comment at: packages/Python/lldbsuite/test/lang/cpp/std-function-step-into-callable/TestStdFunctionStepIntoCallable.py:64
+        self.thread.StepInto()
+        self.assertEqual( self.thread.GetFrameAtIndex(0).GetLineEntry().GetLine(), self.source_foo_line ) ;
+        self.process.Continue()
----------------
Can you also check the source file?  The way libcxx does inlining nowadays, all these std::function calls introduce lots of inlining so the chance that you got to the right line but in an inlined function is not zero...


================
Comment at: source/Target/CPPLanguageRuntime.cpp:295-301
+  // Handling the case where we are attempting to step into std::function.
+  // Currently due to the the:
+  //
+  //    target.process.thread.step-avoid-regexp
+  //
+  // setting we will currenly step right out of standard library code.
+  //
----------------
I'm not sure this comment is helpful.  You want to do the trampoline detection regardless of the value of the regexp.  And we really aren't violating that contract, since we don't intent to STOP on step into std::function...


================
Comment at: source/Target/CPPLanguageRuntime.cpp:329
+      return ret_plan_sp;
+    } else {
+      ret_plan_sp.reset(new ThreadPlanStepInRange(thread, range_of_curr_func,
----------------
You need to explain what this step in is about.  Otherwise this is pretty mysterious.


https://reviews.llvm.org/D52851