[cfe-dev] Thread Safety for C structs RFC.

[David Blaikie]

On Mon, Jul 15, 2013 at 2:16 PM, Ethan Jackson <ethan at nicira.com> wrote:
> Actually, I had one more question in this area.  Is there any reason
> to only allow structs an classes to be lockable?  In C it would be
> really clean to mark pthread_rwlock_t as lockable for example.  I know
> in most (all?) implementations, this is a struct under the covers, but
> I'm not sure if marking the underlying struct as lockable is portable
> (or possible for that matter).

You're necessarily going to have to annotate implementations - so I'm
not sure what kind of "portability" would be relevant here (each
pthread implementation will have to be annotated separately).

> If you think it's reasonable, I'd be willing to write up a patch
> supporting this (with unit tests of course).

You could have a go - my concern would be that if the thing being
annotated as lockable isn't actually onyl used for that purpose (eg:
if pthread_rwlock_t is a typedef for an int or other basic type) then
the analysis infrastructure might not handle this well in all cases
(trivial example: if you ever passed such a thing to a template & used
template argument deduction, we'd lose the sugar (the typedef) & just
see an 'int' & thus either not annotate it - or, worse, we might
already be using canonical types somewhere, in which case annotating a
typedef of int might result in all 'ints' being considered lockable)

>
> Thoughts?
> Ethan
>
> On Sun, Jul 14, 2013 at 10:15 PM, Delesley Hutchins <delesley at google.com> wrote:
>> Patch looks good to me.  Thanks!  All the thread safety test cases are
>> in C++, so this was probably overlooked.  Please update
>> warn-thread-safety-parsing with a test case.
>>
>>   -DeLesley
>>
>> On Sat, Jul 13, 2013 at 8:29 PM, Ethan Jackson <ethan at nicira.com> wrote:
>>> I'm in the process of evaluating using clang for the Open vSwitch project with
>>> hopes of taking advantage of its thread safety annotations.  Unfortunately, on
>>> tot these annotations only work in C++ because it's only possible to declare C++
>>> classes and structs as "lockable".  This simple patch appears to fix the issue
>>> (in my testing), but it's so trivial that I'm wondering if I've overlooked
>>> something.  I don't know clang well, so any advice in this area would be
>>> appreciated.
>>>
>>> Thanks,
>>> Ethan Jackson
>>>
>>> ---
>>>  lib/Sema/SemaDeclAttr.cpp |    3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp
>>> index 4c18a33..dc1c75d 100644
>>> --- a/lib/Sema/SemaDeclAttr.cpp
>>> +++ b/lib/Sema/SemaDeclAttr.cpp
>>> @@ -584,8 +584,7 @@ static bool checkLockableAttrCommon(Sema &S, Decl *D,
>>>    if (!checkAttributeNumArgs(S, Attr, 0))
>>>      return false;
>>>
>>> -  // FIXME: Lockable structs for C code.
>>> -  if (!isa<CXXRecordDecl>(D)) {
>>> +  if (!isa<CXXRecordDecl>(D) && !isa<RecordDecl>(D)) {
>>>      S.Diag(Attr.getLoc(), diag::warn_thread_attribute_wrong_decl_type)
>>>        << Attr.getName() << ThreadExpectedClassOrStruct;
>>>      return false;
>>> --
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>>
>> --
>> DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev