[Patch] Clang: allow scoped_lockable to acquire multiple locks

[Delesley Hutchins]

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.

Another minor nit is that underlying() returns a pointer.  If it can never
be null, then it should return a reference.

Other than that, this patch looks good to me.  Thanks!  I'll defer to Aaron
regarding all style issues.  However, Ed is right that it doesn't matter
whether the guard object is exclusive or shared.  I also don't think
there's a problem with the corner case.  If there are no underlying locks,
then the correct behavior is to lock the guard object on construction, and
unlock it on destruction, which is exactly what this patch does, and what
the original corner case was designed to handle.

  -DeLesley



On Tue, Sep 2, 2014 at 7:20 AM, Ed Schouten <ed at 80386.nl> wrote:

> Hi Aaron,
>
> First of all, thanks for your comments! I'll address the formatting
> issues and the crash bug ASAP. Weird that it didn't do this here.
>
> On 2 September 2014 14:56, Aaron Ballman <aaron at aaronballman.com> wrote:
> > This adds all locks to the analyzer as exclusive locks, but the
> > previous code would handled shared locks separately. Is that
> > intentional?
>
> So the intent behind this change is that regardless of the number of
> locks that are picked up by the guard object, it is tracked as always
> single acquisition of the lock guard object itself. The lock on the
> guard object will reference all of the actual underlying locks that
> are performed, that are either locked exclusively or shared.
>
> In short, to my knowledge it is irrelevant whether the lock on the
> guard object is shared or exclusive. The guard object gets locked upon
> construction and unlocked upon destruction. This is why I just picked
> exclusive.
>
> Now that you mention it, this change will indeed break if there are no
> underlying locks. The set of locks will be empty and our check against
> empty() to determine whether we are a scoped guard object or not will
> be incorrect.
>
> You know what? Let me take a look at the existing FactEntry class and
> see if we can introduce some polymorphism here. Ideally, we should
> treat "lockable" and "scoped_lockable" objects completely separately.
>
> --
> 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/01fd0c3e/attachment.html>