This is an archive of the discontinued LLVM Phabricator instance.

Move full initialization to lldb/API
ClosedPublic

Authored by zturner on Mar 19 2015, 2:04 PM.

Details

Summary

This patch builds on top of D8428. The idea behind this change is to separate full initialization and LLGS initialization more strongly. Because InitializeForLLGS is in the same source file (or even the same library) as Initialize(), the linker has to deal with all these extra symbols, increasing binary size. In theory it can do dead code elimination, but it doesn't always do an optimal job (as the numbers later on show). Furthermore, there is some duplicated logic with regards to checking the ref count, and making sure Debugger::Initialize() and Debugger::Terminate is called in the correct order.

This patch addresses all of these issues by introducing a class called SystemLifetimeManager which *only* deals with the interaction between the ref count, calling Debugger::Initialize / Terminate, and actually shutting down the complete set of system services. In particular, it does *not* deal with the implementation details of what system services to initialize and shutdown.

This is accomplished by introducing an abstract base class called SystemInitializer with Initialize() and Terminate() methods. A standard implementation called SystemInitializerCommon is provided in the Initialization library. The functionality contained in here is identical to the functionality which used to be in InitializeForLLGS() and TerminateLLGS().

The full initialization implemetantion is now compiled directly into source/API via source/API/SystemInitializerFull.cpp, which provides an implementation built on top of SystemInitializerCommon (this is equivalent to the functionality that used to be in lldb_private::Initialize() / lldb_private::Terminate()

This all seems rather complicated, but using this approach the binary size of llgs is reduced quite a bit. Here are some numbers:

Before Patch:

Debug 738,065,786 bytes
Release 27,333,622 bytes
MinSizeRel 21,818,230 bytes
MinSizeRel with stripped symbols 16,364,008 bytes

After Patch:

Debug 475,575,879 bytes
Release 24,120,433 bytes
MinSizeRel 19,034,215 bytes
MinSizeRel with stripped symbols 14,395,872 bytes

So I'm seeing an improvement of about 12% for a fully minimized binary.

Note I haven't updated the Xcode project yet. I will do that now, but I wanted to put this up first for the high level idea.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 22300.Mar 19 2015, 2:04 PM
zturner retitled this revision from to Move full initialization to lldb/API.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: clayborg, ovyalov, vharron, flackr.
zturner added a subscriber: Unknown Object (MLST).
zturner updated this object.Mar 19 2015, 2:06 PM

My mistake. The patch builds on top of D8428, not the one I typed previously.

zturner updated this revision to Diff 22310.Mar 19 2015, 2:52 PM

Update the Xcode project, this should build cleanly on OSX now when applied on top of the first patch.

clayborg requested changes to this revision.Mar 19 2015, 3:01 PM
clayborg edited edge metadata.

See inlined changes.

source/API/SBDebugger.cpp
104 ↗(On Diff #22310)

Please put this in a static function and call the singleton and have it return a reference. The Apple build system prohibits frameworks from having global initializers.

So make this:

static SBDebuggerLifetimeManager &
GetDebuggerLifetimeManager ()
{
    static llvm::ManagedStatic<SBDebuggerLifetimeManager> g_debugger_lifetime;
    return g_debugger_lifetime;
}
142 ↗(On Diff #22310)

Change this to:

GetDebuggerLifetimeManager ().AddRef();
148 ↗(On Diff #22310)

Change this to:

GetDebuggerLifetimeManager (). Release();
tools/lldb-server/lldb-server.cpp
35 ↗(On Diff #22310)
NOTE: this static is OK because this is in an application, not in a shared library or framework and Apple builds are ok with this.
This revision now requires changes to proceed.Mar 19 2015, 3:01 PM

See inlined changes.

I'll change it, but out of curiosity, I built it in Xcode and didn't see any issues. How would I need to build it to see the failure? ManagedStatic<> has a trivial constructor, is the Apple build system ok with it in that case?

Try making a small C++ class and then making a global. You should see a warning for this global object. If this doesn't happen with your managed static, then we are ok and no changes are required. Just make sure you see the warning I am talking about with a C++ class that maybe inits a std:string with something.

I checked and I saw the warning with a different class but not with ManagedStatic<>. Since ManagedStatic<> is POD it only relies on the linker for initialization, so it looks like it doesn't have this issue. I think we don't have to worry about that changing in the future either. A comment in ManagedStaticBase.h says this:

// This should only be used as a static variable, which guarantees that this
// will be zero initialized.

So relying on the linker for initialization is part of the guarantee of the class, so it should never get a global constructor in the future either. Seems like we're ok then?

I checked and I saw the warning with a different class but not with ManagedStatic<>. Since ManagedStatic<> is POD it only relies on the linker for initialization, so it looks like it doesn't have this issue. I think we don't have to worry about that changing in the future either. A comment in ManagedStaticBase.h says this:

// This should only be used as a static variable, which guarantees that this
// will be zero initialized.

So relying on the linker for initialization is part of the guarantee of the class, so it should never get a global constructor in the future either. Seems like we're ok then?

Just to be certain I saw the same warning you're talking about, I believe it said something like "Declaration requires a global destructor"

Yes the warning is "Declaration requires a global destructor".

Ok, seems like we're good on that front then. Any thoughts on the rest of the CL? Incidentally it seems like g_debugger_shared_refcount is no longer needed after this CL, but it was too risky for this CL, so I thought I would try to do that later.

clayborg accepted this revision.Mar 19 2015, 5:28 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Mar 19 2015, 5:28 PM
ovyalov accepted this revision.Mar 19 2015, 10:46 PM
ovyalov edited edge metadata.
ovyalov added inline comments.
source/API/SystemInitializerFull.cpp
1 ↗(On Diff #22310)

Could we keep all initializers within source/Initialization folder?

source/Initialization/SystemLifetimeManager.cpp
68 ↗(On Diff #22310)

s/shared_ptr/unique_ptr

Actually the entire point of the CL is to move the full initializer. By
keeping it in the same place the linker has to include all those extra
symbols. It can do dead code elimination, but it's not 100% foolproof. The
reason this CL reduces LLGS binary size from 16M to 14M is precisely
because full initializer is moved out

Incidentally it seems like g_debugger_shared_refcount is no longer needed after this CL, but it was too risky for this CL, so I thought I would try to do that later.

I think it is still required by source/Interpreter/ScriptInterpreterPython.cpp. I would be happy if we can remove the magic (hack?) using it there but it looks far from trivial to do it.

vharron edited edge metadata.Mar 20 2015, 7:20 AM

Can we hold this CL until we have buildbots testing remote Linux? Maybe a
couple of weeks?

Thanks for this CL BTW

I think this CL won't effect remote Linux in any different way then local Linux so I don't see any reason why should we hold it back. We do several other change every week what can have major effect on remote Linux without having a build bot and we haven't seen too much issue because of it. Also holding back this CL for several weeks can cause merge conflicts where I think the chance to make a mistake when resolving them would be higher then the risk of this change. I also think that this is just 1 step from a longer process of cleaning up the dependencies in lldb. Blocking that process for a longer time seems like a very bad idea for me.

What Tamas said. I can hold it if there's a good reason, but I don't think
there's much risk involved here. The actual initialization / shutdown code
is a straight copy/paste from what it was before. So the question is just
if it's calling the initialization and shutdown code correctly. And the
local linux test suite would catch that if it were an issue.

vharron accepted this revision.Mar 20 2015, 12:45 PM
vharron edited edge metadata.

One of these lldb-server optimization changes did break remote linux and I had to diagnose it. Tamas, can you test for Zach before committing?

@zturner: Please give me a ping when you sorted out the "ScriptInterpreterPython initialization" issue so I can try out your patch on remote Linux just to make sure.

Ok, i will try to get to it today, i got sidetracked on a few other issues

zturner updated this revision to Diff 22765.Mar 26 2015, 3:42 PM
zturner edited edge metadata.

So the way I had to fix this was to change the semantics of Initialization and Termination slightly. I wanted to hold off on doing this until after this went in, but it seems there's no way to get around doing it now.

Basically, the original problem was this:

When running lldb the executable, we call SBDebugger::Initialize() and SBDebugger::Terminate(). Later, when ScriptInterpreterPython is initialized, it does an "import lldb", which implicitly calls SBDebugger::Initialize() *again*, which increments the refcount again. But there's only ever that one call to SBDebugger::Terminate -- the one we write explicitly. To account for this we had this hack where we decremented the refcount after the import statement if the import increased the refcount.

What this hack was really doing was saying "if we are running inside of lldb.exe, decrement the refcount after this import statement, so that at the end of the program, SBDebugger::Terminate() actually causes a clean shutdown. If we are running inside of python, leave the refcount alone, because there's only a single SBDebugger::Initialize() anyway -- the one that implicitly happens when you import -- so the atexit SBDebugger::Terminate will work".

To fix this in what I believe is the "correct" way, we simply kill the notion of a refcount entirely. It's either initialized or it's not. You can call Initialize() as many times as you want, and only the first call ever does anything. The rest of the calls just exit. And the first time you call Terminate() it really terminates.

This matches both the python usage pattern as well as the lldb.exe usage pattern. In the former, SBDebugger::Initialize() is called twice, but the second is a no-op, and the atexit handler to call SBDebugger::Terminate does the right thing. In the latter, there's only a single SBDebugger::Initialize and terminate, so everything works fine.

This also satisfies Greg's requirement that "import lldb" should be all you need to get up and running, since the SBDebugger.Initialize() is still there.

A side benefit of this change is that if someone accidentally calls SBDebugger.Initialize() from python, with this patch everything will still work. Prior to this patch, that would have raised the refcount to 2, and the single SBDebugger.Terminate() call we have in the atexit handler would only drop it to 1.

zturner updated this revision to Diff 22768.Mar 26 2015, 4:24 PM

Fixed an error in my previous patch that would cause everything to fail. I wasn't setting m_initialized to true the first time around.

If you fix the namespace problem (see inline comment) than it looks good and don't cause any issue with remote Linux.

tools/lldb-server/lldb-server.cpp
20 ↗(On Diff #22768)

SystemLifetimeManager is in lldb_private namespace. Please add namespace declaration as it isn't compile on Linux this way.

This revision was automatically updated to reflect the committed changes.
lldb/trunk/source/Core/Debugger.cpp