Merge lp://qastaging/~smspillaz/compiz-core/compiz-core.fix_931927 into lp://qastaging/compiz-core

The noOptions API change makes me nervous. Especially the part about returning a reference to a static local:

203 +CompOption::Vector &
204 +noOptions ()
205 +{
206 + static CompOption::Vector v (0);
207 + return v;
208 +}

Why not just change the global:
    CompOption::Vector noOptions (0);
to:
    const CompOption::Vector noOptions (0);
??

And change:
    extern CompOption::Vector noOptions;
to:
    extern const CompOption::Vector noOptions;

And the same for all of:
164 -CompOption::Vector noOptions (0);
165 -CompOption::Value::Vector emptyList;
166 -CompString emptyString;
167 -CompMatch emptyMatch;
168 -CompAction emptyAction;
169 -unsigned short emptyColor[4];

All the globals should remain globals instead of functions. Perhaps convert them to const for safety.

Static initialization and destruction, basically.

There's a case where the destructor would be called on the global when a plugin was loaded (compiler, linker bug?) before it was actually fully constructed.

I would have made them const, but a lot of the Option* code requires a non const reference. It worked fine even when the globals weren't const, so I don't think this is an issue.

(See http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.14)

OK, that's not a compiler/linker/loader bug at all. That is correct behaviour. I have seen many such static ctor/dtor DSO bugs on various Unixes before, but not Linux, at least not for ~10 years.

The real problem must be globals using other globals during their construction/destruction (again). That's what really needs fixing I think.

Its not worth fixing in this branch :)

The best way to get around this fiasco, is like the article says, to use the construct-upon-access idiom which is what I have done here.

> OK, that's not a compiler/linker/loader bug at all. That is correct behaviour.
> I have seen many such static ctor/dtor DSO bugs on various Unixes before, but
> not Linux, at least not for ~10 years.

To clarify, I guess the reason why I said linker bug was because of this

#0 CompAction::active
#1 CompOption::~CompOption // wtf ?
#2 __static_initialization_and_destruction

The use of uninitialized globals within the constructors of other globals is fine, but calling the destructor to a static object before we hit main () is very strange indeed.

(And yes, I've hit compiler bugs before where destructors were incorrectly called :P)

OK, I agree it's a quick fix that should work, even though it impacts the public API in a less than ideal way.

Alan already started logging bugs describing the removal of globals such as screen (in the distant future)...

And I do believe you now :)

But you will probably find some obscure DSO-related reason for the apparently premature destruction.

> OK, I agree it's a quick fix that should work, even though it impacts the
> public API in a less than ideal way.
>
> Alan already started logging bugs describing the removal of globals such as
> screen (in the distant future)...

The good news is that only one plugin was impacted by this.

It would be so awesome if const references could be NULL. I guess that's what exceptions are for.

I have to agree with Daniel on a number of points. This is kinda ick, but
better than nothing as far as I can tell.

Might as well remove the boost/noncopyable header since you are not using it.

You do hit a couple of differences here.

Static initialisation initially zeros things out. So...

unsigned short emptyColor[4];
would be zero initialised.

When you have a static variable in a function, I don't think it is zero
initialised. Worth checking with Alan about that. So, inside the emptyColor
function use:
  static unsigned short v[4] = {0,0,0,0};

We need to file bugs about the catch (...) bits, that's just ick.

I can't comment on whether this fix actually works. Ideally we'd have test
coverage :-(

> And I do believe you now :)
>
> But you will probably find some obscure DSO-related reason for the apparently
> premature destruction.

What do you mean by DSO-related?

And I have to say that everytime I've come across a weird destruction bug, whether mis-timed or double delete, it has been a coding error, not the compiler.

I mean that if this destruction issue /is/ a bug then it is most likely triggered by using dynamic shared objects. I've seen many such loader/construction/destruction bugs before. But none in Linux since at least before Ubuntu existed.

Re: "Static initialisation initially zeros things out"

Is that in the C++ spec? As far as I know, basic types like;
    unsigned short emptyColor[4];
will not be initialized at all, because they don't have a constructor.

> Re: "Static initialisation initially zeros things out"
>
> Is that in the C++ spec? As far as I know, basic types like;
> unsigned short emptyColor[4];
> will not be initialized at all, because they don't have a constructor.

Normally no, but for statics / globals, yes.

If that's true I would still recommend explicit initialization for the sake of readability :)

