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

[Jason Molenda via Phabricator via lldb-commits]

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66250/new/

https://reviews.llvm.org/D66250