Bug 221614 – Enable -Wthread-safety, add attributes to custom lock classes, and provide macros to declare guards

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Kilzer (:ddkilzer)]

Enable -Wthread-safety, add attributes to custom lock classes, and provide macros to declare guards.

Instrumenting various variables should probably be done on an individual basis.

For more info, see:  <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html>

[David Kilzer (:ddkilzer)]

Any opinions on using this?  The warnings are purely a compile-time feature (no runtime overhead), and it relies on attributes being added to existing lock classes and to the variables that need locking.

[Kimmo Kinnunen]

I’ve used these successfully in chromium related projects..

[Darin Adler]

Let’s try it. I can’t tell how well it will work from the description alone.

[Radar WebKit Bug Importer]

<rdar://problem/74396781>

[Kimmo Kinnunen]

Created attachment 425770 [details]
Patch

[Kimmo Kinnunen]

Created attachment 425775 [details]
Patch

[Kimmo Kinnunen]

Created attachment 425777 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 425770 [details]
Patch

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

r- to remove the "done-static-lock" text from ChangeLogs, fix Copyright blocks, and to figure out a better name than StaticLock/StaticLocker.

I strongly recommend writing a check-webkit-style checker, too.  (I can help with that if you give me some examples of old/new style.)  Maybe file a separate bug for that since it doesn't need to block landing this.

> Source/JavaScriptCore/ChangeLog:9
> +        done-static-lock

This text doesn't seem to serve any meaningful purpose, so it should be removed from all of the ChangeLogs.

> Source/WTF/ChangeLog:21
> +        * wtf/StaticLock.h: Added.
> +        * wtf/StaticLocker.h: Added.
> +        Add StaticLock and StaticLocker variants that are amendable to static
> +        analysis.

I assume "Static" in the class names comes from "static analysis", but "static" has too many other meanings that make the class name confusing.

We should try to pick a better name; maybe event rename Lock/Locker to DeprecatedLock/DeprecatedLocker and call this one Lock/Locker, if folks are agreeable with that direction.  (See Tools/Scripts/do-webcore-rename for details about mass renaming.)

Or in the meantime, maybe something like CheckedLock/CheckedLocker would be better since the compiler can check its usage?  If we intend to replace Lock/Locker, though, we should rename it to add the "Deprecated" prefix.

