Bug 211036 – Use `static Lock` instead of `static NeverDestroyed<Lock>`

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Use `static Lock` instead of `static NeverDestroyed<Lock>`

[Yusuke Suzuki]

Created attachment 397601 [details]
Patch

[Darin Adler]

Comment on attachment 397601 [details]
Patch

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

> Source/WTF/ChangeLog:8
> +        Lock can be static-initialized since it has constexpr constructor. No need to use NeverDestroyed<Lock>.

Patch is correct, but this comment is wrong.

It’s true that we don’t need NeverDestroyed with Lock! But it has nothing to do with the constructor. NeverDestroyed is about the *destructor*.

The constexpr constructor means we can have global initialized Lock objects outside a function. Not what we are changing here, though.

[Yusuke Suzuki]

Comment on attachment 397601 [details]
Patch

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

>> Source/WTF/ChangeLog:8
>> +        Lock can be static-initialized since it has constexpr constructor. No need to use NeverDestroyed<Lock>.
> 
> Patch is correct, but this comment is wrong.
> 
> It’s true that we don’t need NeverDestroyed with Lock! But it has nothing to do with the constructor. NeverDestroyed is about the *destructor*.
> 
> The constexpr constructor means we can have global initialized Lock objects outside a function. Not what we are changing here, though.

There is constructor difference between `static NeverDestroyed<Lock>` and `static Lock`. The problem is that NeverDestroyed's constructor is not constexpr. This means that this constructor is dynamic, and called when this static local variable is first initialized.
And since we are disabling C++ thread guard around static variable access, this initialization is potentially racy.

On the other hand, Lock's constructor is constexpr. This means that this object construction is following constant initialization[1].
In practice, this is initialized at compile time[2]. But, IIUC, strictly speaking, it is still possible that it is initialized when we enter this function's scope first, and in that case, it is racy (but clang and gcc are not at least).
Maybe, the best standard compliant way is putting global variable, `static Lock observerLock`. And in that way, we can ensure that this initialization must happen before the main function. And, since we are disabling global-constructors, we can ensure that all of them are initialized already when WebKit code runs. And in practice, all of them are const-initialized and compile-time-initialized. So when loading an image, initialization is already done.

Maybe, should I put them as a global variable instead?

[1]: https://en.cppreference.com/w/cpp/language/constant_initialization
[2]: https://en.cppreference.com/w/cpp/language/storage_duration

[Yusuke Suzuki]

Comment on attachment 397601 [details]
Patch

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

>>> Source/WTF/ChangeLog:8
>>> +        Lock can be static-initialized since it has constexpr constructor. No need to use NeverDestroyed<Lock>.
>> 
>> Patch is correct, but this comment is wrong.
>> 
>> It’s true that we don’t need NeverDestroyed with Lock! But it has nothing to do with the constructor. NeverDestroyed is about the *destructor*.
>> 
>> The constexpr constructor means we can have global initialized Lock objects outside a function. Not what we are changing here, though.
> 
> There is constructor difference between `static NeverDestroyed<Lock>` and `static Lock`. The problem is that NeverDestroyed's constructor is not constexpr. This means that this constructor is dynamic, and called when this static local variable is first initialized.
> And since we are disabling C++ thread guard around static variable access, this initialization is potentially racy.
> 
> On the other hand, Lock's constructor is constexpr. This means that this object construction is following constant initialization[1].
> In practice, this is initialized at compile time[2]. But, IIUC, strictly speaking, it is still possible that it is initialized when we enter this function's scope first, and in that case, it is racy (but clang and gcc are not at least).
> Maybe, the best standard compliant way is putting global variable, `static Lock observerLock`. And in that way, we can ensure that this initialization must happen before the main function. And, since we are disabling global-constructors, we can ensure that all of them are initialized already when WebKit code runs. And in practice, all of them are const-initialized and compile-time-initialized. So when loading an image, initialization is already done.
> 
> Maybe, should I put them as a global variable instead?
> 
> [1]: https://en.cppreference.com/w/cpp/language/constant_initialization
> [2]: https://en.cppreference.com/w/cpp/language/storage_duration

Maybe, this is OK. static-local-variable's declaration scheme only happens when the initialization is not zero-initialized / constant-initialized. This means that this static local variables are also initialized before program starts anyway.

[Yusuke Suzuki]

Comment on attachment 397601 [details]
Patch

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

