On Tue, 8 Nov 2016, Jakub Jelinek wrote:
> Hi!
>
> In the libsanitizer merge Maxim posted I've noticed that llvm split
> the -fsanitize=shift option into suboption (at least one really weirdly
> named, I have never seen the shift count being called exponent), this
> patch implements the same with the same option names for gcc.
> The interesting part is if the two suboptions disagree on
> -fsanitize-recover, then we need to emit two checks and two calls (one with
> _abort, one without).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Richard.
> 2016-11-08 Jakub Jelinek <ja...@redhat.com>
>
> * flag-types.h (enum sanitize_code): Add SANITIZE_SHIFT_BASE
> and SANITIZE_SHIFT_EXPONENT, change SANITIZE_SHIFT to bitwise
> or of them, renumber other enumerators.
> * opts.c (sanitizer_opts): Add shift-base and shift-exponent.
> * doc/invoke.texi: Document -fsanitize=shift-base and
> -fsanitize-shift-exponent, document -fsanitize=shift as
> having those 2 suboptions.
> c-family/
> * c-ubsan.c (ubsan_instrument_shift): Handle split
> -fsanitize=shift-base and -fsanitize=shift-exponent.
> testsuite/
> * gcc.dg/ubsan/c99-shift-3.c: New test.
> * gcc.dg/ubsan/c99-shift-4.c: New test.
> * gcc.dg/ubsan/c99-shift-5.c: New test.
> * gcc.dg/ubsan/c99-shift-6.c: New test.
>
> --- gcc/flag-types.h.jj 2016-11-07 14:04:29.129755670 +0100
> +++ gcc/flag-types.h 2016-11-07 18:50:54.989652650 +0100
> @@ -211,24 +211,26 @@ enum sanitize_code {
> /* LeakSanitizer. */
> SANITIZE_LEAK = 1UL << 4,
> /* UndefinedBehaviorSanitizer. */
> - SANITIZE_SHIFT = 1UL << 5,
> - SANITIZE_DIVIDE = 1UL << 6,
> - SANITIZE_UNREACHABLE = 1UL << 7,
> - SANITIZE_VLA = 1UL << 8,
> - SANITIZE_NULL = 1UL << 9,
> - SANITIZE_RETURN = 1UL << 10,
> - SANITIZE_SI_OVERFLOW = 1UL << 11,
> - SANITIZE_BOOL = 1UL << 12,
> - SANITIZE_ENUM = 1UL << 13,
> - SANITIZE_FLOAT_DIVIDE = 1UL << 14,
> - SANITIZE_FLOAT_CAST = 1UL << 15,
> - SANITIZE_BOUNDS = 1UL << 16,
> - SANITIZE_ALIGNMENT = 1UL << 17,
> - SANITIZE_NONNULL_ATTRIBUTE = 1UL << 18,
> - SANITIZE_RETURNS_NONNULL_ATTRIBUTE = 1UL << 19,
> - SANITIZE_OBJECT_SIZE = 1UL << 20,
> - SANITIZE_VPTR = 1UL << 21,
> - SANITIZE_BOUNDS_STRICT = 1UL << 22,
> + SANITIZE_SHIFT_BASE = 1UL << 5,
> + SANITIZE_SHIFT_EXPONENT = 1UL << 6,
> + SANITIZE_DIVIDE = 1UL << 7,
> + SANITIZE_UNREACHABLE = 1UL << 8,
> + SANITIZE_VLA = 1UL << 9,
> + SANITIZE_NULL = 1UL << 10,
> + SANITIZE_RETURN = 1UL << 11,
> + SANITIZE_SI_OVERFLOW = 1UL << 12,
> + SANITIZE_BOOL = 1UL << 13,
> + SANITIZE_ENUM = 1UL << 14,
> + SANITIZE_FLOAT_DIVIDE = 1UL << 15,
> + SANITIZE_FLOAT_CAST = 1UL << 16,
> + SANITIZE_BOUNDS = 1UL << 17,
> + SANITIZE_ALIGNMENT = 1UL << 18,
> + SANITIZE_NONNULL_ATTRIBUTE = 1UL << 19,
> + SANITIZE_RETURNS_NONNULL_ATTRIBUTE = 1UL << 20,
> + SANITIZE_OBJECT_SIZE = 1UL << 21,
> + SANITIZE_VPTR = 1UL << 22,
> + SANITIZE_BOUNDS_STRICT = 1UL << 23,
> + SANITIZE_SHIFT = SANITIZE_SHIFT_BASE | SANITIZE_SHIFT_EXPONENT,
> SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE |
> SANITIZE_UNREACHABLE
> | SANITIZE_VLA | SANITIZE_NULL | SANITIZE_RETURN
> | SANITIZE_SI_OVERFLOW | SANITIZE_BOOL | SANITIZE_ENUM
> --- gcc/opts.c.jj 2016-11-07 14:04:29.131755645 +0100
> +++ gcc/opts.c 2016-11-07 18:50:54.990652637 +0100
> @@ -1477,6 +1477,8 @@ const struct sanitizer_opts_s sanitizer_
> SANITIZER_OPT (thread, SANITIZE_THREAD, false),
> SANITIZER_OPT (leak, SANITIZE_LEAK, false),
> SANITIZER_OPT (shift, SANITIZE_SHIFT, true),
> + SANITIZER_OPT (shift-base, SANITIZE_SHIFT_BASE, true),
> + SANITIZER_OPT (shift-exponent, SANITIZE_SHIFT_EXPONENT, true),
> SANITIZER_OPT (integer-divide-by-zero, SANITIZE_DIVIDE, true),
> SANITIZER_OPT (undefined, SANITIZE_UNDEFINED, true),
> SANITIZER_OPT (unreachable, SANITIZE_UNREACHABLE, false),
> --- gcc/doc/invoke.texi.jj 2016-11-07 14:04:28.853759109 +0100
> +++ gcc/doc/invoke.texi 2016-11-07 18:50:54.998652536 +0100
> @@ -10555,6 +10555,21 @@ at runtime. Current suboptions are:
> This option enables checking that the result of a shift operation is
> not undefined. Note that what exactly is considered undefined differs
> slightly between C and C++, as well as between ISO C90 and C99, etc.
> +This option has two suboptions, @option{-fsanitize=shift-base} and
> +@option{-fsanitize=shift-exponent}.
> +
> +@item -fsanitize=shift-exponent
> +@opindex fsanitize=shift-exponent
> +This option enables checking that the second argument of a shift operation
> +is not negative and is smaller than the precision of the promoted first
> +argument.
> +
> +@item -fsanitize=shift-base
> +@opindex fsanitize=shift-base
> +If the second argument of a shift operation is within range, check that the
> +result of a shift operation is not undefined. Note that what exactly is
> +considered undefined differs slightly between C and C++, as well as between
> +ISO C90 and C99, etc.
>
> @item -fsanitize=integer-divide-by-zero
> @opindex fsanitize=integer-divide-by-zero
> --- gcc/c-family/c-ubsan.c.jj 2016-11-07 14:04:29.102756006 +0100
> +++ gcc/c-family/c-ubsan.c 2016-11-07 18:50:54.991652625 +0100
> @@ -130,7 +130,8 @@ ubsan_instrument_shift (location_t loc,
> /* If this is not a signed operation, don't perform overflow checks.
> Also punt on bit-fields. */
> if (TYPE_OVERFLOW_WRAPS (type0)
> - || GET_MODE_BITSIZE (TYPE_MODE (type0)) != TYPE_PRECISION (type0))
> + || GET_MODE_BITSIZE (TYPE_MODE (type0)) != TYPE_PRECISION (type0)
> + || (flag_sanitize & SANITIZE_SHIFT_BASE) == 0)
> ;
>
> /* For signed x << y, in C99/C11, the following:
> @@ -171,8 +172,27 @@ ubsan_instrument_shift (location_t loc,
> /* In case we have a SAVE_EXPR in a conditional context, we need to
> make sure it gets evaluated before the condition. */
> t = fold_build2 (COMPOUND_EXPR, TREE_TYPE (t), unshare_expr (op0), t);
> - t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, t,
> - tt ? tt : integer_zero_node);
> +
> + enum sanitize_code recover_kind = SANITIZE_SHIFT_EXPONENT;
> + tree else_t = void_node;
> + if (tt)
> + {
> + if ((flag_sanitize & SANITIZE_SHIFT_EXPONENT) == 0)
> + {
> + t = fold_build1 (TRUTH_NOT_EXPR, boolean_type_node, t);
> + t = fold_build2 (TRUTH_AND_EXPR, boolean_type_node, t, tt);
> + recover_kind = SANITIZE_SHIFT_BASE;
> + }
> + else
> + {
> + if (flag_sanitize_undefined_trap_on_error
> + || ((!(flag_sanitize_recover & SANITIZE_SHIFT_EXPONENT))
> + == (!(flag_sanitize_recover & SANITIZE_SHIFT_BASE))))
> + t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, t, tt);
> + else
> + else_t = tt;
> + }
> + }
>
> if (flag_sanitize_undefined_trap_on_error)
> tt = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0);
> @@ -185,7 +205,7 @@ ubsan_instrument_shift (location_t loc,
> data = build_fold_addr_expr_loc (loc, data);
>
> enum built_in_function bcode
> - = (flag_sanitize_recover & SANITIZE_SHIFT)
> + = (flag_sanitize_recover & recover_kind)
> ? BUILT_IN_UBSAN_HANDLE_SHIFT_OUT_OF_BOUNDS
> : BUILT_IN_UBSAN_HANDLE_SHIFT_OUT_OF_BOUNDS_ABORT;
> tt = builtin_decl_explicit (bcode);
> @@ -193,8 +213,22 @@ ubsan_instrument_shift (location_t loc,
> op1 = unshare_expr (op1);
> tt = build_call_expr_loc (loc, tt, 3, data, ubsan_encode_value (op0),
> ubsan_encode_value (op1));
> + if (else_t != void_node)
> + {
> + bcode = (flag_sanitize_recover & SANITIZE_SHIFT_BASE)
> + ? BUILT_IN_UBSAN_HANDLE_SHIFT_OUT_OF_BOUNDS
> + : BUILT_IN_UBSAN_HANDLE_SHIFT_OUT_OF_BOUNDS_ABORT;
> + tree else_tt = builtin_decl_explicit (bcode);
> + op0 = unshare_expr (op0);
> + op1 = unshare_expr (op1);
> + else_tt = build_call_expr_loc (loc, else_tt, 3, data,
> + ubsan_encode_value (op0),
> + ubsan_encode_value (op1));
> + else_t = fold_build3 (COND_EXPR, void_type_node, else_t,
> + else_tt, void_node);
> + }
> }
> - t = fold_build3 (COND_EXPR, void_type_node, t, tt, void_node);
> + t = fold_build3 (COND_EXPR, void_type_node, t, tt, else_t);
>
> return t;
> }
> --- gcc/testsuite/gcc.dg/ubsan/c99-shift-3.c.jj 2016-11-07
> 18:54:33.916880052 +0100
> +++ gcc/testsuite/gcc.dg/ubsan/c99-shift-3.c 2016-11-07 19:08:08.318566032
> +0100
> @@ -0,0 +1,18 @@
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=shift-base -fno-sanitize=shift-exponent -w
> -std=c99" } */
> +
> +int
> +main (void)
> +{
> + int a = -42;
> + int b = -43;
> + volatile int c = 129;
> + int d = 1;
> + a << 1;
> + b << c;
> + a << 1;
> + d <<= 31;
> +}
> +/* { dg-output "left shift of negative value -42\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*left shift of negative value
> -42\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*left shift of 1 by 31 places cannot be represented
> in type 'int'" } */
> --- gcc/testsuite/gcc.dg/ubsan/c99-shift-4.c.jj 2016-11-07
> 18:55:20.786286472 +0100
> +++ gcc/testsuite/gcc.dg/ubsan/c99-shift-4.c 2016-11-07 19:16:59.049847278
> +0100
> @@ -0,0 +1,18 @@
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=shift-exponent -fno-sanitize=shift-base -w
> -std=c99" } */
> +
> +int
> +main (void)
> +{
> + int a = -42;
> + int b = -43;
> + volatile int c = 129;
> + int d = 1;
> + b << c;
> + a << 1;
> + a << 1;
> + d <<= 31;
> + b << (c + 1);
> +}
> +/* { dg-output "shift exponent 129 is too large for \[^\n\r]*-bit type
> 'int'\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*shift exponent 130 is too large for \[^\n\r]*-bit
> type 'int'" } */
> --- gcc/testsuite/gcc.dg/ubsan/c99-shift-5.c.jj 2016-11-07
> 19:11:29.554018440 +0100
> +++ gcc/testsuite/gcc.dg/ubsan/c99-shift-5.c 2016-11-07 19:16:11.676446989
> +0100
> @@ -0,0 +1,21 @@
> +/* { dg-do run } */
> +/* { dg-shouldfail "ubsan" } */
> +/* { dg-options "-fsanitize=shift -fsanitize-recover=shift-base
> -fno-sanitize-recover=shift-exponent -w -std=c99" } */
> +
> +int
> +main (void)
> +{
> + int a = -42;
> + int b = -43;
> + volatile int c = 129;
> + int d = 1;
> + a << 1;
> + a << 1;
> + d <<= 31;
> + b << c;
> + a << 1;
> +}
> +/* { dg-output "left shift of negative value -42\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*left shift of negative value
> -42\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*left shift of 1 by 31 places cannot be represented
> in type 'int'\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*shift exponent 129 is too large for \[^\n\r]*-bit
> type 'int'" } */
> --- gcc/testsuite/gcc.dg/ubsan/c99-shift-6.c.jj 2016-11-07
> 19:14:21.190845651 +0100
> +++ gcc/testsuite/gcc.dg/ubsan/c99-shift-6.c 2016-11-07 19:15:53.807673194
> +0100
> @@ -0,0 +1,18 @@
> +/* { dg-do run } */
> +/* { dg-shouldfail "ubsan" } */
> +/* { dg-options "-fsanitize=shift -fsanitize-recover=shift-exponent
> -fno-sanitize-recover=shift-base -w -std=c99" } */
> +
> +int
> +main (void)
> +{
> + int a = -42;
> + int b = -43;
> + volatile int c = 129;
> + 1 << c;
> + 1 << (c + 1);
> + a << 1;
> + b << 1;
> +}
> +/* { dg-output "shift exponent 129 is too large for \[^\n\r]*-bit type
> 'int'\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*shift exponent 130 is too large for \[^\n\r]*-bit
> type 'int'\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*left shift of negative value
> -42\[^\n\r]*(\n|\r\n|\r)" } */
>
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
