Thread Safety for C structs RFC.

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

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

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)) {

Before this is committed, though - you can just replace this with "if
(!isa<RecordDecl>(D))" I hope (please test/try this, of course), since
all CXXRecordDecls are RecordDecls. (& as Delesley mentioned - a test
case will be appropriate/necessary)

All of this sounds good to me. I'll write up a patch and send it out
this week. Thanks for being so responsive.

Ethan

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).

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

Thoughts?
Ethan

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)

Hey DeLesley and David,

I'm working with Ethan on this.

I have posted my tests last week and had David Chisnall reviewed it already.

The review can be found here:
http://clang-developers.42468.n3.nabble.com/PATCH-2-2-Thread-safety-Add-test-for-Wthread-safety-check-for-C-td4033444.html

Could you reviewed it? I'd like to do anything necessary to make it applied.

Please check the attachment for the entire patch
c_thread_safety_attributes.diff
<http://clang-developers.42468.n3.nabble.com/file/n4033535/c_thread_safety_attributes.diff>
.

Kind Regards,
Alex Wang,

Hey DeLesley and David,

I'm working with Ethan on this.

I have posted my tests last week and had David Chisnall reviewed it already.

The review can be found here:
http://clang-developers.42468.n3.nabble.com/PATCH-2-2-Thread-safety-Add-test-for-Wthread-safety-check-for-C-td4033444.html

Could you reviewed it? I'd like to do anything necessary to make it applied.

DeLesley's been out on leave - I'm only remotely aware of this code,
but given the simplicity of the change & low risk to existing code,
I've committed this in r187365. Thanks for your patience/reminders.

- David

Thanks for your review, David. ;D