RE: [PATCH] Enable experimental TSAN support for Ada

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

Hi Richard,

On Fri, 9 Jan 2015 17:19:57, Richard Biener wrote:
>
>
> Yes. As said, you generally need to run folding results through force_gimple_operand.
>
> Richard.
>


I have now used force_gimple_operand instead of special casing the VIEW_CONVERT_EXPRs.
And I see that all Ada test cases still work with -fsanitize=thread. So this feels like an improvement.

I have checked with a large C++ application, to see if the generated code changes or not.
And although this looked like it should not change the resulting code, I found one small difference
at -O3 -fsanitize=thread while compiling the function xmlSchemaCompareValuesInt in xmlschematypes.c
of libxml2.  The generated code size did not change, only two blocks of code changed place.
That was the only difference in about 16 MB of code.

The reason for this seems to be the following changes in the xmlschemastypes.c.104t.tsan1

   <bb 29>:
   p1_179 = xmlSchemaDateNormalize (x_7(D), 0.0);
   # DEBUG p1 => p1_179
   _180 = _xmlSchemaDateCastYMToDays (p1_179);
-  _660 = &p1_179->value.date;
-  _659 = &MEM[(struct xmlSchemaValDate *)_660 + 8B];
-  __builtin___tsan_read2 (_659);
+  _660 = &MEM[(struct xmlSchemaValDate *)p1_179 + 24B];
+  __builtin___tsan_read2 (_660);
   _181 = p1_179->value.date.day;
   _182 = (long int) _181;
   p1d_183 = _180 + _182;

this pattern is repeated everywhere. (- = before the patch. + = with the patch)

So it looks as if the generated code quality slightly improves with this change.

I have also tried to fold &base + offset + bitpos,  like this:

--- tsan.c.orig    2015-01-10 00:39:06.465210937 +0100
+++ tsan.c    2015-01-11 09:28:38.109423856 +0100
@@ -213,7 +213,18 @@ instrument_expr (gimple_stmt_iterator gs
       align = get_object_alignment (expr);
       if (align < BITS_PER_UNIT)
     return false;
-      expr_ptr = build_fold_addr_expr (unshare_expr (expr));
+      expr_ptr = build_fold_addr_expr (unshare_expr (base));
+      if (bitpos != 0)
+    {
+      if (offset != NULL)
+        offset = size_binop (PLUS_EXPR, offset,
+                 build_int_cst (sizetype,
+                        bitpos / BITS_PER_UNIT));
+      else
+        offset = build_int_cst (sizetype, bitpos / BITS_PER_UNIT);
+    }
+      if (offset != NULL)
+    expr_ptr = fold_build_pointer_plus (expr_ptr, offset);
     }
   expr_ptr = force_gimple_operand (expr_ptr, &seq, true, NULL_TREE);
   if ((size & (size - 1)) != 0 || size> 16


For simplicity first only in the simple case without DECL_BIT_FIELD_REPRESENTATIVE.
I tried this change at the same large C++ application, and see the code still works,
but the binary size increases at -O3 by about 1%.

So my conclusion would be that it is better to use force_gimple_operand directly 
on build_fold_addr_expr (unshare_expr (expr)), without using offset.

Well, I think this still resolves your objections.

Furthermore I used may_be_nonaddressable_p instead of is_gimple_addressable
and just return if it is found to be not true. (That did not happen in my tests.)

And I reworked the block with the pt_solution_includes.

I found that It can be rewritten, because pt_solution_includes can be
expanded to (is_global_var (decl) || pt_solution_includes_1 (&cfun->gimple_df->escaped, decl)
|| pt_solution_includes_1 (&ipa_escaped_pt, decl))

So, by De Morgan's law, you can rewite that block to

  if (DECL_P (base))
    {
      if (!is_global_var (base)
          && !pt_solution_includes_1 (&cfun->gimple_df->escaped, base)
          && !pt_solution_includes_1 (&ipa_escaped_pt, base))
        return false;
      if (!is_global_var (base) && !may_be_aliased (base))
        return false;
    }

Therefore I can move the common term !is_global_var (base) out of the block.  That's what I did.
As far as I can tell, none of the other terms here seem to be redundant.


Attached patch was boot-strapped and regression-tested on x86_64-linux-gnu.
OK for trunk?


Thanks
Bernd.

Attachment: changelog-tsan-cleanup.txt
Description: Text document

Attachment: patch-tsan-cleanup.diff
Description: Binary data