[PATCH] Fix initialization problems with PassRegistry

[David Blaikie]

On Wed, Jun 11, 2014 at 1:20 PM, Reid Kleckner <rnk at google.com> wrote:
> To me the core problem is that things aren't being destroyed in reverse
> order of construction.  This is the timeline:
>
> start ctor of PassListener
> start ctor of PassRegistry
> finish ctor of PassRegistry
> finish ctor of PassListener
> ... main ...
> llvm_shutdown
> start and finish dtor of PassRegistry
> ... return from main...
> start dtor of PassListener -> references the "destroyed" PassRegistry
>
> The fact that this "recovers" by reinitializing the PassRegistry is pretty
> bogus.

Totally.

> As a workaround, I'm strongly in favor of not reinitializing the
> PassRegistry when attempting to deregister the listener.  We'd need some
> helper on ManagedStatic to let us check if the internal pointer is null.

Yep, that would be one way. The other that I've been vaguely
describing/alluding to would be to remove the auto-registration
machinery from PassRegistrationListener and build two devices, one for
use as a global (who's dtor is a no-op) and one for local use that
does both adding and removing the listener.

I'm more-or-less OK with either.

If it's not hard (as this patch demonstrates) to remove all the global
registrations and just make them locals (in which case they should be
able to keep their auto-registration semantics, which this patch
removes - why does it remove them?) then that removes some global
ctors from LLVM (or does it just remove global ctors from LLVM
utilities but not LLVM libraries? in which case it's no great win or
loss), that's not a bad thing.

But I still don't understand why the StaticPassRegistry stuff is here
- the changes to how passes are registered seems like a separate
problem to the issues around how PassRegistrationListeners are
attached/removed. (for example, passes are never removed from a
PassManager, are they?)

- David

>
>
> On Wed, Jun 11, 2014 at 1:00 PM, Zachary Turner <zturner at google.com> wrote:
>>
>> Another alternative is writing to immutable static (not ManagedStatic)
>> data structures during static initialization, and then copying them over to
>> the ManagedStatics after main begins.
>>
>> The fundamental problem, at least in my mind, is this scenario:
>>
>> 1) During static construction, you access a ManagedStatic, causing it to
>> be allocated.
>> 2) Before main exits, you call llvm_shutdown to destroy ManagedStatics,
>> including the one allocated in #1
>> 3) During static destruction, you do the inverse operation of what you did
>> in #1.  This also accesses a ManagedStatic, after llvm_shutdown has been
>> called, and allocates another one.
>>
>> Plus, it only resolves ordering insofar as the ordering of the static
>> constructors is resolved.  Which is to say, not very much.  You get
>> deterministic destruction order, but you still get non-deterministic
>> construction order, because you dont' know in which order the static
>> constructors which use the ManagedStatics will be called in.
>>
>>
>> On Wed, Jun 11, 2014 at 12:41 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>>
>>> On Wed, Jun 11, 2014 at 11:54 AM, Zachary Turner <zturner at google.com>
>>> wrote:
>>> > I might be able to solve the lock problem by just deleting the lock
>>> > access from the destructor, as you mention, or making the lock an instance
>>> > variable of the PassRegistry class.  But I still think the way I've done is
>>> > the only real way to solve the problem with the PassRegistrationListener.
>>>
>>> It seems like the problem with PassRegistrationListener would be
>>> solved by having a static device (other than
>>> PassRegistrationListener's ctor itself) that simply doesn't remove the
>>> listener in its dtor. The problem being then you change the behavior
>>> of non-static PassRegistrationListeners (since you have to remove the
>>> registration machinery from PassRegistrationListener's ctor/dtor).
>>> Global registration devices are pretty common.
>>>
>>> All that being said, removing global ctors is a soft goal of the
>>> project, so I'm OK doing it for those reasons - I'm just trying not to
>>> conflate different goals/problems & understand which things we're
>>> doing for what reasons.
>>>
>>> > Ultimately, the root of the problem is that ManagedStatics are being
>>> > accessed during static initialization and shutdown.
>>>
>>> I don't actually see those things as inherently wrong. Something like
>>> a ManagedStatic is the only way to resolve ordering in global init
>>> (yes, the alternative is to restrict the project to never have globals
>>> that access other globals - but that's not the only correct answer).
>>> During destruction is more subtle, certainly.
>>>
>>> > This needs to be prevented, and I think we need to try to get to a
>>> > point where we can actually enforce, by way of asserts, that you cannot
>>> > touch a ManagedStatic before main is entered or after it returns.
>>> >
>>> > http://reviews.llvm.org/D3996
>>> >
>>> >
>>
>>
>