Perform ubsan instrumentation for x >= 0 ? x : -x

Hi!

While Marek has been debugging while some perl test fails when perl is built
with GCC 4.9, we've discovered that it is because of undefined behavior in
it:
...
  && (((UV)1 << NV_PRESERVES_UV_BITS) >
      (UV)(SvIVX(sv) > 0 ? SvIVX(sv) : -SvIVX(sv)))
where SvIVX(sv) can be LONG_MIN, at which point there is undefined behavior
on the negation, but -fsanitize=undefined did detect only other issues in
the same source file and not this one, because fold-const.c folded it into
ABS_EXPR early.

This patch disables such folding, because all the A op 0 ? A : -A
operations this if is trying to optimize will need instrumentation with
-fsanitize=signed-integer-overflow.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2014-03-25  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/60636
	* fold-const.c (fold_cond_expr_with_comparison): Don't
	fold A op 0 ? A : -A if -fsanitize=undefined.

	* c-c++-common/ubsan/pr60636.c: New test.


	Jakub

On Tue, 25 Mar 2014, Jakub Jelinek wrote:

> Hi!
> 
> While Marek has been debugging while some perl test fails when perl is built
> with GCC 4.9, we've discovered that it is because of undefined behavior in
> it:
> ...
>   && (((UV)1 << NV_PRESERVES_UV_BITS) >
>       (UV)(SvIVX(sv) > 0 ? SvIVX(sv) : -SvIVX(sv)))
> where SvIVX(sv) can be LONG_MIN, at which point there is undefined behavior
> on the negation, but -fsanitize=undefined did detect only other issues in
> the same source file and not this one, because fold-const.c folded it into
> ABS_EXPR early.
> 
> This patch disables such folding, because all the A op 0 ? A : -A
> operations this if is trying to optimize will need instrumentation with
> -fsanitize=signed-integer-overflow.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Can't one form a valid constant expression using this?  Also isn't
ABS_EXPR undefined for LONG_MIN as well?  So why is ubsan not
instrumenting that instead?

Thanks,
Richard.
 