>>>> Source/WTF/ChangeLog:8
>>>> +        Lock can be static-initialized since it has constexpr constructor. No need to use NeverDestroyed<Lock>.
>>> 
>>> Patch is correct, but this comment is wrong.
>>> 
>>> It’s true that we don’t need NeverDestroyed with Lock! But it has nothing to do with the constructor. NeverDestroyed is about the *destructor*.
>>> 
>>> The constexpr constructor means we can have global initialized Lock objects outside a function. Not what we are changing here, though.
>> 
>> There is constructor difference between `static NeverDestroyed<Lock>` and `static Lock`. The problem is that NeverDestroyed's constructor is not constexpr. This means that this constructor is dynamic, and called when this static local variable is first initialized.
>> And since we are disabling C++ thread guard around static variable access, this initialization is potentially racy.
>> 
>> On the other hand, Lock's constructor is constexpr. This means that this object construction is following constant initialization[1].
>> In practice, this is initialized at compile time[2]. But, IIUC, strictly speaking, it is still possible that it is initialized when we enter this function's scope first, and in that case, it is racy (but clang and gcc are not at least).
>> Maybe, the best standard compliant way is putting global variable, `static Lock observerLock`. And in that way, we can ensure that this initialization must happen before the main function. And, since we are disabling global-constructors, we can ensure that all of them are initialized already when WebKit code runs. And in practice, all of them are const-initialized and compile-time-initialized. So when loading an image, initialization is already done.
>> 
>> Maybe, should I put them as a global variable instead?
>> 
>> [1]: https://en.cppreference.com/w/cpp/language/constant_initialization
>> [2]: https://en.cppreference.com/w/cpp/language/storage_duration
> 
> Maybe, this is OK. static-local-variable's declaration scheme only happens when the initialization is not zero-initialized / constant-initialized. This means that this static local variables are also initialized before program starts anyway.

Yes, static-local-variable is OK. constant-initialization initializes variable's storage with constant value instead of zero-initialization (which is done for usual static local variables). So yeah, before entering this function, this is already initialized with constexpr Lock's result.

[Darin Adler]

Comment on attachment 397601 [details]
Patch

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

>>> Source/WTF/ChangeLog:8
>>> +        Lock can be static-initialized since it has constexpr constructor. No need to use NeverDestroyed<Lock>.
>> 
>> Patch is correct, but this comment is wrong.
>> 
>> It’s true that we don’t need NeverDestroyed with Lock! But it has nothing to do with the constructor. NeverDestroyed is about the *destructor*.
>> 
>> The constexpr constructor means we can have global initialized Lock objects outside a function. Not what we are changing here, though.
> 
> There is constructor difference between `static NeverDestroyed<Lock>` and `static Lock`. The problem is that NeverDestroyed's constructor is not constexpr. This means that this constructor is dynamic, and called when this static local variable is first initialized.
> And since we are disabling C++ thread guard around static variable access, this initialization is potentially racy.
> 
> On the other hand, Lock's constructor is constexpr. This means that this object construction is following constant initialization[1].
> In practice, this is initialized at compile time[2]. But, IIUC, strictly speaking, it is still possible that it is initialized when we enter this function's scope first, and in that case, it is racy (but clang and gcc are not at least).
> Maybe, the best standard compliant way is putting global variable, `static Lock observerLock`. And in that way, we can ensure that this initialization must happen before the main function. And, since we are disabling global-constructors, we can ensure that all of them are initialized already when WebKit code runs. And in practice, all of them are const-initialized and compile-time-initialized. So when loading an image, initialization is already done.
> 
> Maybe, should I put them as a global variable instead?
> 
> [1]: https://en.cppreference.com/w/cpp/language/constant_initialization
> [2]: https://en.cppreference.com/w/cpp/language/storage_duration

OK, interesting.

1) It’s probably fine to change all of these, as long as Lock does not have a destructor that needs to be called at exist time then it might not be good. We don’t want to have any wasteful exit-time destructors. I think we pass -Wexit-time-destructors so we’ll get a warning if we have one, so if this compiles and links it seems unlikely there is a problem here. Again, whether it’s OK to *drop* NeverDestroyed or not has to do with the *destructor*, not the *constructor*.

2) As far as the *constructor* is concerned, as you said, in practice there is no bug here, since NeverDestroyed<Lock> does result in constant initialization in practice even though the standard doesn’t guarantee it will be non-racy. On the other hand, I’m not sure the *standard* is an issue because our disabling of C++ thread guards around static variable access isn’t really "standard" anyway. I think this is sort of a "whole lot of nothing".

3) Maybe we can fix NeverDestroyed so that when its class template argument has a constexpr constructor, the NeverDestroyed class also has one. Seems poissble we can specialize the template in the right way. It never occurred to me before to try it.

4) We may want some kind of static assertion we can write if we are assuming something has no initialization code to get thread safety. We could possibly package that assertion up into a class template like NeverDestroyed so it’s easy to write something like this:

    static ConstantConstructed<Lock> heapLock;

Otherwise it seems really easy to accidentally break assumptions like this.

[Yusuke Suzuki]

Comment on attachment 397601 [details]
Patch

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

