[cfe-commits] [PATCH] Thread-safety analysis: handle attributes on constructors

[Delesley Hutchins]

Thanks for the review!

> Do you want to be this liberal? UO_AddrOf and UO_Deref seem like good things
> to handle (and UO_Plus and UO_Extension seem harmless), but
> increment/decrement in a mutex expression seems like something that should be
> blacklisted.

The purpose of buildMutexID is to construct an object that uniquely
identifies a mutex expression, but is easier to compare for equality
than an Expr*.   This should really be a larger case statement which
handles most kinds of Expr; I  just haven't added the full set of
cases yet.

The question of which expressions should be considered valid mutex
expressions is a separate issue.  Ideally mutex expressions should
have no side effects, but full side-effect analysis is well beyond the
scope of the current implementation; merely eliminating UO_PostInc and
friends is not going to accomplish much.  A more pressing concern is
tracking which variables have changed between lock and unlock, because
the mutex expressions should no longer be equal if they've changed; I
will probably want to move this into the static analyzer proper in
order to get that functionality.

I'll add a comment to clarify.

> The prevalent style in clang is to omit the braces for these ifs.

Done.

> CXXConstructExpr doesn't know which Decl (if any) it's initializing. If you
> want to be able to handle things like:
>
> struct C { Mutex Mu; C() EXCLUSIVE_LOCK_FUNCTION(this->Mu); /*...*/ };
> C c; // c.Mu is known to be acquired
>
> ... then I think you'll need to visit DeclStmts, trawl their declaration group
> looking for initialized VarDecls, and build up some sort of mapping yourself.

Thanks -- I've got a new patch going out today which looks at DeclStmts.  :-)

-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315