[PATCH PR67921]Use sizetype for CHREC_RIGHT when building pointer type CHREC

[PATCH PR67921]Use sizetype for CHREC_RIGHT when building pointer type CHREC

[Bin Cheng]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

Hi,
As analyzed in PR67921, I think the issue is caused by fold_binary_loc which
folds:
  4 - (sizetype) &c - (sizetype) ((int *) p1_8(D) + ((sizetype) a_23 * 24 +
4))
into below form:
  ((sizetype) -((int *) p1_8(D) + ((sizetype) a_23 * 24 + 4)) - (sizetype)
&c) + 4

Look the minus sizetype expression is folded as negative pointer expression,
which seems incorrect.  Apart from this, The direct reason of this ICE is in
CHREC because of an overlook.  In general CHREC supports NEGATE_EXPR for
CHREC, the only problem is it uses pointer type for CHREC_RIGHT, rather than
sizetype, when building pointer type CHREC.

This simple patch fixes the ICE issue.  Bootstrap and test on x86 & x86_64.

Is it OK?

Note, I do think the associate logic in fold_binary_loc needs fix, but that
should be another patch.


2015-10-20  Bin Cheng  <bin.cheng@arm.com>

	PR tree-optimization/67921
	* tree-chrec.c (chrec_fold_multiply): Use sizetype for CHREC_RIGHT
if
	type is pointer type.

2015-10-20  Bin Cheng  <bin.cheng@arm.com>

	PR tree-optimization/67921
	* gcc.dg/ubsan/pr67921.c: New test.

[-- Attachment #2: pr67921-20151020.txt --]
[-- Type: text/plain, Size: 1467 bytes --]

diff --git a/gcc/tree-chrec.c b/gcc/tree-chrec.c
index 649c9fe..ef7b70b 100644
--- a/gcc/tree-chrec.c
+++ b/gcc/tree-chrec.c
@@ -436,7 +436,8 @@ chrec_fold_multiply (tree type,
 	  return build_polynomial_chrec
 	    (CHREC_VARIABLE (op0),
 	     chrec_fold_multiply (type, CHREC_LEFT (op0), op1),
-	     chrec_fold_multiply (type, CHREC_RIGHT (op0), op1));
+	     chrec_fold_multiply (POINTER_TYPE_P (type) ? sizetype : type,
+				  CHREC_RIGHT (op0), op1));
 	}
 
     CASE_CONVERT:
@@ -459,7 +460,8 @@ chrec_fold_multiply (tree type,
 	  return build_polynomial_chrec
 	    (CHREC_VARIABLE (op1),
 	     chrec_fold_multiply (type, CHREC_LEFT (op1), op0),
-	     chrec_fold_multiply (type, CHREC_RIGHT (op1), op0));
+	     chrec_fold_multiply (POINTER_TYPE_P (type) ? sizetype : type,
+				  CHREC_RIGHT (op1), op0));
 
 	CASE_CONVERT:
 	  if (tree_contains_chrecs (op1, NULL))
diff --git a/gcc/testsuite/gcc.dg/ubsan/pr67921.c b/gcc/testsuite/gcc.dg/ubsan/pr67921.c
new file mode 100644
index 0000000..5e7d707
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ubsan/pr67921.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=undefined" } */
+
+typedef struct {
+  int a;
+  int arr[][6];
+}st;
+
+void bar (int);
+void foo (st *p)
+{
+  int a;
+  for (; a < 2; a++)
+    for (; p->a;)
+      {
+	int *b = p->arr[a];
+	int c[66];
+	int j = 0;
+	for (; j < 56; j++)
+	  bar (b[j] - c[j]);
+      }
+}

Re: [PATCH PR67921]Use sizetype for CHREC_RIGHT when building pointer type CHREC

[Richard Biener]

On Wed, Oct 21, 2015 at 6:46 AM, Bin Cheng <bin.cheng@arm.com> wrote:
> Hi,
> As analyzed in PR67921, I think the issue is caused by fold_binary_loc which
> folds:
>   4 - (sizetype) &c - (sizetype) ((int *) p1_8(D) + ((sizetype) a_23 * 24 +
> 4))
> into below form:
>   ((sizetype) -((int *) p1_8(D) + ((sizetype) a_23 * 24 + 4)) - (sizetype)
> &c) + 4
>
> Look the minus sizetype expression is folded as negative pointer expression,
> which seems incorrect.  Apart from this, The direct reason of this ICE is in
> CHREC because of an overlook.  In general CHREC supports NEGATE_EXPR for
> CHREC, the only problem is it uses pointer type for CHREC_RIGHT, rather than
> sizetype, when building pointer type CHREC.
>
> This simple patch fixes the ICE issue.  Bootstrap and test on x86 & x86_64.
>
> Is it OK?

