Page MenuHomePhabricator

Advanced guessing of rendezvous breakpoint
ClosedPublic

Authored by eugene on Dec 21 2017, 7:07 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

eugene created this revision.Dec 21 2017, 7:07 PM
eugene updated this revision to Diff 127980.Dec 21 2017, 7:25 PM
eugene updated this revision to Diff 127981.Dec 21 2017, 7:31 PM
eugene edited the summary of this revision. (Show Details)
labath added inline comments.Dec 22 2017, 3:06 AM
packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
371 ↗(On Diff #127981)

This test has been disabled for a couple of years. I wouldn't be surprised if the problems you are running into with it are due to bitrot.

I didn't know this test exists, so I've added TestBreakpointInGlobalConstructor.py to test this behavior a week or two ago. That one is known to pass on mac, so it shouldn't suffer from bitrot. If that's the case then maybe we can just remove this test (or the other one -- In any case, I think we don't need both of them).

source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
265 ↗(On Diff #127981)

So, the way I understand this, the main purpose of the entry breakpoint is to set the rendezvous breakpoint (the LoadAllCurrentModules is just a side-effect of us not being able to catch the library load early enough -- if we resolve the rendezvous breakpoint this way, it should not be needed.

I expect we can just omit the entry breakpoint altogether if we've managed to set the rendezvous breakpoint this way (we can probably keep the code as a fallback, but use it only for cases where we don't locate any of the rendezvous symbols).

345 ↗(On Diff #127981)

s/tring/trying

384 ↗(On Diff #127981)

the LLDB_LOG will automatically insert file+function (if you do log enable --file-function) so you can just drop this __FUNCTION__ part

599 ↗(On Diff #127981)

s/moduel/module

eugene updated this revision to Diff 128064.Dec 22 2017, 6:31 PM
  • Addressing review comments
  • Making sure all tests pass on Linux
  • Reformating
eugene marked 3 inline comments as done.Dec 22 2017, 6:37 PM
eugene added inline comments.
packages/Python/lldbsuite/test/functionalities/load_unload/TestLoadUnload.py
371 ↗(On Diff #127981)

I fixed both of them, I think it's fine to keep both.

eugene updated this revision to Diff 128065.Dec 22 2017, 6:57 PM
eugene edited the summary of this revision. (Show Details)

Don't set entry breakpoint if rendezvous breakpoint was set right after launch.

eugene marked an inline comment as done.Dec 22 2017, 6:57 PM
eugene updated this revision to Diff 129002.Jan 8 2018, 4:49 PM
eugene edited the summary of this revision. (Show Details)
eugene added a reviewer: tberghammer.
eugene added a project: Restricted Project.
eugene added a subscriber: lldb-commits.

Fix tests.

I'm adding some people with non-linux non-x86/arm targets who use this plugin for more visibility. I have a couple of small comments, but in general, I am happy with this.

packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py
24 ↗(On Diff #129002)

The right way to handle this is to use the environment which is returned from self.registerSharedLibrariesWithTarget as the launch environment (you'll need to launch using the SB API). The reason that TestLoadUnload does not handle this is probably because it predates this function.

source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
224 ↗(On Diff #129002)

s/enty/entry

379 ↗(On Diff #129002)

It should be possible to create the breakpoint via Target::CreateBreakpoint (the overload that lets you specify a list of function names) instead of manually resolving the symbols... Have you tried doing that?

Looks fine. Set the breakpoint using the list of names and delete the breakpoint if you get no locations and this will be good to go.

source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
365–368 ↗(On Diff #129002)

Will only one of these ever be present?

379 ↗(On Diff #129002)

Agreed, I would use the breakpoint setting function in target that allows you to specify one or more names as labath suggested. One question about this code though: how would we ever get a load address from a module if the dynamic loader doesn't load it itself? Are these symbols in the "[vdso]" module and is the [vdso]" module loaded differently?

399 ↗(On Diff #129002)

You should delete the breakpoint if the number of locations is zero (for this one by address and also in case you end up setting the breakpoint using a list of names). Otherwise you will have a dangling breakpoint that no one wants.

eugene marked 5 inline comments as done.Jan 9 2018, 8:04 PM
eugene added inline comments.
source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
365–368 ↗(On Diff #129002)

Yes. These are the names of rendezvous functions in different known linkers (Android, Linux, BSD) None of them should have more than one function from this list.

379 ↗(On Diff #129002)

OS loads the dynamic loader, if it is requested by the executable, and passes its base address via AT_BASE value in the auxiliary vector (https://refspecs.linuxfoundation.org/LSB_1.3.0/IA64/spec/auxiliaryvector.html). That's how we know where to look for it. (see EvalSpecialModulesStatus)

vdso is yet another special module, but it is not related to the dynamic loader.

eugene updated this revision to Diff 129216.Jan 9 2018, 8:05 PM
eugene marked an inline comment as done.

Addressing code review comments. Switching from manual symbol resolution to the appropriate overload of CreateBreakpoint.

labath accepted this revision.Jan 10 2018, 2:46 AM

looks good, thanks.

This revision is now accepted and ready to land.Jan 10 2018, 2:46 AM
ted added a comment.Jan 10 2018, 9:43 AM

Thanks for adding me, Pavel.

Hexagon running Linux uses this plugin. These changes lgtm.

Standalone Hexagon uses its own dyld plugin; I need to look at it and see if I want to pull any of these ideas into it.

This revision was automatically updated to reflect the committed changes.