This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Add signed integer overflow checking to ubsan
- From: Jakub Jelinek <jakub at redhat dot com>
- To: Jeff Law <law at redhat dot com>
- Cc: Marek Polacek <polacek at redhat dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 3 Dec 2013 23:05:24 +0100
- Subject: Re: [PATCH] Add signed integer overflow checking to ubsan
- Authentication-results: sourceware.org; auth=none
- References: <20131122095416 dot GG30062 at redhat dot com> <20131127081302 dot GD31608 at redhat dot com> <529E49A9 dot 9020100 at redhat dot com>
- Reply-to: Jakub Jelinek <jakub at redhat dot com>
On Tue, Dec 03, 2013 at 02:14:17PM -0700, Jeff Law wrote:
> >--- gcc/ubsan.h.mp 2013-11-27 08:46:28.046629473 +0100
> >+++ gcc/ubsan.h 2013-11-27 08:46:57.578753342 +0100
> >@@ -21,6 +21,12 @@ along with GCC; see the file COPYING3.
> > #ifndef GCC_UBSAN_H
> > #define GCC_UBSAN_H
> >
> >+/* From predict.c. */
> >+#define PROB_VERY_UNLIKELY (REG_BR_PROB_BASE / 2000 - 1)
> >+#define PROB_EVEN (REG_BR_PROB_BASE / 2)
> >+#define PROB_VERY_LIKELY (REG_BR_PROB_BASE - PROB_VERY_UNLIKELY)
> >+#define PROB_ALWAYS (REG_BR_PROB_BASE)
> Seems like this should factor out rather than get duplicated.
Yeah, the question is where, predict.h?
> >+/* Add sub/add overflow checking to the statement STMT.
> >+ CODE says whether the operation is +, or -. */
> >+
> >+void
> >+ubsan_expand_si_overflow_addsub_check (tree_code code, gimple stmt)
> Does this need to use Jakub's recent changes for pr58864 (pending
> stack adjustment stuff). The code seems to have the same kind of
> structure Jakub was cleaning up already.
No, the code always emits conditional jumps and one of the branches
will do a call, so we always want no pending stack adjustment
before the conditional jump and again no pending stack adjustment
after the library call, so that we don't make the stack pointer
inconsistent.
> >--- gcc/fold-const.c.mp 2013-11-27 08:46:27.941629031 +0100
> >+++ gcc/fold-const.c 2013-11-27 08:46:57.548753215 +0100
> >@@ -10335,14 +10335,16 @@ fold_binary_loc (location_t loc,
> >
> > case PLUS_EXPR:
> > /* A + (-B) -> A - B */
> >- if (TREE_CODE (arg1) == NEGATE_EXPR)
> >+ if (TREE_CODE (arg1) == NEGATE_EXPR
> >+ && (flag_sanitize & SANITIZE_SI_OVERFLOW) == 0)
> > return fold_build2_loc (loc, MINUS_EXPR, type,
> > fold_convert_loc (loc, type, arg0),
> > fold_convert_loc (loc, type,
> > TREE_OPERAND (arg1, 0)));
> > /* (-A) + B -> B - A */
> > if (TREE_CODE (arg0) == NEGATE_EXPR
> >- && reorder_operands_p (TREE_OPERAND (arg0, 0), arg1))
> >+ && reorder_operands_p (TREE_OPERAND (arg0, 0), arg1)
> >+ && (flag_sanitize & SANITIZE_SI_OVERFLOW) == 0)
> > return fold_build2_loc (loc, MINUS_EXPR, type,
> > fold_convert_loc (loc, type, arg1),
> > fold_convert_loc (loc, type,
> Presumably because you want to see the original arithmetic to
> instrument it?
Yes. Dunno if Marek has added testcase for it, but if we have say:
int a = -10, b = INT_MIN;
then
int c = a - b;
isn't supposed to overflow, while
int d = a + (-b);
is. The signed integer overflow is in the latter case already
on the negation. Without -fsanitize=undefined we just assume
that it is undefined behavior and therefore ignore that case,
but with -fsanitize=undefined we actually want to diagnose it at runtime to
the user.
Jakub