Bug 226573 – [WTF][GStreamer] Enforce clang thread safety annotations

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Alicia Boya García]

This patch:

(1) Finishes the work done by Chris Dumez in r278248, fixing the
    warnings when using DataMutexLocker::runUnlocked() and
    DataMutex::earlyUnlock().

(2) Fixes almost all* thread safety warnings in platform/graphics/gstreamer.

(3) Introduces WTF_BEGIN_ENFORCE_THREAD_SAFETY_ANALYSIS and
    WTF_END_ENFORCE_THREAD_SAFETY_ANALYSIS macros which turn thread safety
    warnings into compile-time errors when building in clang.

(4) Makes use of WTF_BEGIN_ENFORCE_THREAD_SAFETY_ANALYSIS in almost
    all* files in platform/graphics/gstreamer that make use of locks, and
    in the DataMutex unit test suite.

*Exceptionally, ImageDecoderGStreamer.cpp hasn't been modified to add
the enforce annotation because of race condition present detected by
the analyzer. Fixing that is a separate issue in
https://bugs.webkit.org/show_bug.cgi?id=226495.

[Alicia Boya García]

Created attachment 430454 [details]
Patch

[Alicia Boya García]

Created attachment 430455 [details]
Patch

[Alicia Boya García]

Created attachment 430459 [details]
Patch

[Michael Catanzaro]

(In reply to Alicia Boya García from comment #0)
> (3) Introduces WTF_BEGIN_ENFORCE_THREAD_SAFETY_ANALYSIS and
>     WTF_END_ENFORCE_THREAD_SAFETY_ANALYSIS macros which turn thread safety
>     warnings into compile-time errors when building in clang.

So is it basically -Werror=thread-safety, but scoped to only GStreamer files. (Apple won't use it, since they already build everything with -Werror.)

It would be obsoleted by bug #155047 if we turn on -Werror in developer mode, right?

[Alicia Boya García]

(In reply to Michael Catanzaro from comment #4)
> (In reply to Alicia Boya García from comment #0)
> > (3) Introduces WTF_BEGIN_ENFORCE_THREAD_SAFETY_ANALYSIS and
> >     WTF_END_ENFORCE_THREAD_SAFETY_ANALYSIS macros which turn thread safety
> >     warnings into compile-time errors when building in clang.
> 
> So is it basically -Werror=thread-safety, but scoped to only GStreamer
> files. (Apple won't use it, since they already build everything with
> -Werror.)
> 
> It would be obsoleted by bug #155047 if we turn on -Werror in developer
> mode, right?

It won't. `thread-safety` is not a warning by default. At the moment, clang raises thread safety issues in many parts of the code. These macros allow to enforce the checks in those files where it has already been checked that it's not a problem and shouldn't be regressed.

Ideally at some point all WebKit would have all the thread-safety warnings fixed and we could build the entire codebase with -Werror=thread-safety.

[Chris Dumez]

Comment on attachment 430459 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        (3) Introduces WTF_BEGIN_ENFORCE_THREAD_SAFETY_ANALYSIS and

I am not convinced we want these particular macros. Their name is a bit confusing since thread safety analysis is always enforced. I'll also note that there is discussion to turn warnings into errors all the time for GTK port.
On Cocoa ports, we treat all warnings as errors already. Doing it at a file level seems odd to me.

[Darin Adler]

(In reply to Chris Dumez from comment #6)
> > Source/WebCore/ChangeLog:16
> > +        (3) Introduces WTF_BEGIN_ENFORCE_THREAD_SAFETY_ANALYSIS and
> 
> I am not convinced we want these particular macros. Their name is a bit
> confusing since thread safety analysis is always enforced. I'll also note
> that there is discussion to turn warnings into errors all the time for GTK
> port.
> On Cocoa ports, we treat all warnings as errors already. Doing it at a file
> level seems odd to me.

I agree with those points.

[Alicia Boya García]

(In reply to Chris Dumez from comment #6)
> On Cocoa ports, we treat all warnings as errors already. Doing it at a file
> level seems odd to me.

I wasn't aware, since I saw a bunch of warnings while building WebKit. If we can get there, that would be better indeed.

[Chris Dumez]

(In reply to Alicia Boya García from comment #8)
> (In reply to Chris Dumez from comment #6)
> > On Cocoa ports, we treat all warnings as errors already. Doing it at a file
> > level seems odd to me.
> 
> I wasn't aware, since I saw a bunch of warnings while building WebKit. If we
> can get there, that would be better indeed.

There is definitely no warnings for Cocoa ports so the only warnings you're seeing are likely specific to the Glib-port. It would indeed be great if those could get fixed.

Note that DataMutex is only used by some gstreamer code and I personally don't think it brings much compared to a separate lock + data members, especially now that we support thread safety annotations. I considered dropping it entirely but it wasn't convenient for me since I cannot build the GTK port and I'd need to clear it with people from that port first.

[Kimmo Kinnunen]

From my perspective (not saying it's would be terribly interesting perspective to GStreamer using ports..): The changelog says what the code does. The code also says what the code does.. The changelog does not say is: why?

More specifically, why cannot the thread safety errors as errors just be enabled by default, business-as-usual, in the project files? First fix the existing bugs, enable the checks and then more annotations can be added later? There isn't "ENFORCE INTS ARE NOT FLOATS WARNING" macros either.

It'd be somewhat simpler for everybody if there was not that many gotchas related to which pieces of code are compiled in which distinct modes..

[Michael Catanzaro]

(In reply to Alicia Boya García from comment #8)
> I wasn't aware, since I saw a bunch of warnings while building WebKit. If we
> can get there, that would be better indeed.

I try to keep the build warning-free with latest GCC on x86_64. Currently the only warnings are covered by bug #226193 and bug #226557.

I intend to turn on -Werror for DEVELOPER_MODE builds in bug #155047 unless somebody objects soon.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #11)
> (In reply to Alicia Boya García from comment #8)
> > I wasn't aware, since I saw a bunch of warnings while building WebKit. If we
> > can get there, that would be better indeed.
> 
> I try to keep the build warning-free with latest GCC on x86_64. Currently
> the only warnings are covered by bug #226193 and bug #226557.
> 
> I intend to turn on -Werror for DEVELOPER_MODE builds in bug #155047 unless
> somebody objects soon.

Looks reasonable to me, moreover when the build environment (mainly
compiler versions) is nowadays the same for all developers thanks to
the Flatpak SDK :)

[Alicia Boya García]

I withdraw the patch. I will move the acceptable corrections to other patches.