[lldb] Consider binary as module of last resort

[hawkinsw]

Summary

When setting an address breakpoint using a non-section address
in lldb before having ever run the program, the binary itself
is not considered a module. As a result, the breakpoint is
unresolved (and never gets resolved subsequently).

This patch changes that behavior: as a last resort, the binary
is considered as a module when resolving a non-section address
breakpoint.

[hawkinsw]

First, a thank you from a long-time gdb user who has finally seen the light and working to switch to lldb! The work that you all do to build an awesome, modern debugger is amazing.

As for this patch, I have committed a few typo fixes in the past, but this is my first code submission. I hope that I am following proper procedures and that this is a useful improvement.

Please let me know if I have done anything wrong and/or how to make this submission better. Thanks again!

[JDevlieghere]

This looks reasonable to me, but I'm not sure if there's anything special about the executable module that would result in us doing the wrong thing. I'm sure Jim or Pavel will know.

[jingham]

This seems like a reasonable fallback, and the implementation looks fine.
You need to add a test case setting an address breakpoint in the executable and making sure that works. Should be easy, set a line breakpoint in the main executable, get the resolved address and set an address breakpoint with that and make sure that breakpoint got a location.
You should also add some words to the "break set" command help saying that this is the fallback behavior.

[hawkinsw]

In D124731#3494609, @jingham wrote:

This seems like a reasonable fallback, and the implementation looks fine.
You need to add a test case setting an address breakpoint in the executable and making sure that works. Should be easy, set a line breakpoint in the main executable, get the resolved address and set an address breakpoint with that and make sure that breakpoint got a location.
You should also add some words to the "break set" command help saying that this is the fallback behavior.

Thank you for the feedback! I will make those changes!

[hawkinsw]

Updated patch including responses to helpful feedback from @jingham.

[hawkinsw]

(for real) Updating the patch based on helpful feedback from @jingham.

[hawkinsw]

In D124731#3498195, @hawkinsw wrote:

(for real) Updating the patch based on helpful feedback from @jingham.

I used to know better how to make sure that phabricator notified people that the commit was updated based on their feedback, but I've since forgotten! Sorry! I hope that you didn't get spammed @jingham. Thanks again for the helpful review! It was fun to get to learn the unit-testing system. I hope that the test is acceptable!

[hawkinsw]

Sorry to bother you all (@jingham, @JDevlieghere and @labath), but I just wondered if there was anything else that I could do to spruce this up! I want to make sure that it meets everyone's expectations! It's been awesome to work on this submission!

Will

[hawkinsw]

Hello @labath and @JDevlieghere -- I hope that your Friday is going well! I'd love to make any other changes to this if necessary! Thanks for everything!

[JDevlieghere]

lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py

11 ↗

(On Diff #427794)

Can we reuse this target? If so you can store it as self.target and reuse it from test_set_address_no_module.

21–22 ↗

(On Diff #427794)

Consistency nit: I don't think we use tgt anywhere. We do use dbg but only for the global debugger stored in self.dbg. I'd use debugger and target instead to avoid confusion.

25 ↗

(On Diff #427794)

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

31 ↗

(On Diff #427794)

34 ↗

(On Diff #427794)

36 ↗

(On Diff #427794)

[hawkinsw]

lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py

11 ↗

(On Diff #427794)

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?

25 ↗

(On Diff #427794)

Great suggestion!

[hawkinsw]

Updating the patchset according to the helpful feedback from @JDevlieghere.

[hawkinsw]

@JDevlieghere Thanks again for the feedback. I updated the patchset according to your comments. I left open one of your suggestions to get your feedback.

Thanks again!
Will

[hawkinsw]

Just wanted to see if there was anything else I could do to help make this patch acceptable, @JDevlieghere and @jingham . Thanks again for all your help!

[JDevlieghere]

Still a few places where we have a dedicated assert

lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py

25 ↗

(On Diff #429411)

28 ↗

(On Diff #429411)

[hawkinsw]

Stupidly missed a few of the places where bare assertions were used.

[hawkinsw]

@JDevlieghere So sorry! I cannot believe I missed those. Thanks for putting up with me!

[JDevlieghere]

No worries, thank you for your patience. This LGTM.

[JDevlieghere]

Please let me know if you need someone to land this on your behalf.

[hawkinsw]

In D124731#3528269, @JDevlieghere wrote:

Please let me know if you need someone to land this on your behalf.

Unfortunately, I do! Sorry! I have not yet completed the committer process. I still feel too new. Thanks for the mentorship on this patch!

[hawkinsw]

I am going to investigate why the buildbot failed. When I ran the tests locally everything ran fine. I am terribly, terribly sorry!

@JDevlieghere -- If you could help me debug the failure, or point me to the documentation on how to read buildbot output, that would be great!

Sorry again!

[hawkinsw]

I realize the reason for the failure. It would appear that there are tests that are otherwise affected by the change in the behavior introduced here. I will take care of updating these tests as quickly as possible. I am sorry for the trouble, @JDevlieghere !

[omjavaid]

I have reverted this temporarily as It broke LLDB API test TestBadAddressBreakpoints.py

[hawkinsw]

In D124731#3530692, @omjavaid wrote:

I have reverted this temporarily as It broke LLDB API test TestBadAddressBreakpoints.py

@omjavaid We have updated that test so that this should no longer cause a problem. I am sorry to say that I don't know the proper procedure for reopening this. Should I open a separate request? Thank you for your help!

Will

[DavidSpickett]

(I also keep an eye on the bots Omair was referring to) You can just reland this change as is (revert your revert) and if something else goes wrong you'll either get a failure email or we'll let you know.

lldb/source/Breakpoint/BreakpointResolverAddress.cpp

 
 Searcher::CallbackReturn BreakpointResolverAddress::SearchCallback(
     SearchFilter &filter, SymbolContext &context, Address *addr) {
   BreakpointSP breakpoint_sp = GetBreakpoint();
   Breakpoint &breakpoint = *breakpoint_sp;
 
   if (filter.AddressPasses(m_addr)) {
     if (breakpoint.GetNumLocations() == 0) {
-      // If the address is just an offset, and we're given a module, see if we
-      // can find the appropriate module loaded in the binary, and fix up
-      // m_addr to use that.
-      if (!m_addr.IsSectionOffset() && m_module_filespec) {
+      // If the address is just an offset ...
+      if (!m_addr.IsSectionOffset()) {
+        ModuleSP containing_module_sp = nullptr;
         Target &target = breakpoint.GetTarget();
+        if (m_module_filespec) {
+          // ... and we're given a module, see if we can find the
+          // appropriate module loaded in the binary, and fix up
+          // m_addr to use that.
           ModuleSpec module_spec(m_module_filespec);
-        ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec);
-        if (module_sp) {
+          containing_module_sp =
+              target.GetImages().FindFirstModule(module_spec);
+        } else {
+          // ... and we're not given a module, see if the offset is
+          // somewhere in the executable module. If it is, then we'll
+          // fix up m_addr to use that.
+          containing_module_sp = target.GetExecutableModule();
+        }
+        if (containing_module_sp) {
           Address tmp_address;
-          if (module_sp->ResolveFileAddress(m_addr.GetOffset(), tmp_address))
+          if (containing_module_sp->ResolveFileAddress(m_addr.GetOffset(),
+                                                       tmp_address))
             m_addr = tmp_address;
         }
       }
 
       m_resolved_addr = m_addr.GetLoadAddress(&breakpoint.GetTarget());
       BreakpointLocationSP bp_loc_sp(AddLocation(m_addr));
       if (bp_loc_sp && !breakpoint.IsInternal()) {
         StreamString s;