Bug 20511 – Remove static initializers on Windows (StaticConstructors.h)

• Component: Platform
• Status: RESOLVED FIXED

[danceoffwithyourpantsoff]

StaticConstructors.h does some really ugly macro replacements, to allow you to declare something an object than is global, but to have the construct run when you choose.  This is done by declaring the variable as a void*[], and later filling it in with placement new.  This won't link with MSVC, because they types aren't correct, you're expecting to link against a FooType, and you're getting a void*[].  Overall, this is approach is a super ugly hack, and I think the system should just be fixed.  Can we not just make these function calls (lazy initialized singletons?), or structure with a pointer that is NULL, and then gets initialized in the init routines?  I realize this would require changing a bunch of accessing code, but I think it is the correct solution.

In the mean time, I have a hacky working solution in MSVC (although, it's less hacky then the current code).  The problem is that in order to make it work, AVOID_STATIC_CONSTRUCTORS must actually be undefined, which will require either fixing the whole system, or leaving it undefined, even when we really are avoiding static constructors...

If you drop this in StaticConstructors.h, it will avoid static initialization under MSVC:

// This is a hack.  For MSVC, AVOID_STATIC_CONSTRUCTORS will not
// be set in config.h, because it's not supported.  The problem is that, we
// need to take a bunch of the paths that !AVOID_STATIC_CONSTRUCTORS takes
// this is the easiest thing to do:
// - Leave AVOID_STATIC_CONSTRUCTORS undefined, if we define it, the variables
//   will not be extern'd and we won't be able to link.  Also, we need the
//   default empty constructor for QualifiedName that we only get if
//   AVOID_STATIC_CONSTRUCTORS is undefined.
// - Assume that all includes of this header want ALL of their static
//   initializers ignored.  This is currently the case.  This means that if
//   a .cc includes this header (or it somehow gets included), all static
//   initializers after the include will be ignored.
// - We do this with a pragma, so that all of the static initializer pointers
//   go into our own section, and the CRT won't call them.  Eventually it would
//   be nice if the section was discarded, because we don't want the pointers.
//   See: http://msdn.microsoft.com/en-us/library/7977wcck(VS.80).aspx
#ifndef AVOID_STATIC_CONSTRUCTORS
#if COMPILER(MSVC)
#pragma warning(disable:4075)
#pragma init_seg(".unwantedstaticinits")
#endif
#endif

[Darin Adler]

(In reply to comment #0)
> Can we not just make these function calls
> (lazy initialized singletons?), or structure with a pointer that is NULL, and
> then gets initialized in the init routines?  I realize this would require
> changing a bunch of accessing code, but I think it is the correct solution.

Sure, we could do something like that. There are two issues:

    1) What would the accessing code look like? Would it be significantly less elegant?
    2) What would this do to performance? Would the code be measurably slower?

If we tackle both issues, we could certainly change this.

> In the mean time, I have a hacky working solution in MSVC (although, it's less
> hacky then the current code).  The problem is that in order to make it work,
> AVOID_STATIC_CONSTRUCTORS must actually be undefined, which will require either
> fixing the whole system, or leaving it undefined, even when we really are
> avoiding static constructors.

Or we could just change the name to make it clear that what was formerly named AVOID_STATIC_CONSTRUCTORS is really about a specific technique for doing avoiding static constructors with GCC. There's nothing sacred about the current macro name.

I'd love to see a patch that changes the name of the macro for clarity and also applies your technique for MSVC.

[danceoffwithyourpantsoff]

Created attachment 23029 [details]
Patch attempt at guard rename and MSVC support

[Darin Adler]

Comment on attachment 23029 [details]
Patch attempt at guard rename and MSVC support

 92 #if COMPILER(MSVC)
 93 #define HIDE_STATIC_CONSTRUCTORS_FROM_MSVC 1
 94 #else
 95 #define HIDE_STATIC_CONSTRUCTORS_FROM_GCC 1
 96 #endif

If the GCC technique is really GCC-specific, then I think we should guard it with #if COMPILER too. That might prevent use from having to mention these STATIC_CONSTRUCTORS macros at all in the config.h file, which would be a good thing, and might remove the need for the #undef. But that can be done in a separate patch.

But is the name good? Does this hide static constructors or eliminate them entirely? We might want to tweak the terminology to be as precise as possible. I'm not sure what the best terminology is for these global objects that need load time initialization. And for what exactly we're doing -- eliminating the load time initialization and replacing it with explicit initialization.

 23 // For WebCore we need to avoid having static constructors.  We use two hacks

I don't think the term "hacks" is really good here. There's just too much subjective opinion about what is and is not a "hack". The sentence about portability is clear and specific enough without using the term "hack". We should try to use specific language that's more precise

We normally don't use two spaces after periods in comments.

 28 // initializers.  The constructors will never be called and the object will

