This is an archive of the discontinued LLVM Phabricator instance.

Add Debugger::InitializeForLLGS to allow ref counted LLGS initialization.
ClosedPublic

Authored by flackr on Mar 9 2015, 2:44 PM.

Details

Summary

After http://reviews.llvm.org/D8133 landed as r231550 process launch on remote platform stopped working.

This adds Debugger::InitializeForLLGS and tracks whether one or both of Initialize and InitializeForLLGS have been called, calling only the corresponding lldb_private::Terminate* methods as necessary. Since lldb_private::Terminate calls lldb_private::TerminateForLLGS, the latter method may be called twice if Initialize was called for both however the terminate methods ensure they are only called once after being initialized.

This still maintains the reduced binary size, though it does now technically link in lldb_private::Terminate on lldb-server even though this should never be called.

This should resolve the issue raised in http://reviews.llvm.org/D8133 where Debugger::Terminate assumed that there were 0 references to debugger and terminated early.

Diff Detail

Repository
rL LLVM

Event Timeline

flackr updated this revision to Diff 21514.Mar 9 2015, 2:44 PM
flackr retitled this revision from to Add Debugger::InitializeForLLGS to allow ref counted LLGS initialization..
flackr updated this object.
flackr edited the test plan for this revision. (Show Details)
flackr added reviewers: vharron, tberghammer, clayborg.
flackr set the repository for this revision to rL LLVM.
flackr added a subscriber: Unknown Object (MLST).
vharron requested changes to this revision.Mar 9 2015, 2:49 PM
vharron edited edge metadata.

Please build and run tests under configure+make before commit.

This revision now requires changes to proceed.Mar 9 2015, 2:49 PM
flackr requested a review of this revision.Mar 9 2015, 4:29 PM
flackr edited edge metadata.

Please build and run tests under configure+make before commit.

Confirmed, tests using configure+make work with this commit.

flackr edited the test plan for this revision. (Show Details)Mar 9 2015, 4:32 PM
flackr edited edge metadata.
vharron accepted this revision.Mar 9 2015, 4:43 PM
vharron edited edge metadata.
This revision is now accepted and ready to land.Mar 9 2015, 4:43 PM
tberghammer edited edge metadata.Mar 10 2015, 3:10 AM

The reference counting looks good. Thanks for adding it.

source/lldb.cpp
189–192 ↗(On Diff #21514)

Please discuss with @vharron if ScriptInterpreterPython is necessary in LLGS or not (he removed it with a previous CL) and only put it back if it is used for something.

flackr added inline comments.Mar 10 2015, 9:45 AM
source/lldb.cpp
189–192 ↗(On Diff #21514)

Yes, he removed it because it was calling Debugger::Terminate in configure+make builds (oddly didn't seem to in cmake) which since it didn't have any references called lldb_private::Terminate cleaning up the objects we intended to use. It does seem to be necessary at least for remote process launching, and he was okay with adding it back as long as the configure+make tests still work which I've tested.

tberghammer accepted this revision.Mar 10 2015, 9:51 AM
tberghammer edited edge metadata.

Thanks for the explanation

This revision was automatically updated to reflect the committed changes.
clayborg edited edge metadata.Mar 10 2015, 1:45 PM

Looks good.