[PATCH] tsan: Add optional support for distinguishing volatiles
Martin Liška
mliska@suse.cz
Wed May 20 14:19:36 GMT 2020
On 5/20/20 4:04 PM, Marco Elver wrote:
> On Wed, 20 May 2020 at 15:30, Martin Liška <mliska@suse.cz> wrote:
>>
>> On 4/23/20 5:42 PM, Marco Elver via Gcc-patches wrote:
>>
>> Hello.
>>
>> Not being a maintainer of libsanitizer but I can provide a feedback:
>
> Thank you for the review!
>
> Note, this is not touching libsanitizer or user-space TSAN runtime,
> only the compiler. Alternative runtimes may enable the option where
> required (particularly, kernel space runtimes).
You are right ;) Anyway, a maintainer will be needed, but Jakub promised
to make a review once I'm done.
>
>>> Add support to optionally emit different instrumentation for accesses to
>>> volatile variables. While the default TSAN runtime likely will never
>>> require this feature, other runtimes for different environments that
>>> have subtly different memory models or assumptions may require
>>> distinguishing volatiles.
>>>
>>> One such environment are OS kernels, where volatile is still used in
>>> various places for various reasons, and often declare volatile to be
>>> "safe enough" even in multi-threaded contexts. One such example is the
>>> Linux kernel, which implements various synchronization primitives using
>>> volatile (READ_ONCE(), WRITE_ONCE()). Here the Kernel Concurrency
>>> Sanitizer (KCSAN) [1], is a runtime that uses TSAN instrumentation but
>>> otherwise implements a very different approach to race detection from
>>> TSAN.
>>>
>>> While in the Linux kernel it is generally discouraged to use volatiles
>>> explicitly, the topic will likely come up again, and we will eventually
>>> need to distinguish volatile accesses [2]. The other use-case is
>>> ignoring data races on specially marked variables in the kernel, for
>>> example bit-flags (here we may hide 'volatile' behind a different name
>>> such as 'no_data_race').
>>
>> Do you have a follow up patch that will introduce such an attribute? Does clang
>> already have the attribute?
>
> Ah, sorry I wasn't clear enough here. As far as the compiler is aware,
> no extra attribute, so no patch for the compilers for that. It's an
> extra use-case, but not the main reason we need this. Re attribute, we
> may do:
>
> #ifdef __SANITIZE_THREAD__
> #define no_data_race volatile
> #else
> #define no_data_race
> #endif
>
> in the kernel. It's something that was expressed by kernel
> maintainers, as some people want to just have a blanket annotation to
> make the data race detector ignore or treat certain variables as if
> they were atomic, even though they're not. But for all intents and
> purposes, please ignore the 'no_data_race' comment.
That's a reasonable approach for now!
>
> The main use-case, of actually distinguishing volatile accesses is now
> required for KCSAN in the kernel, as without it the race detector
> won't work anymore after some {READ,WRITE}_ONCE() rework. Right now,
> KCSAN in the kernel is therefore Clang only:
> https://lore.kernel.org/lkml/20200515150338.190344-1-elver@google.com/
>
> Getting this patch into GCC gets us one step closer to being able to
> re-enable KCSAN for GCC in the kernel, but there are some other loose
> ends that I don't know how to resolve (independent of this patch).
Ok.
>
> [...]
>>> +-param=tsan-distinguish-volatile=
>>> +Common Joined UInteger Var(param_tsan_distinguish_volatile) IntegerRange(0, 1) Param
>>> +Emit special instrumentation for accesses to volatiles.
>>
>> You want to add 'Optimization' keyword as the parameter can be different
>> per-TU (in LTO mode).
>
> Will add in v2.
>
>>> +
>>> -param=uninit-control-dep-attempts=
>>> Common Joined UInteger Var(param_uninit_control_dep_attempts) Init(1000) IntegerRange(1, 65536) Param Optimization
>>> Maximum number of nested calls to search for control dependencies during uninitialized variable analysis.
>>> diff --git a/gcc/sanitizer.def b/gcc/sanitizer.def
>>> index 11eb6467eba..a32715ddb92 100644
>>> --- a/gcc/sanitizer.def
>>> +++ b/gcc/sanitizer.def
>>> @@ -214,6 +214,27 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_READ_RANGE, "__tsan_read_range",
>>> DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_WRITE_RANGE, "__tsan_write_range",
>>> BT_FN_VOID_PTR_PTRMODE, ATTR_NOTHROW_LEAF_LIST)
>>>
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ1, "__tsan_volatile_read1",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ2, "__tsan_volatile_read2",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ4, "__tsan_volatile_read4",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ8, "__tsan_volatile_read8",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_READ16, "__tsan_volatile_read16",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE1, "__tsan_volatile_write1",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE2, "__tsan_volatile_write2",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE4, "__tsan_volatile_write4",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE8, "__tsan_volatile_write8",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE16, "__tsan_volatile_write16",
>>> + BT_FN_VOID_PTR, ATTR_NOTHROW_LEAF_LIST)
>>> +
>>> DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_ATOMIC8_LOAD,
>>> "__tsan_atomic8_load",
>>> BT_FN_I1_CONST_VPTR_INT, ATTR_NOTHROW_LEAF_LIST)
>>> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
>>> index 245c1512c76..f1d3e236b86 100644
>>> --- a/gcc/testsuite/ChangeLog
>>> +++ b/gcc/testsuite/ChangeLog
>>> @@ -1,3 +1,7 @@
>>> +2020-04-23 Marco Elver <elver@google.com>
>>> +
>>> + * c-c++-common/tsan/volatile.c: New test.
>>> +
>>> 2020-04-23 Jakub Jelinek <jakub@redhat.com>
>>>
>>> PR target/94707
>>> diff --git a/gcc/testsuite/c-c++-common/tsan/volatile.c b/gcc/testsuite/c-c++-common/tsan/volatile.c
>>> new file mode 100644
>>> index 00000000000..d51d1e3ce8d
>>> --- /dev/null
>>> +++ b/gcc/testsuite/c-c++-common/tsan/volatile.c
>>
>> Can you please add a run-time test-case that will check gd-output for TSAN
>> error messages?
>
> What do you mean? The user-space TSAN runtime itself does not make use
> of the option, and therefore will and should never implement
> __tsan_volatile*.
I've got it. So at least please add scanning of assembly or a tree dump
for the expected __tsan_* calls.
Martin
>
> As stated in the commit message, it's an option for alternative
> runtimes. Recently, the KCSAN runtime in the Linux kernel (there are
> also "CSAN" ports to NetBSD and FreeBSD kernels, which also had the
> same problem that default TSAN instrumentation doesn't distinguish
> volatiles). Note, we chose "CSAN" instead of "TSAN" for naming the
> different runtime, to avoid confusion since the runtimes function very
> very differently, just use the same instrumentation. (There was also a
> KTSAN for the kernel, but it turned out to be too complex in kernel
> space -- still, very little in common with the user-space runtime,
> just similar algorithm.)
>
> FWIW we have a test in the Linux kernel that checks the runtime, since
> that's where the runtime is implemented.
>
>>> @@ -0,0 +1,62 @@
>>> +/* { dg-additional-options "--param=tsan-distinguish-volatile=1" } */
>>> +
>>> +#include <assert.h>
>>> +#include <stdint.h>
>>> +#include <stdio.h>
>>> +
>>> +int32_t Global4;
>>> +volatile int32_t VolatileGlobal4;
>>> +volatile int64_t VolatileGlobal8;
> [...]
>>> else if (rhs == NULL)
>>> - g = gimple_build_call (get_memory_access_decl (is_write, size),
>>> - 1, expr_ptr);
>>> + {
>>> + builtin_decl = get_memory_access_decl (is_write, size,
>>> + TREE_THIS_VOLATILE(expr));
>>> + g = gimple_build_call (builtin_decl, 1, expr_ptr);
>>> + }
>>> else
>>> {
>>> builtin_decl = builtin_decl_implicit (BUILT_IN_TSAN_VPTR_UPDATE);
>>>
>>
>> And please check coding style, 8 spares are not expanded with a tab.
>
> Will fix for v2.
>
> Thanks,
> -- Marco
>
More information about the Gcc-patches
mailing list