Should be "objects", not "object".

 31 // We both of these tricks, we then must defined and explicitly call an init

Missing verb here in "Web both". Typo "defined".

I'll say r=me, since these nitpicks are small things.

[danceoffwithyourpantsoff]

(In reply to comment #3)
> (From update of attachment 23029 [details] [edit])
>  92 #if COMPILER(MSVC)
>  93 #define HIDE_STATIC_CONSTRUCTORS_FROM_MSVC 1
>  94 #else
>  95 #define HIDE_STATIC_CONSTRUCTORS_FROM_GCC 1
>  96 #endif
> 
> If the GCC technique is really GCC-specific, then I think we should guard it
> with #if COMPILER too. That might prevent use from having to mention these
> STATIC_CONSTRUCTORS macros at all in the config.h file, which would be a good
> thing, and might remove the need for the #undef. But that can be done in a
> separate patch.

This is at least somewhat GCC specific.  Whether it would work on other compiler chains (what else really is used outside of gcc and msvc?) I'm not sure.  Even if you used a compiler like ICC, I imagine you're still going to use the GNU linker tools.

I think someone who understands the system better might want to do the config.h changes, or compiler guards.  I noticed it's removed on Symbian, but I have no idea how webkit is build there.

> 
> But is the name good? Does this hide static constructors or eliminate them
> entirely? We might want to tweak the terminology to be as precise as possible.
> I'm not sure what the best terminology is for these global objects that need
> load time initialization. And for what exactly we're doing -- eliminating the
> load time initialization and replacing it with explicit initialization.

Hide is the best term I could come up with.  Avoid isn't much better.  The idea is more or less this are static objects with static constructors, but we're somehow tricking the compiler into ignoring them, and we'll do it ourself.  MAKE_GCC_IGNORE_SOME_STATIC_CONSTRUCTORS ? :\

I would like the name to be as clear as possible, but I didn't come up with anything great.  I think you will need to read the documentation in the header to really understand what's going on anyway.  I don't think a single guard name is going to convey everything we need to convey.

> 
>  23 // For WebCore we need to avoid having static constructors.  We use two
> hacks
> 
> I don't think the term "hacks" is really good here. There's just too much
> subjective opinion about what is and is not a "hack". The sentence about
> portability is clear and specific enough without using the term "hack". We
> should try to use specific language that's more precise

It is a hack.  I will avoid using the term though.

> 
> We normally don't use two spaces after periods in comments.

All those high school typing classes gone to waste.

> 
>  28 // initializers.  The constructors will never be called and the object will
> 
> Should be "objects", not "object".
> 
>  31 // We both of these tricks, we then must defined and explicitly call an
> init
> 
> Missing verb here in "Web both". Typo "defined".
> 
> I'll say r=me, since these nitpicks are small things.
> 

I will fix this all up and send another patch tomorrow.  Thanks for the quick review.

[danceoffwithyourpantsoff]

Created attachment 23051 [details]
HIDE -> SKIP, English fixes in comments.

Hopefully addressed all of your advice.  I decided that SKIP is a more accurate description than HIDE, so I've updated the guards.

[danceoffwithyourpantsoff]

If you could commit this for me, that would be really great.  I'm not sure if it needs any ChangeLog changes, is so, anonymous authorship is fine.

Thanks

[Eric Seidel (no email)]

We don't use 2 spaces after periods in comments?

[Darin Adler]

Comment on attachment 23051 [details]
HIDE -> SKIP, English fixes in comments.

Great!

Looks fine; we can make slight refinements to the macros later, but this is excellent. All we need now is a ChangeLog and it's ready to go.

review- because I don't want the committer to have to write the ChangeLog entry -- please include it in the final patch.

[Darin Adler]

Comment on attachment 23051 [details]
HIDE -> SKIP, English fixes in comments.

OK, I'll add a ChangeLog and land it myself. Next time please write the ChangeLog even if you want to be anonymous.

[Darin Adler]

(In reply to comment #7)
> We don't use 2 spaces after periods in comments?

It's true, we don't. There may be occasional uses of it, but it's not the preferred style. We could put it in the coding guidelines to be more clear.

Here are some starting points for the wider discussion/debate on this: <http://www.webword.com/reports/period.html> and <http://www.google.com/search?q=two+spaces+after+period>.

[danceoffwithyourpantsoff]

Was this committed yet?  If not, now that I can decloak, I can write it with a proper ChangeLog and non-anonymous authorship.

Thanks

[Darin Adler]

No, I haven't committed yet. Sorry, I didn't get to it yet!

[danceoffwithyourpantsoff]

No problem.  If you are still up for making the ChangeLog, "Dean McNamee <[e-mail]>".

Thanks.

[Mark Rowe (bdash)]

Landed in r36071.