| Message ID | 20141118220447.GT1745@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jakub@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.000000000 +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 > >
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
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.
--- gcc/internal-fn.c.jj 2014-11-12 13:28:47.000000000 +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; + } +}
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 <jakub@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. Jakub