> Source/WTF/ChangeLog:25
> +        StaticLocker is a std::scoped_lock. The scoped_lock cannot be aliased,
> +        since it appears that (Apple's) libcxx is not compiled with thread safety
> +        analysis support enabled by default.

We should have a radar tracking this enablement internally.

> Source/WTF/ChangeLog:36
> +        New types are needed due Locker move constructor and conditional locking.
> +        The Locker has default usage pattern of:
> +          auto locker = holdLock(m_lock);
> +        This forces dynamism that removes the possibility of simple statical
> +        analysis that thread safety analysis capabilities "mutex" and "scoped_lock"
> +        currently implement. Most likely large fraction of call sites is due to historical
> +        lack of CTAD and as such can be converted to less general form.
> +        Once the pattern is not used by default, StaticLock/StaticLocker can be deleted
> +        and the move dynamism bits of Locker can be moved to some more specific type
> +        ("DynamicLocker").

We should also write a check-webkit-style checker that warns against added this old code pattern if we eventually want to replace it.

A nice benefit of this is that it will prevent adding even more "old-style" locks, and check-webkit-style can be run on the entire WebKit code base to find out how many old-style locks exist.

> Source/WTF/wtf/StaticLock.h:61
> +    bool try_lock() WTF_ACQUIRES_LOCK_IF(true) { return Lock::try_lock(); } // NOLINT

What does "NOLINT" do here?

> Source/WTF/wtf/StaticLocker.h:26
> + * Copyright (C) 2021 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

This is actually the old-style BSD copyright statement since it has 3 numbered items.  Use the one from Source/WebCore/LICENSE-APPLE instead.

> Source/WTF/wtf/ThreadSafetyAnalysis.h:26
> + * Copyright (C) 2021 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Ditto.  This should use text from Source/WebCore/LICENSE-APPLE instead.

[Ryosuke Niwa]

(In reply to David Kilzer (:ddkilzer) from comment #8)
>
> I assume "Static" in the class names comes from "static analysis", but
> "static" has too many other meanings that make the class name confusing.
> 
> We should try to pick a better name; maybe event rename Lock/Locker to
> DeprecatedLock/DeprecatedLocker and call this one Lock/Locker, if folks are
> agreeable with that direction.  (See Tools/Scripts/do-webcore-rename for
> details about mass renaming.)

I suspect JSC probably won't be able to enable this check in all the places so I don't think that's quite right.

> Or in the meantime, maybe something like CheckedLock/CheckedLocker would be
> better since the compiler can check its usage?  If we intend to replace
> Lock/Locker, though, we should rename it to add the "Deprecated" prefix.

Yeah, I like this direction.

[Kimmo Kinnunen]

Created attachment 425856 [details]
Patch

[Kimmo Kinnunen]

Thanks for looking at this.

I removed CheckedLocker altogether. Now it's just CheckedLock.
Also added a (positive) compile check.

(In reply to David Kilzer (:ddkilzer) from comment #8)
> We should also write a check-webkit-style checker that warns against added
> this old code pattern if we eventually want to replace it.

I think the good strategy is to just remove the "old" pattern, it's not useful.
E.g. replace:
  auto locker = holdLock(...);
with:
  Locker locker { ... };

I'll file a bug or two about this at my leisure.

Once all sites are modified, holdLock can be removed and there's no old pattern to check style against.

Then we can convert neccessary sites from:
Replace:
  Lock m_lock;
With:
  CheckedLock m_lock;

Later, we can discuss about renaming:
Lock -> UncheckedLock
CheckedLock -> Lock

So ultimately the pattern should be:
UncheckedLock m_uncheckedLock;
Locker movableLocker { m_uncheckedLock }; // for JSC where we move the ownership of the locker.

Lock m_lock;
Locker locker { m_lock }; // For almost all cases.



> > Source/WTF/wtf/StaticLock.h:61
> > +    bool try_lock() WTF_ACQUIRES_LOCK_IF(true) { return Lock::try_lock(); } // NOLINT
> 
> What does "NOLINT" do here?

check-webkit-style complains about lack_of_camel_case.

[David Kilzer (:ddkilzer)]

(In reply to Kimmo Kinnunen from comment #11)
> Thanks for looking at this.
> 
> I removed CheckedLocker altogether. Now it's just CheckedLock.
> Also added a (positive) compile check.
> 
> (In reply to David Kilzer (:ddkilzer) from comment #8)
> > We should also write a check-webkit-style checker that warns against added
> > this old code pattern if we eventually want to replace it.
> 
> I think the good strategy is to just remove the "old" pattern, it's not
> useful.
> E.g. replace:
>   auto locker = holdLock(...);
> with:
>   Locker locker { ... };
> 
> I'll file a bug or two about this at my leisure.
> 
> Once all sites are modified, holdLock can be removed and there's no old
> pattern to check style against.
> 
> Then we can convert neccessary sites from:
> Replace:
>   Lock m_lock;
> With:
>   CheckedLock m_lock;
> 
> Later, we can discuss about renaming:
> Lock -> UncheckedLock
> CheckedLock -> Lock
> 
> So ultimately the pattern should be:
> UncheckedLock m_uncheckedLock;
> Locker movableLocker { m_uncheckedLock }; // for JSC where we move the
> ownership of the locker.
> 
> Lock m_lock;
> Locker locker { m_lock }; // For almost all cases.

I like this strategy!

> > > Source/WTF/wtf/StaticLock.h:61
> > > +    bool try_lock() WTF_ACQUIRES_LOCK_IF(true) { return Lock::try_lock(); } // NOLINT
> > 
> > What does "NOLINT" do here?
> 
> check-webkit-style complains about lack_of_camel_case.

This isn't commonly used (other than in Google sources we've imported), but I guess it's okay.  (We could also update webkit-check-style, but maybe it's not worth it.)

$ grep -l -r NOLINT Source | grep -v Source/ThirdParty
Source/WTF/wtf/dtoa/utils.h
Source/WTF/wtf/dtoa/double-conversion.cc
Source/WTF/icu/unicode/numberformatter.h
Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.h
Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.cpp

[David Kilzer (:ddkilzer)]

Comment on attachment 425856 [details]
Patch

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

r=me with the following changes:
- License header fix.
- GTK/Windows build fixes.
- Fix spelling of "amenable".

> Source/WTF/ChangeLog:22
> +        Add CheckedLock, a Lock variant that is amendable to static
> +        analysis.
> +        Add a Locker specialization for CheckedLock that is amendable to
> +        static analysis.

Spelling:  amendable => amenable  (no "d")

> Source/WTF/wtf/ThreadSafetyAnalysis.h:15
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.

This needs to be updated to use Source/WebCore/LICENSE-APPLE.

> Source/WTF/wtf/ThreadSafetyAnalysis.h:32
> +// https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

Nit: Make this a statement; something like:

// See <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html> for details.

> Tools/TestWebKitAPI/Tests/WTF/CheckedLock.cpp:28
> +#include <wtf/CheckedLock.h> // NOLINT: Avoid problem where webkit-check-style thinks this test file is the implementation file.

Nit: We could rename this file to CheckedLockTest.cpp so we don't need the NOLINT comment.  (Wish we followed this naming convention more often.)

[Kimmo Kinnunen]

Created attachment 426016 [details]
Patch

[Darin Adler]

Comment on attachment 426016 [details]
Patch

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

> Source/WTF/wtf/ThreadSafetyAnalysis.h:38
> +#define WTF_CAPABILITY_LOCK WTF_THREAD_ANNOTATION_ATTRIBUTE(capability("mutex"))
> +
> +#define WTF_CAPABILITY_SCOPED_LOCK WTF_THREAD_ANNOTATION_ATTRIBUTE(scoped_lockable)

Would like this file better without the blank lines between each #define, better if sorted in some logical order or just alphabetically, and better if grouped somehow if it makes the sorting easier to understand.

[Kimmo Kinnunen]

Created attachment 426080 [details]
Patch

[Kimmo Kinnunen]

Created attachment 426091 [details]
Patch

[Kimmo Kinnunen]

(In reply to Darin Adler from comment #15)
> Would like this file better without the blank lines between each #define,
> better if sorted in some logical order or just alphabetically, and better if
> grouped somehow if it makes the sorting easier to understand.

I tried to re-order logically in the "similar role" order. E.g capability declarations, member variable requirement declarations, requirement declarations, acquisition declarations, release declarations, misc. 

I think the order might not have made sense without the docs, so I added some docs in order to avoid having to add the grouping topics as comments. I don't know if the text makes sense ..

[Darin Adler]

(In reply to Kimmo Kinnunen from comment #18)
> I think the order might not have made sense without the docs, so I added
> some docs in order to avoid having to add the grouping topics as comments. I
> don't know if the text makes sense ..

OK. Does seem better.

But kind of the opposite of what I was suggesting.

Since this is simple wrapping of the underlying clang feature, I think a reference to documentation for that would be better than writing our own instructions. But I don’t feel strongly about that.

[Darin Adler]

(In reply to Darin Adler from comment #19)
> I think a
> reference to documentation for that would be better than writing our own
> instructions

And we have that with the link you already included.

I would probably have just sorted alphabetically and not attempted to have our file document these things. Our goal is just to re-export them with a guard that allows them to be used without worrying about compiler support. Maybe some day this gets sophisticated if we have to bridge multiple inconsistent features on different compilers, but at the moment it’s just us re-documenting something, and the reason we have macros is simply ease of use.

[Darin Adler]

Comment on attachment 426091 [details]
Patch

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

Since Dave already reviewed this, no need to set review? unless you want some other type of review or want Dave to review again. I am unclear on what you are after, so I’m not reviewing again.

> Source/JavaScriptCore/Configurations/Base.xcconfig:101
> +WARNING_CFLAGS = -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wconditional-uninitialized -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -Wvla -Wliteral-conversion  -Wthread-safety;

Two spaces here before "-Wthread-safety"

> Source/bmalloc/Configurations/Base.xcconfig:97
> +WARNING_CFLAGS = -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wconditional-uninitialized -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -Wvla -Wliteral-conversion  -Wthread-safety;

Two spaces here.

[Darin Adler]

Looks good to me.

[Kimmo Kinnunen]

Created attachment 426398 [details]
Patch

[Kimmo Kinnunen]

> I would probably have just sorted alphabetically and not attempted to have our file document these things.

Thanks. Reverted to this.

> Our goal is just to re-export them with a guard that allows them to be used without worrying about compiler support. 

But currently the patch does not do only this. It additionally tries to ensure the vocabulary of the macros are from WebKit vocabulary, not from clang docs and not from clang implementation. I can fix this up in a separate patch if it's a problem.

The original docs addressed also:
 * The vocabulary of clang annotations is different from the vocabulary of clang docs, which makes understanding the docs difficult.
 * The vocabulary of clang docs and WebKit is different, making it difficult to associate the docs concepts with WebKit concepts.

However, I'm sure the differences are minor and don't cause trouble.

[EWS]

Committed r276247 (236729@main): <https://commits.webkit.org/236729@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426398 [details].