* [PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520)
@ 2014-11-18 22:14 Jakub Jelinek
2014-11-19 9:17 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2014-11-18 22:14 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
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.
--- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520)
2014-11-18 22:14 [PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520) Jakub Jelinek
@ 2014-11-19 9:17 ` Richard Biener
2014-11-19 9:29 ` Jakub Jelinek
0 siblings, 1 reply; 4+ messages in thread
From: Richard Biener @ 2014-11-19 9:17 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, ebotcazou
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
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendoerffer, HRB 21284
(AG Nuernberg)
Maxfeldstrasse 5, 90409 Nuernberg, Germany
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520)
2014-11-19 9:17 ` Richard Biener
@ 2014-11-19 9:29 ` Jakub Jelinek
2014-11-19 10:23 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2014-11-19 9:29 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, ebotcazou
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520)
2014-11-19 9:29 ` Jakub Jelinek
@ 2014-11-19 10:23 ` Richard Biener
0 siblings, 0 replies; 4+ messages in thread
From: Richard Biener @ 2014-11-19 10:23 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, ebotcazou
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-11-19 9:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-18 22:14 [PATCH] Fix ubsan -fsanitize=signed-integer-overflow expansion (PR sanitizer/63520) Jakub Jelinek
2014-11-19 9:17 ` Richard Biener
2014-11-19 9:29 ` Jakub Jelinek
2014-11-19 10:23 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).