| Message ID | df36be45-b8a6-749c-0ebc-b2e8bc710448@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Add -Wtsan. | expand |
On 12/9/20 2:24 AM, Martin Liška wrote: > Hello. > > The newly added warning is about warning a user > that std::atomic_thread_fence is not supported by TSAN. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin > > gcc/ChangeLog: > > PR sanitizer/97868 > * common.opt: Add new warning -Wtsan. > * doc/invoke.texi: Likewise. > * tsan.c (instrument_builtin_call): Warn users about unsupported > std::atomic_thread_fence. > --- > gcc/common.opt | 4 ++++ > gcc/doc/invoke.texi | 8 +++++++- > gcc/tsan.c | 6 ++++++ > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/gcc/common.opt b/gcc/common.opt > index 6645539f5e5..6c24c7bbffb 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -842,6 +842,10 @@ Wvector-operation-performance > Common Var(warn_vector_operation_performance) Warning > Warn when a vector operation is compiled outside the SIMD. > > +Wtsan > +Common Var(warn_tsan) Init(1) Warning > +Warn about unsupported features in the ThreadSanitizer. ^^^ Just a minor grammatical nit. As a name, ThreadSanitizer should not be preceded by an article. Same in invoke.texi below. > + > Xassembler > Driver Separate > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index f7e8c8b29b0..5bd18c78e99 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -377,7 +377,7 @@ Objective-C and Objective-C++ Dialects}. > -Wswitch -Wno-switch-bool -Wswitch-default -Wswitch-enum @gol > -Wno-switch-outside-range -Wno-switch-unreachable -Wsync-nand @gol > -Wsystem-headers -Wtautological-compare -Wtrampolines -Wtrigraphs @gol > --Wtype-limits -Wundef @gol > +-Wtsan -Wtype-limits -Wundef @gol > -Wuninitialized -Wunknown-pragmas @gol > -Wunsuffixed-float-constants -Wunused @gol > -Wunused-but-set-parameter -Wunused-but-set-variable @gol > @@ -7951,6 +7951,12 @@ Note that the code above is invalid in C++11. > > This warning is enabled by default. > > +@item -Wtsan > +@opindex Wtsan > +@opindex Wno-tsan > +Warn about unsupported features in the ThreadSanitizer. > +This warning is enabled by default. > + > @item -Wtype-limits > @opindex Wtype-limits > @opindex Wno-type-limits > diff --git a/gcc/tsan.c b/gcc/tsan.c > index 4d6223454b5..be9fabea62a 100644 > --- a/gcc/tsan.c > +++ b/gcc/tsan.c > @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3. If not see > #include "asan.h" > #include "builtins.h" > #include "target.h" > +#include "diagnostic-core.h" > > /* Number of instrumented memory accesses in the current function. */ > > @@ -500,6 +501,11 @@ instrument_builtin_call (gimple_stmt_iterator *gsi) > continue; > else > { > + if (fcode == BUILT_IN_ATOMIC_THREAD_FENCE) > + warning_at (gimple_location (stmt), OPT_Wtsan, > + "%qs is not supported by ThreadSanitizer and may " > + "lead to false positives", "atomic_thread_fence"); Most similar warnings mention the sanitizer option rather than referring to the tool by name. E.g., "transactional memory is not supported with %<-fsanitize=address%>" or "%<-fsanitize=leak%> is incompatible with %<-fsanitize=thread%>" For the sake of consistency (and also to provide a bit of additional detail) I would suggest to follow that style. Martin > + > tree decl = builtin_decl_implicit (tsan_atomic_table[i].tsan_fcode); > if (decl == NULL_TREE) > return;
On 12/10/20 4:50 PM, Martin Sebor wrote: > Most similar warnings mention the sanitizer option rather than > referring to the tool by name. E.g., > > "transactional memory is not supported with %<-fsanitize=address%>" > or > > "%<-fsanitize=leak%> is incompatible with %<-fsanitize=thread%>" > > For the sake of consistency (and also to provide a bit of additional > detail) I would suggest to follow that style. > > Martin Thank you Martin for the useful feedback. I've just tested the updated patch. Martin
On 12/14/20 3:09 AM, Martin Liška wrote: > On 12/10/20 4:50 PM, Martin Sebor wrote: >> Most similar warnings mention the sanitizer option rather than >> referring to the tool by name.  E.g., >> >>    "transactional memory is not supported with %<-fsanitize=address%>" >> or >> >>    "%<-fsanitize=leak%> is incompatible with %<-fsanitize=thread%>" >> >> For the sake of consistency (and also to provide a bit of additional >> detail) I would suggest to follow that style. >> >> Martin > > Thank you Martin for the useful feedback. > > I've just tested the updated patch. > > Martin > > 0001-Add-Wtsan.patch > > From 7c982d06749fd3650825a5fb417e5e3487ecbcaf Mon Sep 17 00:00:00 2001 > From: Martin Liska <mliska@suse.cz> > Date: Mon, 7 Dec 2020 15:55:59 +0100 > Subject: [PATCH] Add -Wtsan. > > gcc/ChangeLog: > > PR sanitizer/97868 > * common.opt: Add new warning -Wtsan. > * doc/invoke.texi: Likewise. > * tsan.c (instrument_builtin_call): Warn users about unsupported > std::atomic_thread_fence. > > gcc/testsuite/ChangeLog: > > PR sanitizer/97868 > * gcc.dg/tsan/atomic-fence.c: New test. OK.  Please consider a more through description in invoke.texi though.  What unsupported feature are we warning about and what are the consequences of not supporting that feature. jeff
On 12/16/20 1:19 AM, Jeff Law wrote: > OK.  Please consider a more through description in invoke.texi > though.  What unsupported feature are we warning about and what are the > consequences of not supporting that feature. Done that and installed the patch. Thank you for the review, Martin
diff --git a/gcc/common.opt b/gcc/common.opt index 6645539f5e5..6c24c7bbffb 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -842,6 +842,10 @@ Wvector-operation-performance Common Var(warn_vector_operation_performance) Warning Warn when a vector operation is compiled outside the SIMD. +Wtsan +Common Var(warn_tsan) Init(1) Warning +Warn about unsupported features in the ThreadSanitizer. + Xassembler Driver Separate diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index f7e8c8b29b0..5bd18c78e99 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -377,7 +377,7 @@ Objective-C and Objective-C++ Dialects}. -Wswitch -Wno-switch-bool -Wswitch-default -Wswitch-enum @gol -Wno-switch-outside-range -Wno-switch-unreachable -Wsync-nand @gol -Wsystem-headers -Wtautological-compare -Wtrampolines -Wtrigraphs @gol --Wtype-limits -Wundef @gol +-Wtsan -Wtype-limits -Wundef @gol -Wuninitialized -Wunknown-pragmas @gol -Wunsuffixed-float-constants -Wunused @gol -Wunused-but-set-parameter -Wunused-but-set-variable @gol @@ -7951,6 +7951,12 @@ Note that the code above is invalid in C++11. This warning is enabled by default. +@item -Wtsan +@opindex Wtsan +@opindex Wno-tsan +Warn about unsupported features in the ThreadSanitizer. +This warning is enabled by default. + @item -Wtype-limits @opindex Wtype-limits @opindex Wno-type-limits diff --git a/gcc/tsan.c b/gcc/tsan.c index 4d6223454b5..be9fabea62a 100644 --- a/gcc/tsan.c +++ b/gcc/tsan.c @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3. If not see #include "asan.h" #include "builtins.h" #include "target.h" +#include "diagnostic-core.h" /* Number of instrumented memory accesses in the current function. */ @@ -500,6 +501,11 @@ instrument_builtin_call (gimple_stmt_iterator *gsi) continue; else { + if (fcode == BUILT_IN_ATOMIC_THREAD_FENCE) + warning_at (gimple_location (stmt), OPT_Wtsan, + "%qs is not supported by ThreadSanitizer and may " + "lead to false positives", "atomic_thread_fence"); + tree decl = builtin_decl_implicit (tsan_atomic_table[i].tsan_fcode); if (decl == NULL_TREE) return;