This is an archive of the discontinued LLVM Phabricator instance.

Move LLDB initialization / shutdown to Application
ClosedPublic

Authored by zturner on Mar 18 2015, 4:16 PM.

Details

Summary

Move LLDB initialization/shutdown to Application.

This creates a new top-level folder called Application which is intended to hold code specific to using LLDB as an application. Currently this holds the Initialize() and Terminate() functions,
as well as the fatal error handler.

This provides a means to break the massive dependency cycle which is caused by the fact that Debugger calls Initialize and Terminate which bring in the entire LLDB project. So linking against Core forces a link against all of LLDB, instead of against just Core.

With the structure here, applications invoke lldb_private::Initialize() directly, and that invokes Debugger::Initialize. This way Application depends on Debugger, but Debugger doesn't depend on Application.

I think this organization makes better sense anyway. Having a bunch of functionality in source/lldb.cpp with an ambiguous name doesn't really make it clear how the functionality is related or what it is. By creating an Application folder, it makes more sense that we can put other stuff in there which is of interest specifically to program startup and initialization. There's probably some duplicated logic already between lldb-server, lldb, and other applications that link against LLDB which could be put here someday.

Diff Detail

Event Timeline

zturner updated this revision to Diff 22225.Mar 18 2015, 4:16 PM
zturner retitled this revision from to Move LLDB initialization / shutdown to Application.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a reviewer: clayborg.
zturner added a subscriber: Unknown Object (MLST).

Sorry, this isn't quite right yet. I didn't get Termination correct.

zturner updated this revision to Diff 22231.Mar 18 2015, 5:15 PM

This should better handle termination. Previously people would call Debugger::Terminate() which would test the refcount, and only invoke lldb_private::Terminate() when the refcount was 0. This patch reverses this. Applications now call lldb_private::Terminate(), which *always* calls Debugger::Terminate(), and Debugger::Terminate() returns the refcount. lldb_private::Terminate() then only does the actual shutdown once the refcount is 0.

+flackr and ovyalov since this is of interest to lldb-server. I have another patch which is built on top of this patch which reduces the binary size of lldb-server by about 15% (from 27M down to 23M on my machine). Also you guys have done some work in this area recently.

clayborg requested changes to this revision.Mar 18 2015, 5:44 PM
clayborg edited edge metadata.

Can we change "Application" to something else? Maybe "InternalInitialization", "Initialization", or "Private" to make it clear that is for internal initialization? I don't want people thinking "Hey, I am making an application and somehow I need access to this "Application" stuff. This initialization/termination code is designed for people that will user the lldb_private::* APIs only and we should document this in the header file. Public APIs will still call SBDebugger::Initialize() and SBDebugger::Terminate(). "Application" just sounds like the wrong wording here. I like the general concept, if we can just work out this naming I would like all of it.

This revision now requires changes to proceed.Mar 18 2015, 5:44 PM

Maybe "InternalAPI"?

Or "APIPrivate" or "PrivateAPI"?

I actually had it called Initialization originally. I like Initialization slightly better than InternalAPI because it makes it clear that that the stuff is specific to initialization. InternalAPI makes me think I can go put a bunch of random stuff in it which might make more sense somewhere else where it's more logically grouped.

Other names might be something like DebuggerInit or DebuggerLifetime

Anything with API in it makes me think it's related to the SB API. But LLGS uses it for example, and it doesnt' even link in the public API. So I think the name should say something about the fact that it's related to initialization and term. I kind of like DebuggerLifetime now, what do you think?

"Initialization" would be fine.

zturner updated this revision to Diff 22272.Mar 19 2015, 10:17 AM
zturner edited edge metadata.

Changed name to Initialization.

flackr edited edge metadata.Mar 19 2015, 2:24 PM

Looks like a good separation of dependencies, thanks for doing this.

include/lldb/Core/Debugger.h
70

You should document what this returns since it seems non-obvious.

include/lldb/Initialization/InitializeLLDB.h
44

DCInitialize should be just Initialize right?

56

ditto.

source/Core/Debugger.cpp
420

I think this is no longer used.

438

Might it not make more sense to just move the ref counting to InitializeLLDB at this point? Do we still depend on calls to Debugger::Initialize adding to the ref count? And if so that would represent a chance to not terminate at shutdown due to a pre-existing debugger reference right?

zturner added inline comments.Mar 19 2015, 2:37 PM
source/Core/Debugger.cpp
438

It would be nice wouldn't it? But g_shared_debugger_refcount is still used quite a bit inside of Debugger.cpp.

It's probably possible to get rid of it, but it's somewhat intertwined with various functions, so it's a little riskier than I'm comfortable with just for this CL. After D8462 goes in, it might a nice cleanup to see if g_shared_debugger_refcount can be gotten rid of, as once that CL goes in, I'm explicitly maintaining the ref count in Initialization

But it's *very important* that Debugger not depend on Initialization, because that will just re-introduce the dependency cycle which I'm explicitly trying to break. I have some ideas about how to solve that too, but I think it makes more sense to talk about it after D8462 goes in, otherwise too much stuff will be going into the same CL.

zturner updated this revision to Diff 22309.Mar 19 2015, 2:50 PM
zturner edited edge metadata.

Update the Xcode project, this should build cleanly now on OSX

clayborg accepted this revision.Mar 19 2015, 2:54 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Mar 19 2015, 2:54 PM
zturner closed this revision.Oct 15 2015, 1:55 PM