On Tue, 9 Jun 2020 at 11:50, Jakub Jelinek <ja...@redhat.com> wrote:
>
> 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.
Adding Optimization here was Martin's suggestion. I'm fine either way
and just wanted to err on the conservative side.
Do note that in the kernel, we blacklist some files from
instrumentation entirely, which implies leaving '-fsanitize=thread
--param=tsan-distinguish-volatile=1' off. Although given that the
option is only used with -fsanitize=thread, maybe it doesn't matter.
If you strongly feel that Optimization should be removed again, please
let me know.
> > +Emit special instrumentation for accesses to volatiles.
> > +
[...]
> > +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.
>
Thanks for the quick review. I'll try to get v3 sent out later today/tomorrow.
-- Marco
