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>
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.
I’ve used these successfully in chromium related projects..
Let’s try it. I can’t tell how well it will work from the description alone.
<rdar://problem/74396781>
Created attachment 425770 [details] Patch
Created attachment 425775 [details] Patch
Created attachment 425777 [details] Patch
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.
(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.
Created attachment 425856 [details] Patch
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.
(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
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.)
Created attachment 426016 [details] Patch
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.
Created attachment 426080 [details] Patch
Created attachment 426091 [details] Patch
(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 ..
(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.
(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.
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.
Looks good to me.
Created attachment 426398 [details] Patch
> 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.
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].