> 2014-03-25  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR sanitizer/60636
> 	* fold-const.c (fold_cond_expr_with_comparison): Don't
> 	fold A op 0 ? A : -A if -fsanitize=undefined.
> 
> 	* c-c++-common/ubsan/pr60636.c: New test.
> 
> --- gcc/fold-const.c.jj	2014-01-03 11:40:35.000000000 +0100
> +++ gcc/fold-const.c	2014-03-24 17:59:45.395445617 +0100
> @@ -4718,7 +4718,13 @@ fold_cond_expr_with_comparison (location
>  	      && operand_equal_p (TREE_OPERAND (arg1, 0),
>  				  TREE_OPERAND (arg2, 1), 0)
>  	      && operand_equal_p (TREE_OPERAND (arg1, 1),
> -				  TREE_OPERAND (arg2, 0), 0))))
> +				  TREE_OPERAND (arg2, 0), 0)))
> +      /* Don't fold this if sanitizing undefined behavior,
> +	 -A or Y-X might overflow and after folding this we wouldn't
> +	 be able to detect that.  */
> +      && ((flag_sanitize & SANITIZE_SI_OVERFLOW) == 0
> +	  || !INTEGRAL_TYPE_P (TREE_TYPE (arg01))
> +	  || TYPE_OVERFLOW_WRAPS (TREE_TYPE (arg01))))
>      switch (comp_code)
>        {
>        case EQ_EXPR:
> --- gcc/testsuite/c-c++-common/ubsan/pr60636.c.jj	2014-03-24 18:04:33.875925324 +0100
> +++ gcc/testsuite/c-c++-common/ubsan/pr60636.c	2014-03-24 18:09:18.696419079 +0100
> @@ -0,0 +1,15 @@
> +/* PR sanitizer/60636 */
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=undefined" } */
> +
> +volatile long long int a;
> +
> +int
> +main ()
> +{
> +  long long int u = -__LONG_LONG_MAX__ - 1;
> +  a = u > 0 ? u : -u;
> +  return 0;
> +}
> +
> +/* { dg-output "negation of -9223372036854775808 cannot be represented in type 'long long int'" } */
> 
> 	Jakub
> 
>

On Tue, Mar 25, 2014 at 10:09:26AM +0100, Richard Biener wrote:
> On Tue, 25 Mar 2014, Jakub Jelinek wrote:
> 
> > Hi!
> > 
> > While Marek has been debugging while some perl test fails when perl is built
> > with GCC 4.9, we've discovered that it is because of undefined behavior in
> > it:
> > ...
> >   && (((UV)1 << NV_PRESERVES_UV_BITS) >
> >       (UV)(SvIVX(sv) > 0 ? SvIVX(sv) : -SvIVX(sv)))
> > where SvIVX(sv) can be LONG_MIN, at which point there is undefined behavior
> > on the negation, but -fsanitize=undefined did detect only other issues in
> > the same source file and not this one, because fold-const.c folded it into
> > ABS_EXPR early.
> > 
> > This patch disables such folding, because all the A op 0 ? A : -A
> > operations this if is trying to optimize will need instrumentation with
> > -fsanitize=signed-integer-overflow.
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Can't one form a valid constant expression using this?  Also isn't
> ABS_EXPR undefined for LONG_MIN as well?  So why is ubsan not
> instrumenting that instead?

Well, for a constant expression A would need to fold to a constant, but then
A op 0 would fold to a constant too and so would the whole A op 0 ? A : -A.

Instrumenting ABS_EXPR is certainly possible, but definitely more code than
this patch.  I could revert this patch and implement it for 5.0 as ABS_EXPR
instrumentation.  Of course, if you think we should do the instrumentation
of ABS_EXPR, I can hack it up now.

I was afraid of also the other transformations in:
     A == 0? A : -A    same as -A
     A != 0? A : -A    same as A
     A >= 0? A : -A    same as abs (A)
     A > 0?  A : -A    same as abs (A)
     A <= 0? A : -A    same as -abs (A)
     A < 0?  A : -A    same as -abs (A)
but only the second one doesn't add ABS_EXPR and removes the negation, but
in that case the negation would happen only for 0, thus it would be fine.

	Jakub

On Tue, 25 Mar 2014, Jakub Jelinek wrote:

> On Tue, Mar 25, 2014 at 10:09:26AM +0100, Richard Biener wrote:
> > On Tue, 25 Mar 2014, Jakub Jelinek wrote:
> > 
> > > Hi!
> > > 
> > > While Marek has been debugging while some perl test fails when perl is built
> > > with GCC 4.9, we've discovered that it is because of undefined behavior in
> > > it:
> > > ...
> > >   && (((UV)1 << NV_PRESERVES_UV_BITS) >
> > >       (UV)(SvIVX(sv) > 0 ? SvIVX(sv) : -SvIVX(sv)))
> > > where SvIVX(sv) can be LONG_MIN, at which point there is undefined behavior
> > > on the negation, but -fsanitize=undefined did detect only other issues in
> > > the same source file and not this one, because fold-const.c folded it into
> > > ABS_EXPR early.
> > > 
> > > This patch disables such folding, because all the A op 0 ? A : -A
> > > operations this if is trying to optimize will need instrumentation with
> > > -fsanitize=signed-integer-overflow.
> > > 
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > Can't one form a valid constant expression using this?  Also isn't
> > ABS_EXPR undefined for LONG_MIN as well?  So why is ubsan not
> > instrumenting that instead?
> 
> Well, for a constant expression A would need to fold to a constant, but then
> A op 0 would fold to a constant too and so would the whole A op 0 ? A : -A.
> 
> Instrumenting ABS_EXPR is certainly possible, but definitely more code than
> this patch.  I could revert this patch and implement it for 5.0 as ABS_EXPR
> instrumentation.  Of course, if you think we should do the instrumentation
> of ABS_EXPR, I can hack it up now.
> 
> I was afraid of also the other transformations in:
>      A == 0? A : -A    same as -A
>      A != 0? A : -A    same as A
>      A >= 0? A : -A    same as abs (A)
>      A > 0?  A : -A    same as abs (A)
>      A <= 0? A : -A    same as -abs (A)
>      A < 0?  A : -A    same as -abs (A)
> but only the second one doesn't add ABS_EXPR and removes the negation, but
> in that case the negation would happen only for 0, thus it would be fine.

Yes, all transforms in fold-const would be invalid if the result doesn't
behave in the same way wrt overflow.  Thus you really should instrument
ABS_EXPR - you can treat it as A > 0 ? A : -A if that simplifies it.

I don't like the conditions that disable stuff based on sanitization.

Instrumenting ABS_EXPR shouldn't be too difficult.

Richard.