[Patch] Clang: allow scoped_lockable to acquire multiple locks

[Delesley Hutchins]

I think this is a generally useful refactoring, especially since it lays
the groundwork for adding different kinds of FactEntries; Thanks!  Two
comments:

(1) FactEntry needs a virtual destructor to avoid memory leaks.
(2) I would like the different FactEntry classes to be declared together
(at the top of the file), rather than split as they are currently.

Other than that, looks good.

  -DeLesley



On Tue, Sep 2, 2014 at 12: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?
>
> --
> Ed Schouten <ed at 80386.nl>
>



-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140902/860ec5f3/attachment.html>