Hmm, I think not - we shouldn't ever get pointer typed
multiplications.  Did you track
down which is the bogus fold transform (I agree the result above is
bogus)?  It's
probably related to STRIP_NOPS stripping sizetype conversions from pointers
so we might get split_tree to build such negate.  Note that split_tree strips
(sign!) nops itself and thus should probably simply receive op0 and op1 instead
of arg0 and arg1.

I'm testing

@@ -9505,8 +9523,8 @@ fold_binary_loc (location_t loc,
             then the result with variables.  This increases the chances of
             literals being recombined later and of generating relocatable
             expressions for the sum of a constant and literal.  */
-         var0 = split_tree (arg0, code, &con0, &lit0, &minus_lit0, 0);
-         var1 = split_tree (arg1, code, &con1, &lit1, &minus_lit1,
+         var0 = split_tree (op0, code, &con0, &lit0, &minus_lit0, 0);
+         var1 = split_tree (op1, code, &con1, &lit1, &minus_lit1,
                             code == MINUS_EXPR);

          /* Recombine MINUS_EXPR operands by using PLUS_EXPR.  */

which fixes the testcase for me.

Richard.

> Note, I do think the associate logic in fold_binary_loc needs fix, but that
> should be another patch.
>
>
> 2015-10-20  Bin Cheng  <bin.cheng@arm.com>
>
>         PR tree-optimization/67921
>         * tree-chrec.c (chrec_fold_multiply): Use sizetype for CHREC_RIGHT
> if
>         type is pointer type.
>
> 2015-10-20  Bin Cheng  <bin.cheng@arm.com>
>
>         PR tree-optimization/67921
>         * gcc.dg/ubsan/pr67921.c: New test.

Re: [PATCH PR67921]Use sizetype for CHREC_RIGHT when building pointer type CHREC

[Bin.Cheng]

On Wed, Oct 21, 2015 at 5:15 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Oct 21, 2015 at 6:46 AM, Bin Cheng <bin.cheng@arm.com> wrote:
>> Hi,
>> As analyzed in PR67921, I think the issue is caused by fold_binary_loc which
>> folds:
>>   4 - (sizetype) &c - (sizetype) ((int *) p1_8(D) + ((sizetype) a_23 * 24 +
>> 4))
>> into below form:
>>   ((sizetype) -((int *) p1_8(D) + ((sizetype) a_23 * 24 + 4)) - (sizetype)
>> &c) + 4
>>
>> Look the minus sizetype expression is folded as negative pointer expression,
>> which seems incorrect.  Apart from this, The direct reason of this ICE is in
>> CHREC because of an overlook.  In general CHREC supports NEGATE_EXPR for
>> CHREC, the only problem is it uses pointer type for CHREC_RIGHT, rather than
>> sizetype, when building pointer type CHREC.
>>
>> This simple patch fixes the ICE issue.  Bootstrap and test on x86 & x86_64.
>>
>> Is it OK?
>
> Hmm, I think not - we shouldn't ever get pointer typed
> multiplications.  Did you track
> down which is the bogus fold transform (I agree the result above is
> bogus)?  It's
> probably related to STRIP_NOPS stripping sizetype conversions from pointers
> so we might get split_tree to build such negate.  Note that split_tree strips
> (sign!) nops itself and thus should probably simply receive op0 and op1 instead
> of arg0 and arg1.
Yes, I was going to send similar patch for fold stuff.  Just thought
it might be useful to support POINTER chrec in *_multiply.  I will
drop this and let you test yours.

Thanks,
bin
>
> I'm testing
>
> @@ -9505,8 +9523,8 @@ fold_binary_loc (location_t loc,
>              then the result with variables.  This increases the chances of
>              literals being recombined later and of generating relocatable
>              expressions for the sum of a constant and literal.  */
> -         var0 = split_tree (arg0, code, &con0, &lit0, &minus_lit0, 0);
> -         var1 = split_tree (arg1, code, &con1, &lit1, &minus_lit1,
> +         var0 = split_tree (op0, code, &con0, &lit0, &minus_lit0, 0);
> +         var1 = split_tree (op1, code, &con1, &lit1, &minus_lit1,
>                              code == MINUS_EXPR);
>
>           /* Recombine MINUS_EXPR operands by using PLUS_EXPR.  */
>
> which fixes the testcase for me.
>
> Richard.
>
>> Note, I do think the associate logic in fold_binary_loc needs fix, but that
>> should be another patch.
>>
>>
>> 2015-10-20  Bin Cheng  <bin.cheng@arm.com>
>>
>>         PR tree-optimization/67921
>>         * tree-chrec.c (chrec_fold_multiply): Use sizetype for CHREC_RIGHT
>> if
>>         type is pointer type.
>>
>> 2015-10-20  Bin Cheng  <bin.cheng@arm.com>
>>
>>         PR tree-optimization/67921
>>         * gcc.dg/ubsan/pr67921.c: New test.

Re: [PATCH PR67921]Use sizetype for CHREC_RIGHT when building pointer type CHREC

[Richard Biener]

On Wed, Oct 21, 2015 at 11:39 AM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Wed, Oct 21, 2015 at 5:15 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Oct 21, 2015 at 6:46 AM, Bin Cheng <bin.cheng@arm.com> wrote:
>>> Hi,
>>> As analyzed in PR67921, I think the issue is caused by fold_binary_loc which
>>> folds:
>>>   4 - (sizetype) &c - (sizetype) ((int *) p1_8(D) + ((sizetype) a_23 * 24 +
>>> 4))
>>> into below form:
>>>   ((sizetype) -((int *) p1_8(D) + ((sizetype) a_23 * 24 + 4)) - (sizetype)
>>> &c) + 4
>>>
>>> Look the minus sizetype expression is folded as negative pointer expression,
>>> which seems incorrect.  Apart from this, The direct reason of this ICE is in
>>> CHREC because of an overlook.  In general CHREC supports NEGATE_EXPR for
>>> CHREC, the only problem is it uses pointer type for CHREC_RIGHT, rather than
>>> sizetype, when building pointer type CHREC.
>>>
>>> This simple patch fixes the ICE issue.  Bootstrap and test on x86 & x86_64.
>>>
>>> Is it OK?
>>
>> Hmm, I think not - we shouldn't ever get pointer typed
>> multiplications.  Did you track
>> down which is the bogus fold transform (I agree the result above is
>> bogus)?  It's
>> probably related to STRIP_NOPS stripping sizetype conversions from pointers
>> so we might get split_tree to build such negate.  Note that split_tree strips
>> (sign!) nops itself and thus should probably simply receive op0 and op1 instead
>> of arg0 and arg1.
> Yes, I was going to send similar patch for fold stuff.  Just thought
> it might be useful to support POINTER chrec in *_multiply.  I will
> drop this and let you test yours.

The patch regresses

Running target unix/
FAIL: gcc.dg/pr52578.c scan-tree-dump-times original "return 2;" 2
FAIL: gcc.dg/strict-overflow-5.c scan-tree-dump optimized "return 3"
FAIL: gcc.dg/tree-ssa/loop-15.c scan-tree-dump-times optimized "\\\\+" 0
FAIL: gcc.dg/tree-ssa/loop-15.c scan-tree-dump-times optimized "n_. \\\\* n_." 1
FAIL: gcc.dg/vect/vect-align-3.c -flto -ffat-lto-objects  scan-tree-dump-not vec
t "vect_do_peeling_for_loop_bound"
FAIL: gcc.dg/vect/vect-align-3.c scan-tree-dump-not vect "vect_do_peeling_for_lo
op_bound"

and for -m32

FAIL: gcc.target/i386/pr67985-3.c scan-assembler movd[ \\t]%xmm[0-7], %eax
FAIL: gcc.target/i386/pr67985-3.c scan-assembler mulss

as well.  So it requires some more complicated handling.  It might need to
take the negation code in split-tree to be performed only implicitely during
associate_trees (then on the correct type).  But that requires (quite) some
refactoring.  I suggest a struct tree_with_negate { tree t; bool negate_p; }
to pass this along.

Not for me at this point (looks I have quite a few patches to review ;))

Richard.

> Thanks,
> bin
>>
>> I'm testing
>>
>> @@ -9505,8 +9523,8 @@ fold_binary_loc (location_t loc,
>>              then the result with variables.  This increases the chances of
>>              literals being recombined later and of generating relocatable
>>              expressions for the sum of a constant and literal.  */
>> -         var0 = split_tree (arg0, code, &con0, &lit0, &minus_lit0, 0);
>> -         var1 = split_tree (arg1, code, &con1, &lit1, &minus_lit1,
>> +         var0 = split_tree (op0, code, &con0, &lit0, &minus_lit0, 0);
>> +         var1 = split_tree (op1, code, &con1, &lit1, &minus_lit1,
>>                              code == MINUS_EXPR);
>>
>>           /* Recombine MINUS_EXPR operands by using PLUS_EXPR.  */
>>
>> which fixes the testcase for me.
>>
>> Richard.
>>
>>> Note, I do think the associate logic in fold_binary_loc needs fix, but that
>>> should be another patch.
>>>
>>>
>>> 2015-10-20  Bin Cheng  <bin.cheng@arm.com>
>>>
>>>         PR tree-optimization/67921
>>>         * tree-chrec.c (chrec_fold_multiply): Use sizetype for CHREC_RIGHT
>>> if
>>>         type is pointer type.
>>>
>>> 2015-10-20  Bin Cheng  <bin.cheng@arm.com>
>>>
>>>         PR tree-optimization/67921
>>>         * gcc.dg/ubsan/pr67921.c: New test.