>>>>>> Source/WTF/ChangeLog:8
>>>>>> +        Lock can be static-initialized since it has constexpr constructor. No need to use NeverDestroyed<Lock>.
>>>>> 
>>>>> Patch is correct, but this comment is wrong.
>>>>> 
>>>>> It’s true that we don’t need NeverDestroyed with Lock! But it has nothing to do with the constructor. NeverDestroyed is about the *destructor*.
>>>>> 
>>>>> The constexpr constructor means we can have global initialized Lock objects outside a function. Not what we are changing here, though.
>>>> 
>>>> There is constructor difference between `static NeverDestroyed<Lock>` and `static Lock`. The problem is that NeverDestroyed's constructor is not constexpr. This means that this constructor is dynamic, and called when this static local variable is first initialized.
>>>> And since we are disabling C++ thread guard around static variable access, this initialization is potentially racy.
>>>> 
>>>> On the other hand, Lock's constructor is constexpr. This means that this object construction is following constant initialization[1].
>>>> In practice, this is initialized at compile time[2]. But, IIUC, strictly speaking, it is still possible that it is initialized when we enter this function's scope first, and in that case, it is racy (but clang and gcc are not at least).
>>>> Maybe, the best standard compliant way is putting global variable, `static Lock observerLock`. And in that way, we can ensure that this initialization must happen before the main function. And, since we are disabling global-constructors, we can ensure that all of them are initialized already when WebKit code runs. And in practice, all of them are const-initialized and compile-time-initialized. So when loading an image, initialization is already done.
>>>> 
>>>> Maybe, should I put them as a global variable instead?
>>>> 
>>>> [1]: https://en.cppreference.com/w/cpp/language/constant_initialization
>>>> [2]: https://en.cppreference.com/w/cpp/language/storage_duration
>>> 
>>> Maybe, this is OK. static-local-variable's declaration scheme only happens when the initialization is not zero-initialized / constant-initialized. This means that this static local variables are also initialized before program starts anyway.
>> 
>> Yes, static-local-variable is OK. constant-initialization initializes variable's storage with constant value instead of zero-initialization (which is done for usual static local variables). So yeah, before entering this function, this is already initialized with constexpr Lock's result.
> 
> OK, interesting.
> 
> 1) It’s probably fine to change all of these, as long as Lock does not have a destructor that needs to be called at exist time then it might not be good. We don’t want to have any wasteful exit-time destructors. I think we pass -Wexit-time-destructors so we’ll get a warning if we have one, so if this compiles and links it seems unlikely there is a problem here. Again, whether it’s OK to *drop* NeverDestroyed or not has to do with the *destructor*, not the *constructor*.
> 
> 2) As far as the *constructor* is concerned, as you said, in practice there is no bug here, since NeverDestroyed<Lock> does result in constant initialization in practice even though the standard doesn’t guarantee it will be non-racy. On the other hand, I’m not sure the *standard* is an issue because our disabling of C++ thread guards around static variable access isn’t really "standard" anyway. I think this is sort of a "whole lot of nothing".
> 
> 3) Maybe we can fix NeverDestroyed so that when its class template argument has a constexpr constructor, the NeverDestroyed class also has one. Seems poissble we can specialize the template in the right way. It never occurred to me before to try it.
> 
> 4) We may want some kind of static assertion we can write if we are assuming something has no initialization code to get thread safety. We could possibly package that assertion up into a class template like NeverDestroyed so it’s easy to write something like this:
> 
>     static ConstantConstructed<Lock> heapLock;
> 
> Otherwise it seems really easy to accidentally break assumptions like this.

1) Yes, WTF::Lock is designed to have constexpr constructor and no destructor. This allows putting Lock as global variables (and we are placing it actually). We can place use it as the same we do like `static std::once_flag onceKey`. WTF::Condition also has same design.
2) I hope so... But I'm not confident about whether Lock initialization (which will be invoked by NeverDestroyed<>'s placement new) does not happen dynamically. If it happens dynamically, it is still possible that this constructor clears the value to zero while the other thread already takes it and put some bits on this variable. So, I think `static Lock` is safer approach.
3) That sounds super interesting, though I don't know how to determine whether the given Type T is constant-initialize-able...
4) C++20 constinit is the feature for this purpose! (https://en.cppreference.com/w/cpp/language/constinit)

[Darin Adler]

I wonder how close is_trivially_constructible is. Probably not close enough.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #8)
> I wonder how close is_trivially_constructible is. Probably not close enough.

Yes, std::is_trivially_constructible requires that field is left non-initialized, this is further restricted one compared to constexpr-constructor. And WTF::Lock requires field word is initialized with zero. So, std::is_trivially_constructible_v<Lock> is false.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #9)
> (In reply to Darin Adler from comment #8)
> > I wonder how close is_trivially_constructible is. Probably not close enough.
> 
> Yes, std::is_trivially_constructible requires that field is left
> non-initialized, this is further restricted one compared to
> constexpr-constructor. And WTF::Lock requires field word is initialized with
> zero. So, std::is_trivially_constructible_v<Lock> is false.

I think, until getting C++20 constinit, warning via Lint would be the best.
I'll add a lint warning in https://bugs.webkit.org/show_bug.cgi?id=211054

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

[Yusuke Suzuki]

(In reply to EWS from comment #11)
> ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Oops.

[Yusuke Suzuki]

Committed r260731: <https://trac.webkit.org/changeset/260731>

[Radar WebKit Bug Importer]

<rdar://problem/62408494>