On Fri, Jan 9, 2015 at 11:21 AM, Bernd Edlinger
<bernd.edlin...@hotmail.de> wrote:
> Hi,
>
> On Fri, 9 Jan 2015 10:57:14, Richard Biener wrote:
>>
>> On Mon, Jan 5, 2015 at 9:00 PM, Jeff Law <l...@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.edlin...@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);
No, I mean the tree might look like
COMPONENT_REF <VIEW_CONVERT_EXPR < COMPONENT_REF <VIEW_CONVERT_EXPR <....
thus VIEW_CONVERT_EXPR doesn't have to be outermost but it
can appear anywhere in the reference chain.
> 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.
Not really, just use one of the force_gimple_operand* functions.
> 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?
Yes, well, not exactly, but I wonder if its worth doing the extra check
if you only check decl accesses anyway and not indirect accesses.
Richard.
>
> Thanks,
> Bernd
>
>> Richard.
>>
>>> Jeff
>>>
>
