On Fri, Sep 13, 2013 at 08:01:36PM +0200, Marek Polacek wrote:
> 2013-09-13 Marek Polacek <pola...@redhat.com>
>
> PR sanitizer/58413
> c-family/
> * c-ubsan.c (ubsan_instrument_shift): Don't instrument
> an expression if we can prove it is correct.
>
> testsuite/
> * c-c++-common/ubsan/shift-4.c: New test.
> * c-c++-common/ubsan/shift-5.c: New test.
> * gcc.dg/ubsan/c-shift-1.c: New test.
>
> --- gcc/c-family/c-ubsan.c.mp3 2013-09-13 19:19:55.410925155 +0200
> +++ gcc/c-family/c-ubsan.c 2013-09-13 19:20:16.766006242 +0200
> @@ -104,6 +104,40 @@ ubsan_instrument_shift (location_t loc,
> tree uprecm1 = build_int_cst (op1_utype, op0_prec - 1);
> tree precm1 = build_int_cst (type1, op0_prec - 1);
>
> + /* If OP0 is of an unsigned type, we may prove that OP1 is not
> + greater than UPRECM1 (and not negative); in that case we can
> + bail out early. */
> + if (TYPE_UNSIGNED (type0)
> + && TREE_CODE (op1) == INTEGER_CST
> + && tree_int_cst_sgn (op1) != -1
> + && !tree_int_cst_lt (uprecm1, op1))
> + return NULL_TREE;
> +
> + /* Even when OP0 is of a signed type, we may prove that there's
> + nothing wrong with the shift if both operands are INTEGER_CSTs
> + and wouldn't trigger UB. We do this only for C.
> + XXX Should we treat ANSI C specially wrt 1 << 31? */
> + if (TREE_CODE (op0) == INTEGER_CST
> + && TREE_CODE (op1) == INTEGER_CST
> + && !TYPE_UNSIGNED (type0)
> + && tree_int_cst_sgn (op1) != -1
> + && !tree_int_cst_lt (uprecm1, op1)
> + && !c_dialect_cxx ())
> + {
> + /* If this is a right shift, we can return now. */
> + if (code == RSHIFT_EXPR)
> + return NULL_TREE;
> +
> + /* Otherwise see comment below. */
> + tree x = fold_convert_loc (loc, unsigned_type_for (type0), op0);
> + x = fold_build2 (RSHIFT_EXPR, TREE_TYPE (x), x,
> + fold_build2 (MINUS_EXPR, TREE_TYPE (precm1), precm1,
> + op1));
> + if (integer_zerop (x))
> + return NULL_TREE;
> + }
> +
> + /* Ok, we have to do the instrumentation. */
I'd say the above is going to be a maintainance nightmare, with all the code
duplication. And you are certainly going to miss cases that way,
e.g.
void
foo (void)
{
int A[-2 / -1] = {};
}
I'd say instead of adding all this, you should just at the right spot insert
if (integer_zerop (t)) return NULL_TREE; or similar.
For shift instrumentation, I guess you could add
if (integer_zerop (t) && (tt == NULL_TREE || integer_zerop (tt)))
return NULL_TREE;
right before:
t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), op0, t);
> +/* PR sanitizer/58413 */
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=shift -w" } */
> +
> +int x = 7;
> +int
> +main (void)
> +{
> + /* All of the following should pass. */
> + int A[128 >> 5] = {};
> + int B[128 << 5] = {};
> +
> + static int e =
> + ((int)
> + (0x00000000 | ((31 & ((1 << (4)) - 1)) << (((15) + 6) + 4)) |
> + ((0) << ((15) + 6)) | ((0) << (15))));
This relies on int32plus, so needs to be /* { dg-do run { target int32plus } }
*/
> --- gcc/testsuite/c-c++-common/ubsan/shift-5.c.mp3 2013-09-13
> 18:25:06.195847144 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/shift-5.c 2013-09-13
> 19:16:38.990211229 +0200
> @@ -0,0 +1,21 @@
> +/* { dg-do compile} */
> +/* { dg-options "-fsanitize=shift -w" } */
> +/* { dg-shouldfail "ubsan" } */
> +
> +int x;
> +int
> +main (void)
> +{
> + /* None of the following should pass. */
> + switch (x)
> + {
> + case 1 >> -1: /* { dg-error "" } */
> + case -1 >> -1: /* { dg-error "" } */
> + case 1 << -1: /* { dg-error "" } */
> + case -1 << -1: /* { dg-error "" } */
> + case -1 >> 200: /* { dg-error "" } */
> + case 1 << 200: /* { dg-error "" } */
Can't you fill in the error you are expecting, or is the problem
that the wording is very different between C and C++?
Jakub
