Page MenuHomePhabricator

[lldb] Consider binary as module of last resort
Needs ReviewPublic

Authored by hawkinsw on Sun, May 1, 12:05 AM.

Details

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.

Diff Detail

Event Timeline

hawkinsw created this revision.Sun, May 1, 12:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptSun, May 1, 12:05 AM
hawkinsw requested review of this revision.Sun, May 1, 12:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptSun, May 1, 12:05 AM

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!

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 requested changes to this revision.Thu, May 5, 11:28 AM

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.

This revision now requires changes to proceed.Thu, May 5, 11:28 AM

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 this revision to Diff 427791.Fri, May 6, 4:56 PM

Updated patch including responses to helpful feedback from @jingham.

hawkinsw updated this revision to Diff 427794.Fri, May 6, 4:59 PM

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

(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!

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

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 added inline comments.Fri, May 13, 12:14 PM
lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
12

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

22–23

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.

26

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

32
35
37
hawkinsw marked 5 inline comments as done.Fri, May 13, 10:10 PM
hawkinsw added inline comments.
lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
12

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?

26

Great suggestion!

hawkinsw marked an inline comment as done.Fri, May 13, 10:13 PM
hawkinsw updated this revision to Diff 429411.Fri, May 13, 10:15 PM

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

@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

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!

Still a few places where we have a dedicated assert

lldb/test/API/commands/breakpoint/set/address-nomodule/TestBreakpointAddressNoModule.py
26
29
hawkinsw updated this revision to Diff 430781.Thu, May 19, 1:00 PM

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

hawkinsw marked 2 inline comments as done.Thu, May 19, 1:02 PM

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

JDevlieghere accepted this revision.Fri, May 20, 9:13 AM

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

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

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!

This revision was not accepted when it landed; it landed in state Needs Review.Fri, May 20, 2:01 PM
This revision was automatically updated to reflect the committed changes.

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!

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 !

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

hawkinsw reopened this revision.Fri, May 27, 8:15 AM

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