On Tue, Jun 09, 2020 at 09:48:34AM +0200, Marco Elver wrote:
> gcc/
> * params.opt: Define --param=tsan-distinguish-volatile=[0,1].
> * sanitizer.def (BUILT_IN_TSAN_VOLATILE_READ1): Define new
> builtin for volatile instrumentation of reads/writes.
> (BUILT_IN_TSAN_VOLATILE_READ2): Likewise.
> (BUILT_IN_TSAN_VOLATILE_READ4): Likewise.
> (BUILT_IN_TSAN_VOLATILE_READ8): Likewise.
> (BUILT_IN_TSAN_VOLATILE_READ16): Likewise.
> (BUILT_IN_TSAN_VOLATILE_WRITE1): Likewise.
> (BUILT_IN_TSAN_VOLATILE_WRITE2): Likewise.
> (BUILT_IN_TSAN_VOLATILE_WRITE4): Likewise.
> (BUILT_IN_TSAN_VOLATILE_WRITE8): Likewise.
> (BUILT_IN_TSAN_VOLATILE_WRITE16): Likewise.
> * tsan.c (get_memory_access_decl): Argument if access is
> volatile. If param tsan-distinguish-volatile is non-zero, and
> access if volatile, return volatile instrumentation decl.
> (instrument_expr): Check if access is volatile.
>
> gcc/testsuite/
> * c-c++-common/tsan/volatile.c: New test.
In general looks ok, just some minor nits.
> --- a/gcc/params.opt
> +++ b/gcc/params.opt
> @@ -908,6 +908,10 @@ Stop reverse growth if the reverse probability of best
> edge is less than this th
> Common Joined UInteger Var(param_tree_reassoc_width) Param Optimization
> Set the maximum number of instructions executed in parallel in reassociated
> tree. If 0, use the target dependent heuristic.
>
> +-param=tsan-distinguish-volatile=
> +Common Joined UInteger Var(param_tsan_distinguish_volatile) IntegerRange(0,
> 1) Param Optimization
Do we need/want Optimization here? Optimization means the option is
per-function, but to me whether you want to distinguish volatiles or not
seems to be a global decision for the whole project.
> +Emit special instrumentation for accesses to volatiles.
> +
> -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.
> --- 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)
This last entry is already too long (line limit 80 chars), so should be
wrapped like:
DEF_SANITIZER_BUILTIN(BUILT_IN_TSAN_VOLATILE_WRITE16,
"__tsan_volatile_write16", BT_FN_VOID_PTR,
ATTR_NOTHROW_LEAF_LIST)
instead.
> --- a/gcc/tsan.c
> +++ b/gcc/tsan.c
> @@ -52,25 +52,41 @@ along with GCC; see the file COPYING3. If not see
> void __tsan_read/writeX (void *addr); */
>
> static tree
> -get_memory_access_decl (bool is_write, unsigned size)
> +get_memory_access_decl (bool is_write, unsigned size, bool volatilep)
> {
> enum built_in_function fcode;
>
> - if (size <= 1)
> - fcode = is_write ? BUILT_IN_TSAN_WRITE1
> - : BUILT_IN_TSAN_READ1;
> - else if (size <= 3)
> - fcode = is_write ? BUILT_IN_TSAN_WRITE2
> - : BUILT_IN_TSAN_READ2;
> - else if (size <= 7)
> - fcode = is_write ? BUILT_IN_TSAN_WRITE4
> - : BUILT_IN_TSAN_READ4;
> - else if (size <= 15)
> - fcode = is_write ? BUILT_IN_TSAN_WRITE8
> - : BUILT_IN_TSAN_READ8;
> + if (param_tsan_distinguish_volatile && volatilep)
> + {
> + if (size <= 1)
> + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE1
> + : BUILT_IN_TSAN_VOLATILE_READ1;
> + else if (size <= 3)
> + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE2
> + : BUILT_IN_TSAN_VOLATILE_READ2;
> + else if (size <= 7)
> + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE4
> + : BUILT_IN_TSAN_VOLATILE_READ4;
> + else if (size <= 15)
> + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE8
> + : BUILT_IN_TSAN_VOLATILE_READ8;
> + else
> + fcode = is_write ? BUILT_IN_TSAN_VOLATILE_WRITE16
> + : BUILT_IN_TSAN_VOLATILE_READ16;
> + }
> else
> - fcode = is_write ? BUILT_IN_TSAN_WRITE16
> - : BUILT_IN_TSAN_READ16;
> + {
> + if (size <= 1)
> + fcode = is_write ? BUILT_IN_TSAN_WRITE1 : BUILT_IN_TSAN_READ1;
> + else if (size <= 3)
> + fcode = is_write ? BUILT_IN_TSAN_WRITE2 : BUILT_IN_TSAN_READ2;
> + else if (size <= 7)
> + fcode = is_write ? BUILT_IN_TSAN_WRITE4 : BUILT_IN_TSAN_READ4;
> + else if (size <= 15)
> + fcode = is_write ? BUILT_IN_TSAN_WRITE8 : BUILT_IN_TSAN_READ8;
> + else
> + fcode = is_write ? BUILT_IN_TSAN_WRITE16 : BUILT_IN_TSAN_READ16;
> + }
The above gets too ugly. Please use use instead:
enum built_in_function fcode;
int pos;
if (size <= 1)
pos = 0;
else if (size <= 3)
pos = 1;
else if (size <= 7)
pos = 2;
else if (size <= 15)
pos = 3;
else
pos = 4;
if (param_tsan_distinguish_volatile && volatilep)
fcode = (is_write ? BUILT_IN_TSAN_VOLATILE_WRITE1
: BUILT_IN_TSAN_VOLATILE_READ1);
else
fcode = (is_write ? BUILT_IN_TSAN_WRITE1
: BUILT_IN_TSAN_READ1);
fcode = (built_in_function) (fcode + pos);
We have other code that already relies on certain *builtin*.def ranges being
consecutive.
> @@ -204,8 +220,11 @@ instrument_expr (gimple_stmt_iterator gsi, tree expr,
> bool is_write)
> g = gimple_build_call (builtin_decl, 2, expr_ptr, size_int (size));
> }
> 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));
Formatting, space between VOLATILE and (.
And perhaps you don't need to use the builtin_decl temporary, just:
g = gimple_build_call (get_memory_access_decl (is_write, size,
TREE_THIS_VOLATILE (expr)),
1, expr_ptr);
would be fine. The reason to use the temporary in the other cases is that
it gets too long and needs too much wrapping.
Ok for trunk with those nits fixed.
Jakub
