[Lldb-commits] [PATCH] D124731: [lldb] Consider binary as module of last resort

Fri May 13 22:10:54 PDT 2022

hawkinsw marked 5 inline comments as done.
hawkinsw added inline comments.

================
Comment at: lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py:11
+    def get_address_from_symbol(self, symbol):
+        target = lldbutil.run_to_breakpoint_make_target(self, "a.out", True)
+        bp = target.BreakpointCreateByName(symbol, None)
----------------
JDevlieghere wrote:
> Can we reuse this target? If so you can store it as `self.target` and reuse it from `test_set_address_no_module`.
Thanks for the suggestion! I *wish* that we could reuse it (I know that instantiating multiple targets per test will increase the runtime), but I would prefer (?) to have them separate to guarantee the test is doing what we want. 

If we reused the target from this function when we ran the actual test, it would already have `a.out` loaded and, as a result, not trigger the actual behavior we want to test (setting a breakpoint when the program's "modules" have not already been loaded.). 

Does that make sense? Seem reasonable? 

================
Comment at: lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py:25
+        dbg.HandleCommand(f"break set -a {main_address:#x}")
+        self.assertTrue(tgt.GetNumBreakpoints() == 1)
+
----------------
JDevlieghere wrote:
> Our test harness has a bunch of helper functions that generate better error messages if the assertion fails. For example, here you could use `assertEqual(1, tgt.GetNumBreakpoints())`. If the assertion fails, it will print something like:
> 
> > tgt.GetNumBreakpoints() was expected to be 1 but was 2
> 
> which is much more informative than 
> 
> ? tgt.GetNumBreakpoints() == 1 was expected to be True but was False
Great suggestion! 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124731