Bug 174714 – Drop WTF::initializeDate, if we can stop using -fno-threadsafe-statics

• Status: REOPENED
• Component: New Bugs

[Yusuke Suzuki]

[WTF] Drop initializeDate

[Yusuke Suzuki]

Created attachment 316092 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316092 [details]
Patch

Clearing flags on attachment: 316092

Committed r219732: <http://trac.webkit.org/changeset/219732>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 316092 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316092&action=review

> Source/WTF/ChangeLog:8
> +        minYear static variable will be initialized in an exclusive manner, which is ensured by C++ "static" semantics.

Is that true? I thought we were compiling with no-threadsafe-statics.

[Mark Lam]

(In reply to Alexey Proskuryakov from comment #4)
> Comment on attachment 316092 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316092&action=review
> 
> > Source/WTF/ChangeLog:8
> > +        minYear static variable will be initialized in an exclusive manner, which is ensured by C++ "static" semantics.
> 
> Is that true? I thought we were compiling with no-threadsafe-statics.

I think our NeverDestroyed implementation relies on threadsafe-statics.  Otherwise, we'll have issues, no?

[Yusuke Suzuki]

Comment on attachment 316092 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316092&action=review

>>> Source/WTF/ChangeLog:8
>>> +        minYear static variable will be initialized in an exclusive manner, which is ensured by C++ "static" semantics.
>> 
>> Is that true? I thought we were compiling with no-threadsafe-statics.
> 
> I think our NeverDestroyed implementation relies on threadsafe-statics.  Otherwise, we'll have issues, no?

Right, if we compiled WebKit with no-threadsafe-statics, we should already encounter issues with NeverDestroyed.
I remember that I grepped this a bit while ago, and I can't find any no-threadsafe-statics in WebKit build settings. Only libwebrtc uses it. But WebKit itself does not.
I've grepped again now and I can't find it.

[Darin Adler]

We do compile with no-threadsafe-statics.

We only use initialization NeverDestroyed when thread safety is not needed.

[Darin Adler]

Many of the uses of LazyNeverDestroyed that use std::call_once are for thread safety.

[Darin Adler]

Maybe I am wrong. If we really stopped using -fno-threadsafe-statics, then I can do another round of cleanup and get rid of almost all uses of LazyNeverDestroyed!

[Alexey Proskuryakov]

WebKit is built with -fno-threadsafe-statics. Perhaps there is some configuration that enables that, but all that I checked do not.

See e.g. https://build.webkit.org/builders/Apple%20Sierra%20Release%20%28Build%29/builds/3721/steps/compile-webkit/logs/stdio

[Alexey Proskuryakov]

Checking wtf/DateMath.cpp specifically, it is also compiled with -fno-threadsafe-statics.

[Darin Adler]

So did we create a thread safety problem or not? Can equivalentYearForDST be called from multiple threads?

[Darin Adler]

I think we did, and I think this patch probably needs to be rolled out.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #7)
> We do compile with no-threadsafe-statics.
> 
> We only use initialization NeverDestroyed when thread safety is not needed.

I think we have a problem. I remember that Mark changed NeverDestroyed<String>'s ASCIILiteral parameter to StaticStringImpl because this is not safe to access NeverDestroyed<String>.
It means that some of NeverDestroyed<String> can be accessed from multiple threads. If so, without threadsafe statics, it is dangerous.
I think we will see
1. constructor is called twice, it may break structure during the other thread uses it.
2. we may return half-baked structure if the other thread is constructing it right now.

(In reply to Alexey Proskuryakov from comment #10)
> WebKit is built with -fno-threadsafe-statics. Perhaps there is some
> configuration that enables that, but all that I checked do not.
> 
> See e.g.
> https://build.webkit.org/builders/Apple%20Sierra%20Release%20%28Build%29/
> builds/3721/steps/compile-webkit/logs/stdio

Wow, that's a problem! I didn't know that this is enabled since any xcodeproje does not specify it.
We should adopt this option in CMake build files as well to clarify our build configuration. Filed. https://bugs.webkit.org/show_bug.cgi?id=174747

(In reply to Darin Adler from comment #13)
> I think we did, and I think this patch probably needs to be rolled out.

Yes, we should roll out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 174748

[Yusuke Suzuki]

For example, I think JSC/runtime/JSGlobalObjectFunctions.cpp's Bitmaps are racy if threadsafe statics is not ensured.

[Darin Adler]

To make our programming easier we can start compiling all of WebKit with thread-safe statics if we can do performance tests and prove that it does not come at an unacceptable performance cost. At the time we originally turned that flag on it was critical for performance.

Until then if we accidentally write code with thread safety problems then we should rewrite to avoid those problems.