Lock annotations in chromium?

[Anand]

I've looked a bit and it doesn't look like lock annotations (http://clang.llvm.org/docs/ThreadSafetyAnalysis.html) are available in chromium. There's a copy in tcmalloc and webrtc, but that's about it. I know that chromium generally shies away from locks and threading in favour of posted tasks, but the code base I'm working on (Mojo) is designed to be multi-threaded.

Is there are reason why these annotations are unavailable? And if not, is there any opposition in adding them?

[Nico Weber]

I think jamesr once looked at this and figured that this is blocked by libc++ not having these annotations. So he made a patch, but it's not getting reviewed for some reason: http://reviews.llvm.org/D14731

Also, I think we generally believe in dynamic instrumentation (tsan etc) more than in static annotations for locks, memory safety, and so on.

On Thu, Mar 10, 2016 at 6:41 PM, Anand <ami...@chromium.org> wrote:

I've looked a bit and it doesn't look like lock annotations (http://clang.llvm.org/docs/ThreadSafetyAnalysis.html) are available in chromium. There's a copy in tcmalloc and webrtc, but that's about it. I know that chromium generally shies away from locks and threading in favour of posted tasks, but the code base I'm working on (Mojo) is designed to be multi-threaded.

Is there are reason why these annotations are unavailable? And if not, is there any opposition in adding them?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[James Robinson]

At least that's true for code that is directly using the standard library primitives directly like std::thread, std::mutex, etc.  I think most Chromium code would use the //base wrappers which could be annotated if that was deemed useful.

- James

[Anand K. Mistry]

The code I'm working on uses base::Lock, which could trivially be annotated. Although tsan will catch a lot of the same issues, it would be nice to have compile-time checking to help reduce mistakes.

[Peter Kasting]

On Thu, Mar 10, 2016 at 3:45 PM, Nico Weber <tha...@chromium.org> wrote:

I think jamesr once looked at this and figured that this is blocked by libc++ not having these annotations. So he made a patch, but it's not getting reviewed for some reason: http://reviews.llvm.org/D14731

Looks like maybe this just fell off the one developer's radar and a small ping on it would work?

PK 

[Nico Weber]

On Thu, Mar 10, 2016 at 7:24 PM, Anand K. Mistry <ami...@chromium.org> wrote:

The code I'm working on uses base::Lock, which could trivially be annotated. Although tsan will catch a lot of the same issues, it would be nice to have compile-time checking to help reduce mistakes.

I'm not opposed to experimentally adding them in base and collecting data on how useful they are in practice. (Assuming they don't slow down compiles, but I don't think they do.)

[Nico Weber]

I pinged a libc++ owner and they left a few comments. If the patch author hasn't lost interest in the meantime, maybe the patch can proceed now :-)

One concern on the review was that the lock annotation warnings might not be super polished -- Anand, did you try adding them locally and see if they work well enough for production use? 

[James Robinson]

On Fri, Mar 11, 2016 at 8:56 AM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Mar 10, 2016 at 8:25 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Mar 10, 2016 at 3:45 PM, Nico Weber <tha...@chromium.org> wrote:

I think jamesr once looked at this and figured that this is blocked by libc++ not having these annotations. So he made a patch, but it's not getting reviewed for some reason: http://reviews.llvm.org/D14731

Looks like maybe this just fell off the one developer's radar and a small ping on it would work?

I pinged a libc++ owner and they left a few comments. If the patch author hasn't lost interest in the meantime, maybe the patch can proceed now :-)

It may proceed, but even if it does I don't think it will help Anand out the annotations in libc++ would only apply to the C++ standard library synchronization types which Chromium code doesn't use.  For Anand's use case what would be useful would be adding annotations to //base/synchronization/lock.h and friends.

 

One concern on the review was that the lock annotation warnings might not be super polished -- Anand, did you try adding them locally and see if they work well enough for production use? 

In another project we're using the annotations ironically enough for implementing Mojo primitives and are pretty happy with them.  In the Chromium project, WebRTC and tcmalloc have annotations in place and at least the WebRTC annotations checks are enabled in the build today from what I can tell:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/BUILD.gn&q=Wthread-safety&sq=package:chromium&type=cs&l=116

so the Chromium build already depends on these annotations working at least to some extent.

If you're a Googler you can take a look at the internal uses of these annotations.

In my personal experience the annotations are very useful for simple synchronization patterns such as a member variable guarded unconditionally by a mutex and don't handle complex conditional logic such as a member variable that is sometimes accessed under a lock and sometimes outside but the latter is very uncommon compared to the former (and even less commonly correct) so the extra annotations required for complex cases is well worth it.  Aside from the static checking, having a consistent way to denote the synchronization rules is pretty valuable.  This is being done in comments in various places in the Chromium codebase such as https://code.google.com/p/chromium/codesearch#chromium/src/media/midi/midi_manager_win.cc&q=//%20GUARDED_BY%20exact:yes&sq=package:chromium&type=cs&l=1099 but the tool checking makes this documentation much more valuable.

- James

[Dmitry Skiba]

Just another data point: ART also uses lock annotations: https://android.googlesource.com/platform/art/+/master/runtime/base/macros.h#260

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Nico Weber]

On Fri, Mar 11, 2016 at 2:02 PM, James Robinson <jam...@chromium.org> wrote:

On Fri, Mar 11, 2016 at 8:56 AM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Mar 10, 2016 at 8:25 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Mar 10, 2016 at 3:45 PM, Nico Weber <tha...@chromium.org> wrote:

I think jamesr once looked at this and figured that this is blocked by libc++ not having these annotations. So he made a patch, but it's not getting reviewed for some reason: http://reviews.llvm.org/D14731

Looks like maybe this just fell off the one developer's radar and a small ping on it would work?

I pinged a libc++ owner and they left a few comments. If the patch author hasn't lost interest in the meantime, maybe the patch can proceed now :-)

It may proceed, but even if it does I don't think it will help Anand out the annotations in libc++ would only apply to the C++ standard library synchronization types which Chromium code doesn't use.  For Anand's use case what would be useful would be adding annotations to //base/synchronization/lock.h and friends.

Right, but if base has these annotations and the C++ lib doesn't, then we might be sad if one day we decide to use the C++ standard lib locking primitives.

 

 

One concern on the review was that the lock annotation warnings might not be super polished -- Anand, did you try adding them locally and see if they work well enough for production use? 

In another project we're using the annotations ironically enough for implementing Mojo primitives and are pretty happy with them.  In the Chromium project, WebRTC and tcmalloc have annotations in place and at least the WebRTC annotations checks are enabled in the build today from what I can tell:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/BUILD.gn&q=Wthread-safety&sq=package:chromium&type=cs&l=116

so the Chromium build already depends on these annotations working at least to some extent.

If you're a Googler you can take a look at the internal uses of these annotations.

In my personal experience the annotations are very useful for simple synchronization patterns such as a member variable guarded unconditionally by a mutex and don't handle complex conditional logic such as a member variable that is sometimes accessed under a lock and sometimes outside but the latter is very uncommon compared to the former (and even less commonly correct) so the extra annotations required for complex cases is well worth it.  Aside from the static checking, having a consistent way to denote the synchronization rules is pretty valuable.  This is being done in comments in various places in the Chromium codebase such as https://code.google.com/p/chromium/codesearch#chromium/src/media/midi/midi_manager_win.cc&q=//%20GUARDED_BY%20exact:yes&sq=package:chromium&type=cs&l=1099 but the tool checking makes this documentation much more valuable.

Cool, thanks, that's good to hear :-) 

[Anand]

On Saturday, March 12, 2016 at 6:41:17 AM UTC+11, Nico Weber wrote:

On Fri, Mar 11, 2016 at 2:02 PM, James Robinson <jam...@chromium.org> wrote:

On Fri, Mar 11, 2016 at 8:56 AM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Mar 10, 2016 at 8:25 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Mar 10, 2016 at 3:45 PM, Nico Weber <tha...@chromium.org> wrote:

I think jamesr once looked at this and figured that this is blocked by libc++ not having these annotations. So he made a patch, but it's not getting reviewed for some reason: http://reviews.llvm.org/D14731

Looks like maybe this just fell off the one developer's radar and a small ping on it would work?

I pinged a libc++ owner and they left a few comments. If the patch author hasn't lost interest in the meantime, maybe the patch can proceed now :-)

It may proceed, but even if it does I don't think it will help Anand out the annotations in libc++ would only apply to the C++ standard library synchronization types which Chromium code doesn't use.  For Anand's use case what would be useful would be adding annotations to //base/synchronization/lock.h and friends.

Right, but if base has these annotations and the C++ lib doesn't, then we might be sad if one day we decide to use the C++ standard lib locking primitives.

The common usage of them is in terms of macros which are no-op for non-supporting compilers. If we need to, they can be turned off until libc++ is updated (I assume it will be at some point).

 

 

 

One concern on the review was that the lock annotation warnings might not be super polished -- Anand, did you try adding them locally and see if they work well enough for production use? 

I haven't added them to chromium to check. But they're used in the github Mojo project and are effective there. And I've used them internally which is why I'm requesting them here.

[James Robinson]

These annotations are available in libcxx as of r263611, guarded behind a macro.

- James