RE: [PATCH] Enable experimental TSAN support for Ada

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

Hi,

On Fri, 9 Jan 2015 10:57:14, Richard Biener wrote:
>
> On Mon, Jan 5, 2015 at 9:00 PM, Jeff Law <law@redhat.com> wrote:
>> On 01/03/15 06:49, Bernd Edlinger wrote:
>>>
>>> Hi,
>>>
>>> I was experimenting with enabling TSAN for Ada recently.
>>> I think this gives rather interesting results.
>>>
>>> The Instrumentation worked almost out of the box, we just have
>>> the problem that it is not gimple-OK to fold something like
>>> "& VIEW_CONVERT_EXPR(x)", and this happens in Ada all the time.
>>>
>>> Boot-Strapped and regression-tested on x86_64-linux-gnu.
>>> OK for trunk?
>>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>>
>>>
>>> changelog-tsan-ada.txt
>>>
>>>
>>> gcc/ChangeLog:
>>> 2015-01-03 Bernd Edlinger<bernd.edlinger@hotmail.de>
>>>
>>> Enable experimental TSAN support for Ada.
>>> * tsan.c (instrument_expr): Handle VIEW_CONVERT_EXPR.
>>
>> OK for the trunk with a comment before the new block of code indicating why
>> we need to handle VIEW_CONVERT_EXPR specially here (specifically we can't
>> call build_fold_addr_expr on the VIEW_CONVERT_EXPR).
>
> There may be multiple VIEW_CONVERT_EXPRs in a reference chain
> so simply stripping the outermost only doesn't work (the assert).
>

Hmm, that did not happen in any of the Ada tests in ada/acats nor in gnat.dg,
but with Ada anything may be possible...

Would you like it better if I do it this way:

      align = get_object_alignment (expr);
      if (align < BITS_PER_UNIT)
         return false;

      do
        {
           expr = TREE_OPERAND (expr, 0);
        } while (TREE_CODE (expr) == VIEW_CONVERT_EXPR);

      gcc_checking_assert (is_gimple_addressable (expr));
      expr_ptr = build_fold_addr_expr (unshare_expr (expr));



> I wonder why you do all the special-casing when you have already
> called get_inner_reference on the reference. The address is
> simply &base + offset + bitpos / BITS_PER_UNIT, the bitfield
> case is detectable via bitpos % BITS_PER_UNIT != 0.
>

I tried that first, but for something lile S.A[x].B
offset is someting like a+b*x, and while we handle that in expansion,
it is pretty hard to fold gimple code for &base + offset in this case.
But it is relatively easy to fold &S.A[x] and add a contant byte offset.


> Sth else I noticed, instead of checking points-to in the weird way
> you do you simply want if (DECL_P (base) && !may_be_aliased (base)).
>

you mean,

  /* No need to instrument accesses to decls that don't escape,
     they can't escape to other threads then.  */
  if (DECL_P (base))
    {
      struct pt_solution pt;
      memset (&pt, 0, sizeof (pt));
      pt.escaped = 1;
      pt.ipa_escaped = flag_ipa_pta != 0;
      pt.nonlocal = 1;
      if (!pt_solution_includes (&pt, base))
        return false;
      if (!is_global_var (base) && !may_be_aliased (base))
        return false;
    }

should be equivalent to

   if (DECL_P (base) && !may_be_aliased (base))
    return false;

is that right?


Thanks,
Bernd

> Richard.
>
>> Jeff
>>