[Lldb-commits] [PATCH] D66250: [JIT][Unwinder] Add Trampoline ObjectFile and UnwindPlan support for FCB

Jason Molenda via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Wed Aug 14 19:17:22 PDT 2019

jasonmolenda added a comment.

Overall this changeset looks good to me.  A few small comments.

Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:142
+        expected_fn_name = "$__lldb_jitted_conditional_bp_trampoline"
+        self.assertTrue(frame1 and frame1.GetFunctionName() == expected_fn_name)
Personal style thing, but I would do these as two separate tests -- test that frame1.IsValid() and test that frame1.GetFunctionName returns the expected result.  So when the test fails, you know which is the problem just from the line number.

Comment at: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/fast_conditional_breakpoints/TestFastConditionalBreakpoints.py:159
+        #self.assertEqual(var.GetValue(), "9")
     def inject_invalid_fast_conditional_breakpoint(self):
Is this an SB API problem, or a more basic lldb problem?  If you did 'frame variable local_count' in this frame, would you get 9 or a different value?

Comment at: lldb/source/Symbol/UnwindPlan.cpp:161
+    s.PutChar('=');
+    s.Printf("%#llx (const-value)", m_location.const_value);
+    break;
We tend to do "0x%" PRIx64 " (const-value)", .... in this kind of instance.

Comment at: lldb/source/Target/ABI.cpp:47
+lldb::ModuleSP ABI::PrepareUnwinding(lldb::addr_t address, std::size_t size,
+                                     lldb::addr_t return_address) {
I think we can probably come up with a more descriptive name for this.  ABI::CreateModuleForFastConditionalBreakpointTrampoline ?  Not wedded to that name, but we should give a little more idea of what this method is doing.  Right now, I wouldn't be able to figure it out unless I read the log/error messages.

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list