Re: [PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520)

2014-11-19 Thread Richard Biener 
On Tue, 18 Nov 2014, Jakub Jelinek wrote:

 Hi!
 
 Apparently, expand_expr with EXPR_WRITE can return
 a SUBREG with SUBREG_PROMOTED_VAR_P set on it.  For

Huh, that looks bogus to me.  But of course I know nothing
(read: not enough) to really tell.  Eric?

Richard.

 UBSAN_CHECK_{ADD,SUB,MUL} expansion, I've been doing just
 emit_move_insn into it, which apparently is wrong in that case,
 store_expr instead uses convert_move for it.  The
 {ADD,SUB,MUL}_OVERFLOW (i.e. __builtin_*_overflow) expansion
 shouldn't need it, as the result is complex and complex integers
 aren't promoted that way.  As store_expr* uses a tree expression
 to store, while I have rtx, I just wrote a short helper function
 for this.
 
 Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
 
 2014-11-18  Jakub Jelinek  ja...@redhat.com
 
   PR sanitizer/63520
   * internal-fn.c (expand_ubsan_result_store): New function.
   (expand_addsub_overflow, expand_neg_overflow, expand_mul_overflow):
   Use it instead of just emit_move_insn.
 
   * c-c++-common/ubsan/pr63520.c: New test.
 
 --- gcc/internal-fn.c.jj  2014-11-12 13:28:47.0 +0100
 +++ gcc/internal-fn.c 2014-11-18 15:35:46.395916823 +0100
 @@ -395,6 +395,21 @@ expand_arith_overflow_result_store (tree
write_complex_part (target, lres, false);
  }
  
 +/* Helper for expand_*_overflow.  Store RES into TARGET.  */
 +
 +static void
 +expand_ubsan_result_store (rtx target, rtx res)
 +{
 +  if (GET_CODE (target) == SUBREG  SUBREG_PROMOTED_VAR_P (target))
 +/* If this is a scalar in a register that is stored in a wider mode   
 +   than the declared mode, compute the result into its declared mode
 +   and then convert to the wider mode.  Our value is the computed
 +   expression.  */
 +convert_move (SUBREG_REG (target), res, SUBREG_PROMOTED_SIGN (target));
 +  else
 +emit_move_insn (target, res);
 +}
 +
  /* Add sub/add overflow checking to the statement STMT.
 CODE says whether the operation is +, or -.  */
  
 @@ -809,7 +824,7 @@ expand_addsub_overflow (location_t loc,
if (lhs)
  {
if (is_ubsan)
 - emit_move_insn (target, res);
 + expand_ubsan_result_store (target, res);
else
   {
 if (do_xor)
 @@ -904,7 +919,7 @@ expand_neg_overflow (location_t loc, tre
if (lhs)
  {
if (is_ubsan)
 - emit_move_insn (target, res);
 + expand_ubsan_result_store (target, res);
else
   expand_arith_overflow_result_store (lhs, target, mode, res);
  }
 @@ -1590,7 +1605,7 @@ expand_mul_overflow (location_t loc, tre
if (lhs)
  {
if (is_ubsan)
 - emit_move_insn (target, res);
 + expand_ubsan_result_store (target, res);
else
   expand_arith_overflow_result_store (lhs, target, mode, res);
  }
 --- gcc/testsuite/c-c++-common/ubsan/pr63520.c.jj 2014-11-18 
 15:40:07.271273710 +0100
 +++ gcc/testsuite/c-c++-common/ubsan/pr63520.c2014-11-18 
 15:40:40.971673904 +0100
 @@ -0,0 +1,16 @@
 +/* PR sanitizer/63520 */
 +/* { dg-do compile } */
 +/* { dg-options -fsanitize=undefined } */
 +
 +int a;
 +
 +void
 +foo (void)
 +{
 +  while (1)
 +{
 +  if (a == 1)
 + break;
 +  a -= 1;
 +}
 +}
 
 
   Jakub
 
 

-- 
Richard Biener rguent...@suse.de
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendoerffer, HRB 21284
(AG Nuernberg)
Maxfeldstrasse 5, 90409 Nuernberg, Germany

Re: [PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520)

2014-11-19 Thread Jakub Jelinek 
On Wed, Nov 19, 2014 at 10:02:18AM +0100, Richard Biener wrote:
 On Tue, 18 Nov 2014, Jakub Jelinek wrote:
  Apparently, expand_expr with EXPR_WRITE can return
  a SUBREG with SUBREG_PROMOTED_VAR_P set on it.  For
 
 Huh, that looks bogus to me.  But of course I know nothing
 (read: not enough) to really tell.  Eric?

I've tried to look where it comes from, and it dates back to r2xxx
or so, so forever.
And store_expr has a large:
  else if (GET_CODE (target) == SUBREG  SUBREG_PROMOTED_VAR_P (target))
/* If this is a scalar in a register that is stored in a wider mode
   than the declared mode, compute the result into its declared mode
   and then convert to the wider mode.  Our value is the computed
   expression.  */
handling block.  For targets with only word sized operations something
like that actually makes a lot of sense, I have just not been aware of that.

Jakub

Re: [PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520)

2014-11-19 Thread Richard Biener 
On Wed, 19 Nov 2014, Jakub Jelinek wrote:

 On Wed, Nov 19, 2014 at 10:02:18AM +0100, Richard Biener wrote:
  On Tue, 18 Nov 2014, Jakub Jelinek wrote:
   Apparently, expand_expr with EXPR_WRITE can return
   a SUBREG with SUBREG_PROMOTED_VAR_P set on it.  For
  
  Huh, that looks bogus to me.  But of course I know nothing
  (read: not enough) to really tell.  Eric?
 
 I've tried to look where it comes from, and it dates back to r2xxx
 or so, so forever.
 And store_expr has a large:
   else if (GET_CODE (target) == SUBREG  SUBREG_PROMOTED_VAR_P (target))
 /* If this is a scalar in a register that is stored in a wider mode
than the declared mode, compute the result into its declared mode
and then convert to the wider mode.  Our value is the computed
expression.  */
 handling block.  For targets with only word sized operations something
 like that actually makes a lot of sense, I have just not been aware of that.

Ok.  Then the patch is ok.

Thanks,
Richard.

[PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520)

2014-11-18 Thread Jakub Jelinek 
Hi!

Apparently, expand_expr with EXPR_WRITE can return
a SUBREG with SUBREG_PROMOTED_VAR_P set on it.  For
UBSAN_CHECK_{ADD,SUB,MUL} expansion, I've been doing just
emit_move_insn into it, which apparently is wrong in that case,
store_expr instead uses convert_move for it.  The
{ADD,SUB,MUL}_OVERFLOW (i.e. __builtin_*_overflow) expansion
shouldn't need it, as the result is complex and complex integers
aren't promoted that way.  As store_expr* uses a tree expression
to store, while I have rtx, I just wrote a short helper function
for this.

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

2014-11-18  Jakub Jelinek  ja...@redhat.com

PR sanitizer/63520
* internal-fn.c (expand_ubsan_result_store): New function.
(expand_addsub_overflow, expand_neg_overflow, expand_mul_overflow):
Use it instead of just emit_move_insn.

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

--- gcc/internal-fn.c.jj2014-11-12 13:28:47.0 +0100
+++ gcc/internal-fn.c   2014-11-18 15:35:46.395916823 +0100
@@ -395,6 +395,21 @@ expand_arith_overflow_result_store (tree
   write_complex_part (target, lres, false);
 }
 
+/* Helper for expand_*_overflow.  Store RES into TARGET.  */
+
+static void
+expand_ubsan_result_store (rtx target, rtx res)
+{
+  if (GET_CODE (target) == SUBREG  SUBREG_PROMOTED_VAR_P (target))
+/* If this is a scalar in a register that is stored in a wider mode   
+   than the declared mode, compute the result into its declared mode
+   and then convert to the wider mode.  Our value is the computed
+   expression.  */
+convert_move (SUBREG_REG (target), res, SUBREG_PROMOTED_SIGN (target));
+  else
+emit_move_insn (target, res);
+}
+
 /* Add sub/add overflow checking to the statement STMT.
CODE says whether the operation is +, or -.  */
 
@@ -809,7 +824,7 @@ expand_addsub_overflow (location_t loc,
   if (lhs)
 {
   if (is_ubsan)
-   emit_move_insn (target, res);
+   expand_ubsan_result_store (target, res);
   else
{
  if (do_xor)
@@ -904,7 +919,7 @@ expand_neg_overflow (location_t loc, tre
   if (lhs)
 {
   if (is_ubsan)
-   emit_move_insn (target, res);
+   expand_ubsan_result_store (target, res);
   else
expand_arith_overflow_result_store (lhs, target, mode, res);
 }
@@ -1590,7 +1605,7 @@ expand_mul_overflow (location_t loc, tre
   if (lhs)
 {
   if (is_ubsan)
-   emit_move_insn (target, res);
+   expand_ubsan_result_store (target, res);
   else
expand_arith_overflow_result_store (lhs, target, mode, res);
 }
--- gcc/testsuite/c-c++-common/ubsan/pr63520.c.jj   2014-11-18 
15:40:07.271273710 +0100
+++ gcc/testsuite/c-c++-common/ubsan/pr63520.c  2014-11-18 15:40:40.971673904 
+0100
@@ -0,0 +1,16 @@
+/* PR sanitizer/63520 */
+/* { dg-do compile } */
+/* { dg-options -fsanitize=undefined } */
+
+int a;
+
+void
+foo (void)
+{
+  while (1)
+{
+  if (a == 1)
+   break;
+  a -= 1;
+}
+}


Jakub