[PATCH 2/2] Thread-safety: Add test for -Wthread-safety check for C.

This commit adds test to the -Wthread-safety check for C code.

Co-authored-by: Ethan Jackson <ethan@nicira.com>
Signed-off-by: Alex Wang <alexw@nicira.com>

Index: warn-thread-safety-analysis.c

Before this commit, the -Wthread-safety check is only for C++ code.
This commit makes it available for C code as well.

Co-authored-by: Ethan Jackson <ethan@nicira.com>
Signed-off-by: Alex Wang <alexw@nicira.com>

Index: SemaDeclAttr.cpp

Please include the tests and product code change in the same patch (&
ideally attach the patch at least (including it inline in the email is
optional) or consider using Phabricator

For details:
http://llvm.org/docs/DeveloperPolicy.html
http://llvm.org/docs/Phabricator.html

Hey David,

Thanks for your quick response.

Please check the attachment for the combined patch.

Kind Regards,
Alex Wang,

wthread_safety_for_c.diff (5.07 KB)

Hi,

What is the __has_extension() variable to check for support for these? Or do we need to check for each of the attributes individually?

David

Hey David,

Thanks for your reply, please correct me if my understanding is wrong,

Since I only use the thread safety attributes defined in clang, there should be no requirement on gcc extension.

Actually, I think it does not make very much sense to check for each of the attributes individually, since we only use these attributes for semantic analysis and your cpp test (warn-thread-safety-analysis.cpp) is every detailed.

Kind Regards,
Alex Wang,

Hi Alex,

I think you misunderstand. If I want to use these in, for example, pthread.h, then I want to make sure that the attributes are only used when a compiler that knows about them is being used. Clang provides the __has_feature() mechanism in the preprocessor for these checks at a coarse granularity, so that library headers can conditionally expose compiler and compiler-version specific features.

How do I check, from within a header, that I am using a version of clang that supports thread-safety annotations in C?

David

Hey David,

Sorry for the delayed reply and thanks for your explanation,

I did modification below to add this new “c_wthread_safety” so that it can be checked by “__has_feature()”. Is this what you mean?

Hope to hear more comments,

Kind Regards,
Alex Wang,

Index: lib/Lex/PPMacroExpansion.cpp

Yes, that's the sort of thing. c_thread_safety_attributes would probably be a bit more in keeping with the naming convention for __has_feature entries.

On a mostly unrelated issue, we can do clang -E -dM to find out the list of predefined macros, but the only way of finding the valid __has_feature() and __has_extension() values that I know of is to read the source code. It would be good if there was some command line option to output all of these...

David

Hey David,

Thanks for the comments,

Yes, that's the sort of thing. c_thread_safety_attributes would probably
be a bit more in keeping with the naming convention
for __has_feature entries.

Great! ;D
I attach the entire patch again with this reply.

On a mostly unrelated issue, we can do clang -E -dM to find out the list of

predefined macros, but the only way of finding the valid __has_feature()
and __has_extension() values that I know of is to read the source code. It
would be good if there was some command line option to output all of
these...

I can hack on this over my spare time. Right now, we eagerly look forward
to having "c_thread_safety_attributes". Is there anything else you think
required for applying the patch?

Kind Regards,
Alex Wang,

c_thread_safety_attributes.diff (5.64 KB)

This was committed in r187365, but the test case was found to be flawed in 199347 - would you mind fixing it?

Hey David,

Yes, i’ll take a look and fix it. Thx for the update.

Hey David,

Please see the attachment for fix patch.

Sorry again, I must have run the wrong test the first time.
The test is flawed at the beginning.

I tested the fix, output:
root@server329:~/llvm/llvm/tools/clang/test/Sema#
~/llvm/llvm/utils/lit/lit.py -v warn-
thread-safety-analysis.c
lit.py: lit.cfg:196: note: using clang:
'/root/llvm/llvm/Debug+Asserts/bin/clang'
-- Testing: 1 tests, 1 threads --
PASS: Clang :: Sema/warn-thread-safety-analysis.c (1 of 1)
Testing Time: 0.03s
  Expected Passes : 1

Thanks,
Alex Wang,
test_fix_wthread_safety_for_c.diff
<http://clang-developers.42468.n3.nabble.com/file/n4037219/test_fix_wthread_safety_for_c.diff>

Thanks for that.

Committed as r199762.

Thanks guys