> Re: "Static initialisation initially zeros things out"
>
> Is that in the C++ spec? As far as I know, basic types like;
> unsigned short emptyColor[4];
> will not be initialized at all, because they don't have a constructor.

Local statics have a special case (don't you just love C++) - they are zero initialized (the same as namespace scope variables - which is what they are to the link loader).

Oh, and you don't need

"static unsigned short v[4] = {0,0,0,0};"

either:

"static unsigned short v[] = {0,0,0,0};"

or

"static unsigned short v[4] = {0};"

is fine

*** Found the problem ***

CompOption::Vector noOptions (0);
causes a reserve of at least one CompOption to be constructed (and then quickly destructed)

The fix is to change:
    CompOption::Vector noOptions (0);
to:
    CompOption::Vector noOptions;

> *** Found the problem ***
>
> CompOption::Vector noOptions (0);
> causes a reserve of at least one CompOption to be constructed (and then
> quickly destructed)
>
> The fix is to change:
> CompOption::Vector noOptions (0);
> to:
> CompOption::Vector noOptions;

It is still better to use the on-demand construction, for reasons specified above. I'll fix this though.

This means we don't need to change the public API of "noOptions" or need to change any plugins. You're now doing it for stylistic reasons only. :)

There is still a risk involved. I'd much prefer to have less risky code then to be tripped up later.

This isn't "purely stylistic"

(Also as evidenced by the fact that PrivateAction was not even created during static initialization and destruction makes me more inclined to make it like this)

170 + CompString & emptyString ()

Rather invites:

emptyString () = CompString("Murphy rules!");

(The same applies to a lot of the functions, and the original [non-]constants.)

So, we should add const to the "constants" while addressing this problem.

On the comment discussion: Global data, monostate & singleton are all likely causes for issues. So this code could do with some cleanup regardless of "on demand" or "global data".

As Sam says, it is slightly safer to use "on demand" than global variables - as it ensures they have actually been initialized before use. OTOH, if we "know" there is no init-sequence issue that only makes the design marginally better.

So, I wouldn't have done the "wrapping" if the problem can be fixed more simply. But, if we had the code ready to go I'd take it, it make the codebase a safer place to live.

I can remove all the global data, but I can tell you know that the diff is going to get a *lot* bigger (ditto for adding constness)

Sam, if the change to the vector constructor is enough to fix this bug, we should go with the simplest possible branch.

I have to admit to preferring to use the default constructor for vectors rather than trying to be clever with the size. It also means that people don't have to go and look for what the integer parameter constructor does.

"Don't initialize to list size zero since that creates a temporary and destroys it
(for gosh knows what reasons)"

That's because it calls a constructor with a default parameter. Vis:

CompOption::Vector(o, CompOption());

I'd really like to know what all the push back on delaying initialization till use is, its a far safer way of doing things and doesn't have any obvious drawbacks.

Please also change the constructor in noOptions too:

  static CompOption::Vector v (0);

should be:

  static CompOption::Vector v;

Given that there were also issues around the ordering of the static object initialisation that Sam said he saw, I'm in favour of landing this branch. However, we should look to clean the whole mess up soonish.

I want to know though what the impact is of making these constants.

Yes indeed on demand construction is more than stylistic. I think my clumsy words missed the point there. My point should have been that on demand construction (while being good for preventative maintenance), is causing a lot more code to change just before a release. And that's a risk.

Conditionally approved, pending the 1-line fix Tim mentioned:
210 + static CompOption::Vector v (0);
becomes:
210 + static CompOption::Vector v;

I've also tested for further API breakage in libcompizconfig, compiz-plugins-main and compiz-plugins-extra. Only main (expo) failed, which Sam has already proposed a fix for.

Impact of removing the const is that we need to remove these

CompAction &
CompOption::Value::action ()
{
    try
    {
 return boost::get<CompAction>(mValue);
    }
    catch (...)
    {
 return emptyAction ();
    }
}

CompMatch &
CompOption::Value::match ()
{
    try
    {
 return boost::get<CompMatch>(mValue);
    }
    catch (...)
    {
 return emptyMatch ();
    }
}

CompString &
CompOption::Value::s ()
{
    try
    {
 return boost::get < CompString > (mValue);
    }
    catch (...)
    {
 return emptyString ();
    }
}

which I am +1 for but I'm not sure how much client code uses these