[Patch] Clang: allow scoped_lockable to acquire multiple locks

[Aaron Ballman]

On Tue, Sep 2, 2014 at 3:12 PM, Ed Schouten <ed at 80386.nl> wrote:
> On 2 September 2014 20:56, Delesley Hutchins <delesley at google.com> wrote:
>> The crash is probably being caused by the fact that in removeLock(), you are
>> trying to remove the guard object multiple times.  The call to
>> FSet.removeLock(FactMan, Cp);  should happen at the end of the loop, if
>> FullyRemove is true.  Once that's moved out of the loop, then you can
>> simplify some of the if-logic inside the loop as well.  Ideally there
>> shouldn't be a crash, though; you should get a double-release warning, so
>> you may have uncovered a latent bug somewhere else.
>
> Ah, good catch. Fixed. Thanks!
>
> One thing that made me slightly unhappy about my previous version of
> the patch is that it actually causes a strong increase in code
> duplication and actually makes the intersectAndWarn() function pretty
> hard to understand. In an attempt to solve this, I have done the
> following (also attached):
>
> http://80386.nl/pub/20140902-clang-scoped_lockable.txt
>
> Essentially, it does the following:
>
> - It subclasses the FactEntry into LockableFactEntry and
> ScopedLockableFactEntry. For ScopedLockableFactEntry it forces
> LK_Exclusive and !Asserted. The Managed field can be moved into
> LockableFactEntry.
>
> - By making FactEntry abstract, we need to patch up certain pieces of
> code to use std::unique_ptr, llvm:make_unique, etc.
>
> - To FactEntry, it adds handleRemovalFromIntersection(). This function
> implements a single-directed removal as done by intersectAndWarn().
> The structure of intersectAndWarn() now becomes obvious, as we can use
> this operation in both directions now. While there, it fixes a LEK1 vs
> LEK2 mixup.
>
> - Similar to handleRemovalFromIntersection(), it adds handleUnlock(),
> which is called by removeLock().
>
> Aaron: regarding style: you mentioned clang-format. Do we like
> clang-format enough nowadays that I can just reformat the entire file?
> If so, shall I perform a separate commit to reformat ThreadSafety.cpp
> and rebase the change on top of that?

I would not reformat the entire file at this point (that tends to be
pretty noisy for little gain). Instead, you can run clang-format over
the patch file itself. There's a pretty simple script for doing this
described in the documentation:
http://clang.llvm.org/docs/ClangFormat.html

Thanks!

~Aaron