-fsanitize-recover=list

On Mon, Oct 13, 2014 at 02:47:07PM +0400, Yury Gribov wrote:
> On 09/30/2014 09:39 PM, Jakub Jelinek wrote:
> >LGTM, will hack it up soon in GCC then.
> 
> Do you plan to work on this in near future?

Here is only very lightly tested patch, didn't get to updating
documentation though, plus there is no testsuite coverage for it.
Supposedly, most of the tests that use -fno-sanitize-recover
or -fsanitize-recover in dg-options should be changed to use
-fno-sanitize-recover= or -fsanitize-recover (in some cases for
the kind that is enabled with -fsanitize= only, in other cases
perhaps for something covering that and some other options),
plus perhaps some new smallish tests that test that if you e.g.
do -fsanitize=undefined -fno-sanitize-recover=divide , that
you can recover from several say out of bound shifts, but that
the first divide will terminate, etc. 
Yuri or Marek, would you have spare time for that?

There is one not so nice thing, if one requests e.g.
-fsanitize=null,alignment -fno-sanitize-recover=null -fsanitize-recover=alignment
(or vice versa), as a single call is used for both alignment and null
checks, if both are tested, it needs to pick something, the patch
right now picks recovery rather than abort if the two bits
in flag_sanitize_recover disagree.  In theory, we could in that case
just use two separate calls rather than sharing one call, doing the
check and conditional branch to *_abort () first and if that wasn't true,
do the other check.

Also, the patch enables -fsanitize-recover by default for
-fsanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
but not for kernel-address and doesn't check
(flag_sanitize_recover & SANITIZE_KERNEL_ADDRESS) in asan.c, you'd
need to add that afterwards.

2014-10-17  Jakub Jelinek  <jakub@redhat.com>

	* common.opt (flag_sanitize_recover): New variable.
	(fsanitize-recover): Remove Var/Init, deprecate.
	(fsanitize-recover=): New option.
	* opts.c (finish_options): Use opts->x_flag_sanitize
	instead of flag_sanitize.  Formatting.
	(common_handle_option): Handle OPT_fsanitize_recover_
	and OPT_fsanitize_recover.  Use opts->x_flag_sanitize
	instead of flag_sanitize.
	* asan.c (pass_sanopt::execute): Fix up formatting.
	* ubsan.c (ubsan_expand_bounds_ifn, ubsan_expand_null_ifn,
	ubsan_expand_objsize_ifn, ubsan_build_overflow_builtin,
	instrument_bool_enum_load, ubsan_instrument_float_cast,
	instrument_nonnull_arg, instrument_nonnull_return): Check
	bits in flag_sanitize_recover bitmask instead of
	flag_sanitize_recover as bool flag.
c-family/
	* c-ubsan.c (ubsan_instrument_division, ubsan_instrument_shift,
	ubsan_instrument_vla): Check bits in flag_sanitize_recover bitmask
	instead of flag_sanitize_recover as bool flag.



	Jakub

On 10/17/2014 08:13 PM, Jakub Jelinek wrote:
> On Mon, Oct 13, 2014 at 02:47:07PM +0400, Yury Gribov wrote:
>> On 09/30/2014 09:39 PM, Jakub Jelinek wrote:
>>> LGTM, will hack it up soon in GCC then.
>>
>> Do you plan to work on this in near future?
>
> Here is only very lightly tested patch, didn't get to updating
> documentation though, plus there is no testsuite coverage for it.
> Supposedly, most of the tests that use -fno-sanitize-recover
> or -fsanitize-recover in dg-options should be changed to use
> -fno-sanitize-recover= or -fsanitize-recover (in some cases for
> the kind that is enabled with -fsanitize= only, in other cases
> perhaps for something covering that and some other options),
> plus perhaps some new smallish tests that test that if you e.g.
> do -fsanitize=undefined -fno-sanitize-recover=divide , that
> you can recover from several say out of bound shifts, but that
> the first divide will terminate, etc.
> Yuri or Marek, would you have spare time for that?

I'll take on this!

-Y

Hi Jakub,

I've just prepared a patch implementing -fsanitize-recover=<list> in
Clang (http://reviews.llvm.org/D6302), writing here to make sure we're
on
the same page w.r.t. flag semantics:

* -fsanitize-recover: Enable recovery for all checks enabled by
-fsanitize= which support it.
* -fno-sanitize-recover: Disable recovery for all checks.
* -fsanitize-recover=<list>: Enable recovery for all selected checks
or group of checks. It is forbidden to list unrecoverable sanitizers
here (e.g., "-fsanitize-recover=address" will produce an error).
* -fno-sanitize-recover=<list>: Disable recovery for selected checks
or group of checks.

We maintain the set of "recoverable" sanitizers. Initially it contains
all UBSan checks (except for "unreachable" and "return"). We then
update this set as we
parse flags listed above, from left to right. "-fsanitize-recover" are
"-fsanitize" flags are independent. If a sanitizer is listed as
recoverable, but is not enabled via -fsanitize=,
we just ignore corresponding -fsanitize-recover flag.

Examples:
  * $(CC) -fsanitize=undefined,thread -fsanitize-recover=thread  //
Use UBSan and TSan, both of them are recoverable (Note: in reality,
"-fsanitize-recover=thread" is not implemented in Clang, this example
just illustrates intended semantics).
  * $(CC) -fsanitize=undefined -fno-sanitize-recover
-fsanitize-recover=signed-integer-overflow // Recover only from
signed-integer-overflow check, all other UBSan checks are fatal.

Sounds good?

As for the generated code, I'm at the stage where I can implement the
following: if a single UBSan hander is used to report multiple error
kinds (__ubsan_handle_type_mismatch is used for
-fsanitize=null,alignment,object-size), and these kinds have different
recoverability, then we emit two handler calls like this:

$ (CC) -fsanitize=undefined -fno-sanitize-recover=alignment   //
"null" and "object-size" are recoverable by default.

bool IsNonNull = ...;
bool IsAligned = ...;
bool HasCorrectSize = ...;
bool OK = IsAligned && IsNonNull && HasCorrectSize;
if (UNLIKELY(!OK)) {
  // setup arguments for UBSan handler call
  if (!IsAligned) {
    __ubsan_handle_type_mismatch_abort(<args>);
    __builtin_unreachable();
  }
  __ubsan_handle_type_mismatch(<args>);
}
// user code



On Fri, Oct 17, 2014 at 9:13 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Oct 13, 2014 at 02:47:07PM +0400, Yury Gribov wrote:
>> On 09/30/2014 09:39 PM, Jakub Jelinek wrote:
>> >LGTM, will hack it up soon in GCC then.
>>
>> Do you plan to work on this in near future?
>
> Here is only very lightly tested patch, didn't get to updating
> documentation though, plus there is no testsuite coverage for it.
> Supposedly, most of the tests that use -fno-sanitize-recover
> or -fsanitize-recover in dg-options should be changed to use
> -fno-sanitize-recover= or -fsanitize-recover (in some cases for
> the kind that is enabled with -fsanitize= only, in other cases
> perhaps for something covering that and some other options),
> plus perhaps some new smallish tests that test that if you e.g.
> do -fsanitize=undefined -fno-sanitize-recover=divide , that
> you can recover from several say out of bound shifts, but that
> the first divide will terminate, etc.
> Yuri or Marek, would you have spare time for that?
>
> There is one not so nice thing, if one requests e.g.
> -fsanitize=null,alignment -fno-sanitize-recover=null -fsanitize-recover=alignment
> (or vice versa), as a single call is used for both alignment and null
> checks, if both are tested, it needs to pick something, the patch
> right now picks recovery rather than abort if the two bits
> in flag_sanitize_recover disagree.  In theory, we could in that case
> just use two separate calls rather than sharing one call, doing the
> check and conditional branch to *_abort () first and if that wasn't true,
> do the other check.
>
> Also, the patch enables -fsanitize-recover by default for
> -fsanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
> but not for kernel-address and doesn't check
> (flag_sanitize_recover & SANITIZE_KERNEL_ADDRESS) in asan.c, you'd
> need to add that afterwards.
>
> 2014-10-17  Jakub Jelinek  <jakub@redhat.com>
>
>         * common.opt (flag_sanitize_recover): New variable.
>         (fsanitize-recover): Remove Var/Init, deprecate.
>         (fsanitize-recover=): New option.
>         * opts.c (finish_options): Use opts->x_flag_sanitize
>         instead of flag_sanitize.  Formatting.
>         (common_handle_option): Handle OPT_fsanitize_recover_
>         and OPT_fsanitize_recover.  Use opts->x_flag_sanitize
>         instead of flag_sanitize.
>         * asan.c (pass_sanopt::execute): Fix up formatting.
>         * ubsan.c (ubsan_expand_bounds_ifn, ubsan_expand_null_ifn,
>         ubsan_expand_objsize_ifn, ubsan_build_overflow_builtin,
>         instrument_bool_enum_load, ubsan_instrument_float_cast,
>         instrument_nonnull_arg, instrument_nonnull_return): Check
>         bits in flag_sanitize_recover bitmask instead of
>         flag_sanitize_recover as bool flag.
> c-family/
>         * c-ubsan.c (ubsan_instrument_division, ubsan_instrument_shift,
>         ubsan_instrument_vla): Check bits in flag_sanitize_recover bitmask
>         instead of flag_sanitize_recover as bool flag.
>
> --- gcc/common.opt.jj   2014-10-17 08:47:21.000000000 +0200
> +++ gcc/common.opt      2014-10-17 17:45:41.816337133 +0200
> @@ -211,6 +211,10 @@ bool flag_opts_finished
>  Variable
>  unsigned int flag_sanitize
>
> +; What sanitizers should recover from errors
> +Variable
> +unsigned int flag_sanitize_recover = SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT
> +
>  ; Flag whether a prefix has been added to dump_base_name
>  Variable
>  bool dump_base_name_prefixed = false
> @@ -879,10 +883,14 @@ fsanitize=
>  Common Driver Report Joined
>  Select what to sanitize
>
> -fsanitize-recover
> -Common Report Var(flag_sanitize_recover) Init(1)
> +fsanitize-recover=
> +Common Report Joined
>  After diagnosing undefined behavior attempt to continue execution
>
> +fsanitize-recover
> +Common Report
> +This switch is deprecated; use -fsanitize-recover= instead
> +
>  fsanitize-undefined-trap-on-error
>  Common Report Var(flag_sanitize_undefined_trap_on_error) Init(0)
>  Use trap instead of a library function for undefined behavior sanitization
> --- gcc/opts.c.jj       2014-10-17 12:01:19.000000000 +0200
> +++ gcc/opts.c  2014-10-17 17:17:38.620375035 +0200
> @@ -879,17 +879,17 @@ finish_options (struct gcc_options *opts
>
>    /* Userspace and kernel ASan conflict with each other and with TSan.  */
>
> -  if ((flag_sanitize & SANITIZE_USER_ADDRESS)
> -      && (flag_sanitize & SANITIZE_KERNEL_ADDRESS))
> +  if ((opts->x_flag_sanitize & SANITIZE_USER_ADDRESS)
> +      && (opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS))
>      error_at (loc,
> -              "-fsanitize=address is incompatible with "
> -              "-fsanitize=kernel-address");
> +             "-fsanitize=address is incompatible with "
> +             "-fsanitize=kernel-address");
>
> -  if ((flag_sanitize & SANITIZE_ADDRESS)
> -      && (flag_sanitize & SANITIZE_THREAD))
> +  if ((opts->x_flag_sanitize & SANITIZE_ADDRESS)
> +      && (opts->x_flag_sanitize & SANITIZE_THREAD))
>      error_at (loc,
> -              "-fsanitize=address and -fsanitize=kernel-address "
> -              "are incompatible with -fsanitize=thread");
> +             "-fsanitize=address and -fsanitize=kernel-address "
> +             "are incompatible with -fsanitize=thread");
>  }
>
>  #define LEFT_COLUMN    27
> @@ -1473,6 +1473,7 @@ common_handle_option (struct gcc_options
>        break;
>
>      case OPT_fsanitize_:
> +    case OPT_fsanitize_recover_:
>        {
>         const char *p = arg;
>         while (*p != 0)
> @@ -1539,33 +1540,48 @@ common_handle_option (struct gcc_options
>                   && memcmp (p, spec[i].name, len) == 0)
>                 {
>                   /* Handle both -fsanitize and -fno-sanitize cases.  */
> -                 if (value)
> -                   flag_sanitize |= spec[i].flag;
> +                 if (code == OPT_fsanitize_)
> +                   {
> +                     if (value)
> +                       opts->x_flag_sanitize |= spec[i].flag;
> +                     else
> +                       opts->x_flag_sanitize &= ~spec[i].flag;
> +                   }
>                   else
> -                   flag_sanitize &= ~spec[i].flag;
> +                   {
> +                     if (value)
> +                       opts->x_flag_sanitize_recover |= spec[i].flag;
> +                     else
> +                       opts->x_flag_sanitize_recover &= ~spec[i].flag;
> +                   }
>                   found = true;
>                   break;
>                 }
>
>             if (! found)
>               error_at (loc,
> -                       "unrecognized argument to -fsanitize= option: %q.*s",
> -                       (int) len, p);
> +                       code == OPT_fsanitize_
> +                       ? "unrecognized argument to -fsanitize= option: %q.*s"
> +                       : "unrecognized argument to -fsanitize-recover= "
> +                         "option: %q.*s", (int) len, p);
>
>             if (comma == NULL)
>               break;
>             p = comma + 1;
>           }
>
> +       if (code != OPT_fsanitize_)
> +         break;
> +
>         /* When instrumenting the pointers, we don't want to remove
>            the null pointer checks.  */
> -       if (flag_sanitize & (SANITIZE_NULL | SANITIZE_NONNULL_ATTRIBUTE
> -                            | SANITIZE_RETURNS_NONNULL_ATTRIBUTE))
> +       if (opts->x_flag_sanitize & (SANITIZE_NULL | SANITIZE_NONNULL_ATTRIBUTE
> +                                    | SANITIZE_RETURNS_NONNULL_ATTRIBUTE))
>           opts->x_flag_delete_null_pointer_checks = 0;
>
>         /* Kernel ASan implies normal ASan but does not yet support
>            all features.  */
> -       if (flag_sanitize & SANITIZE_KERNEL_ADDRESS)
> +       if (opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS)
>           {
>             maybe_set_param_value (PARAM_ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD, 0,
>                                    opts->x_param_values,
> @@ -1584,6 +1600,15 @@ common_handle_option (struct gcc_options
>         break;
>        }
>
> +    case OPT_fsanitize_recover:
> +      if (value)
> +       opts->x_flag_sanitize_recover
> +         |= SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT;
> +      else
> +       opts->x_flag_sanitize_recover
> +         &= ~(SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT);
> +      break;
> +
>      case OPT_O:
>      case OPT_Os:
>      case OPT_Ofast:
> --- gcc/asan.c.jj       2014-10-13 17:54:34.000000000 +0200
> +++ gcc/asan.c  2014-10-17 17:46:20.952620580 +0200
> @@ -2884,10 +2884,8 @@ pass_sanopt::execute (function *fun)
>                   no_next = ubsan_expand_objsize_ifn (&gsi);
>                   break;
>                 case IFN_ASAN_CHECK:
> -                 {
> -                   no_next = asan_expand_check_ifn (&gsi, use_calls);
> -                   break;
> -                 }
> +                 no_next = asan_expand_check_ifn (&gsi, use_calls);
> +                 break;
>                 default:
>                   break;
>                 }
> --- gcc/ubsan.c.jj      2014-10-10 19:42:22.000000000 +0200
> +++ gcc/ubsan.c 2014-10-17 17:05:57.609330882 +0200
> @@ -638,7 +638,7 @@ ubsan_expand_bounds_ifn (gimple_stmt_ite
>                              NULL_TREE, NULL_TREE);
>        data = build_fold_addr_expr_loc (loc, data);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_BOUNDS)
>           ? BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS
>           : BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS_ABORT;
>        tree fn = builtin_decl_explicit (bcode);
> @@ -741,7 +741,8 @@ ubsan_expand_null_ifn (gimple_stmt_itera
>    else
>      {
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & ((check_align ? SANITIZE_ALIGNMENT : 0)
> +                                   | (check_null ? SANITIZE_NULL : 0)))
>           ? BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH
>           : BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH_ABORT;
>        tree fn = builtin_decl_implicit (bcode);
> @@ -879,7 +880,7 @@ ubsan_expand_objsize_ifn (gimple_stmt_it
>                                  NULL_TREE);
>           data = build_fold_addr_expr_loc (loc, data);
>           enum built_in_function bcode
> -           = flag_sanitize_recover
> +           = (flag_sanitize_recover & SANITIZE_OBJECT_SIZE)
>               ? BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH
>               : BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH_ABORT;
>           tree p = make_ssa_name (pointer_sized_int_node, NULL);
> @@ -964,22 +965,22 @@ ubsan_build_overflow_builtin (tree_code
>    switch (code)
>      {
>      case PLUS_EXPR:
> -      fn_code = flag_sanitize_recover
> +      fn_code = (flag_sanitize_recover & SANITIZE_SI_OVERFLOW)
>                 ? BUILT_IN_UBSAN_HANDLE_ADD_OVERFLOW
>                 : BUILT_IN_UBSAN_HANDLE_ADD_OVERFLOW_ABORT;
>        break;
>      case MINUS_EXPR:
> -      fn_code = flag_sanitize_recover
> +      fn_code = (flag_sanitize_recover & SANITIZE_SI_OVERFLOW)
>                 ? BUILT_IN_UBSAN_HANDLE_SUB_OVERFLOW
>                 : BUILT_IN_UBSAN_HANDLE_SUB_OVERFLOW_ABORT;
>        break;
>      case MULT_EXPR:
> -      fn_code = flag_sanitize_recover
> +      fn_code = (flag_sanitize_recover & SANITIZE_SI_OVERFLOW)
>                 ? BUILT_IN_UBSAN_HANDLE_MUL_OVERFLOW
>                 : BUILT_IN_UBSAN_HANDLE_MUL_OVERFLOW_ABORT;
>        break;
>      case NEGATE_EXPR:
> -      fn_code = flag_sanitize_recover
> +      fn_code = (flag_sanitize_recover & SANITIZE_SI_OVERFLOW)
>                 ? BUILT_IN_UBSAN_HANDLE_NEGATE_OVERFLOW
>                 : BUILT_IN_UBSAN_HANDLE_NEGATE_OVERFLOW_ABORT;
>        break;
> @@ -1156,7 +1157,8 @@ instrument_bool_enum_load (gimple_stmt_i
>                                      NULL_TREE);
>        data = build_fold_addr_expr_loc (loc, data);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & (TREE_CODE (type) == BOOLEAN_TYPE
> +                                   ? SANITIZE_BOOL : SANITIZE_ENUM))
>           ? BUILT_IN_UBSAN_HANDLE_LOAD_INVALID_VALUE
>           : BUILT_IN_UBSAN_HANDLE_LOAD_INVALID_VALUE_ABORT;
>        tree fn = builtin_decl_explicit (bcode);
> @@ -1278,7 +1280,7 @@ ubsan_instrument_float_cast (location_t
>                                      ubsan_type_descriptor (type), NULL_TREE,
>                                      NULL_TREE);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_FLOAT_CAST)
>           ? BUILT_IN_UBSAN_HANDLE_FLOAT_CAST_OVERFLOW
>           : BUILT_IN_UBSAN_HANDLE_FLOAT_CAST_OVERFLOW_ABORT;
>        fn = builtin_decl_explicit (bcode);
> @@ -1344,7 +1346,7 @@ instrument_nonnull_arg (gimple_stmt_iter
>                                              NULL_TREE);
>               data = build_fold_addr_expr_loc (loc[0], data);
>               enum built_in_function bcode
> -               = flag_sanitize_recover
> +               = (flag_sanitize_recover & SANITIZE_NONNULL_ATTRIBUTE)
>                   ? BUILT_IN_UBSAN_HANDLE_NONNULL_ARG
>                   : BUILT_IN_UBSAN_HANDLE_NONNULL_ARG_ABORT;
>               tree fn = builtin_decl_explicit (bcode);
> @@ -1396,7 +1398,7 @@ instrument_nonnull_return (gimple_stmt_i
>                                          2, loc, NULL_TREE, NULL_TREE);
>           data = build_fold_addr_expr_loc (loc[0], data);
>           enum built_in_function bcode
> -           = flag_sanitize_recover
> +           = (flag_sanitize_recover & SANITIZE_RETURNS_NONNULL_ATTRIBUTE)
>               ? BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN
>               : BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN_ABORT;
>           tree fn = builtin_decl_explicit (bcode);
> --- gcc/c-family/c-ubsan.c.jj   2014-09-10 11:20:49.000000000 +0200
> +++ gcc/c-family/c-ubsan.c      2014-10-17 17:13:14.393241619 +0200
> @@ -104,7 +104,7 @@ ubsan_instrument_division (location_t lo
>                                      NULL_TREE);
>        data = build_fold_addr_expr_loc (loc, data);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_DIVIDE)
>           ? BUILT_IN_UBSAN_HANDLE_DIVREM_OVERFLOW
>           : BUILT_IN_UBSAN_HANDLE_DIVREM_OVERFLOW_ABORT;
>        tt = builtin_decl_explicit (bcode);
> @@ -199,7 +199,7 @@ ubsan_instrument_shift (location_t loc,
>        data = build_fold_addr_expr_loc (loc, data);
>
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_SHIFT)
>           ? BUILT_IN_UBSAN_HANDLE_SHIFT_OUT_OF_BOUNDS
>           : BUILT_IN_UBSAN_HANDLE_SHIFT_OUT_OF_BOUNDS_ABORT;
>        tt = builtin_decl_explicit (bcode);
> @@ -229,7 +229,7 @@ ubsan_instrument_vla (location_t loc, tr
>                                      NULL_TREE);
>        data = build_fold_addr_expr_loc (loc, data);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_VLA)
>           ? BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE
>           : BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE_ABORT;
>        tt = builtin_decl_explicit (bcode);
>
>
>         Jakub

On Mon, Nov 17, 2014 at 06:40:00PM -0800, Alexey Samsonov wrote:
> I've just prepared a patch implementing -fsanitize-recover=<list> in
> Clang (http://reviews.llvm.org/D6302), writing here to make sure we're
> on
> the same page w.r.t. flag semantics:
> 
> * -fsanitize-recover: Enable recovery for all checks enabled by
> -fsanitize= which support it.
> * -fno-sanitize-recover: Disable recovery for all checks.

That is not what I think we've agreed on and what is implemented in GCC.
-fsanitize-recover only enables recovery of the undefined plus
undefined-like sanitizers, in particular it doesn't enable recover from
kernel-address, because -fsanitize-recover should be a deprecated option
and kernel-address never used it before.
So, in GCC -fsanitize-recover stands for
-fsanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
and -fno-sanitize-recover stands for
-fno-sanitize-recover=undefined,float-divide-by-zero,float-cast-overflow

> * -fsanitize-recover=<list>: Enable recovery for all selected checks
> or group of checks. It is forbidden to list unrecoverable sanitizers
> here (e.g., "-fsanitize-recover=address" will produce an error).

We only error on
-fsanitize-recover=address
-fsanitize-recover=thread
-fsanitize-recover=leak
but not say on
-fsanitize-recover=unreachable
which is part of undefined; unreachable isn't recoverable silently.
Likewise -fsanitize-recover=return.  Otherwise one couldn't use
-fsanitize-recover=undefined which is useful.

> * -fno-sanitize-recover=<list>: Disable recovery for selected checks
> or group of checks.

	Jakub

> As for the generated code, I'm at the stage where I can implement the
> following: if a single UBSan hander is used to report multiple error
> kinds (__ubsan_handle_type_mismatch is used for
> -fsanitize=null,alignment,object-size), and these kinds have different
> recoverability, then we emit two handler calls like this:

Nice, I think we do not yet have this in gcc.

-Y

On Mon, Nov 17, 2014 at 10:47 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Mon, Nov 17, 2014 at 06:40:00PM -0800, Alexey Samsonov wrote:
> > I've just prepared a patch implementing -fsanitize-recover=<list> in
> > Clang (http://reviews.llvm.org/D6302), writing here to make sure we're
> > on
> > the same page w.r.t. flag semantics:
> >
> > * -fsanitize-recover: Enable recovery for all checks enabled by
> > -fsanitize= which support it.
> > * -fno-sanitize-recover: Disable recovery for all checks.
>
> That is not what I think we've agreed on and what is implemented in GCC.
> -fsanitize-recover only enables recovery of the undefined plus
> undefined-like sanitizers, in particular it doesn't enable recover from
> kernel-address, because -fsanitize-recover should be a deprecated option
> and kernel-address never used it before.

Hm, indeed, I messed it up. In the older thread we agree that plain
-f(no-)sanitize-recover
should be a deprecated synonym for the current meaning of these flags,
which only specify recoverability for UBSan-related checks :-/

Has GCC already shipped with existing implementation? I'm just curious
if it's convenient to have flags that would enable/disable recovery for all
sanitizers (analogous to -Werror flags which exist in a standalone form, but
may accept specific warnings, so that one can write
  "$(CC) foo.cc -Wno-error -Werror=sign-compare"

> So, in GCC -fsanitize-recover stands for
> -fsanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
> and -fno-sanitize-recover stands for
> -fno-sanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
>
> > * -fsanitize-recover=<list>: Enable recovery for all selected checks
> > or group of checks. It is forbidden to list unrecoverable sanitizers
> > here (e.g., "-fsanitize-recover=address" will produce an error).
>
> We only error on
> -fsanitize-recover=address
> -fsanitize-recover=thread
> -fsanitize-recover=leak

Right, it's a good idea to error on sanitizer kinds we don't have
recovery for. I will
add the errors for TSan/MSan/LSan etc.

> but not say on
> -fsanitize-recover=unreachable
> which is part of undefined; unreachable isn't recoverable silently.
> Likewise -fsanitize-recover=return.  Otherwise one couldn't use
> -fsanitize-recover=undefined which is useful.

Can't this be fixed by checking the actual values of -fsanitize-recover= flag
before expanding the sanitizer groups (i.e. turning "undefined" into
"null,unreachable,return,....")?

We should probably be able to distinguish between -fsanitize-recover=undefined,
and -fsanitize-recover=unreachable,<another checks from undefined>.
In the first case we can enable recovery
for all parts of "undefined" that support it. In the second, we can
produce an error as user
explicitly tried to enable recovery for sanitizer that doesn't support it.

This is only a diagnostic quality issue, though.

>
> > * -fno-sanitize-recover=<list>: Disable recovery for selected checks
> > or group of checks.
>
>         Jakub

On Mon, Nov 17, 2014 at 11:39:47PM -0800, Alexey Samsonov wrote:
> > That is not what I think we've agreed on and what is implemented in GCC.
> > -fsanitize-recover only enables recovery of the undefined plus
> > undefined-like sanitizers, in particular it doesn't enable recover from
> > kernel-address, because -fsanitize-recover should be a deprecated option
> > and kernel-address never used it before.
> 
> Hm, indeed, I messed it up. In the older thread we agree that plain
> -f(no-)sanitize-recover
> should be a deprecated synonym for the current meaning of these flags,
> which only specify recoverability for UBSan-related checks :-/
> 
> Has GCC already shipped with existing implementation? I'm just curious
> if it's convenient to have flags that would enable/disable recovery for all
> sanitizers (analogous to -Werror flags which exist in a standalone form, but
> may accept specific warnings, so that one can write
>   "$(CC) foo.cc -Wno-error -Werror=sign-compare"

Well, no sanitizer is on by default, so you can just use the same
list for -fno-sanitize-recover= or -fsanitize-recover= as for -fsanitize=
if you want.

> > So, in GCC -fsanitize-recover stands for
> > -fsanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
> > and -fno-sanitize-recover stands for
> > -fno-sanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
> >
> > > * -fsanitize-recover=<list>: Enable recovery for all selected checks
> > > or group of checks. It is forbidden to list unrecoverable sanitizers
> > > here (e.g., "-fsanitize-recover=address" will produce an error).
> >
> > We only error on
> > -fsanitize-recover=address
> > -fsanitize-recover=thread
> > -fsanitize-recover=leak
> 
> Right, it's a good idea to error on sanitizer kinds we don't have
> recovery for. I will
> add the errors for TSan/MSan/LSan etc.
> 
> > but not say on
> > -fsanitize-recover=unreachable
> > which is part of undefined; unreachable isn't recoverable silently.
> > Likewise -fsanitize-recover=return.  Otherwise one couldn't use
> > -fsanitize-recover=undefined which is useful.
> 
> Can't this be fixed by checking the actual values of -fsanitize-recover= flag
> before expanding the sanitizer groups (i.e. turning "undefined" into
> "null,unreachable,return,....")?
> 
> We should probably be able to distinguish between -fsanitize-recover=undefined,
> and -fsanitize-recover=unreachable,<another checks from undefined>.
> In the first case we can enable recovery
> for all parts of "undefined" that support it. In the second, we can
> produce an error as user
> explicitly tried to enable recovery for sanitizer that doesn't support it.

But in that case you would need to diagnose it already when parsing the
option, or add code that would remember what bits have been set from other
option sets.
In the former case, you'd diagnose even
-fsanitize-recover=unreachable -fno-sanitize-recover=undefined
even when in that case unreachable in the end is not recoverable.
We don't error out for
-fsanitize-recover=address -fno-sanitize-recover=address
because in the end address is not recovered.

	Jakub

On Mon, Nov 17, 2014 at 11:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Nov 17, 2014 at 11:39:47PM -0800, Alexey Samsonov wrote:
>> > That is not what I think we've agreed on and what is implemented in GCC.
>> > -fsanitize-recover only enables recovery of the undefined plus
>> > undefined-like sanitizers, in particular it doesn't enable recover from
>> > kernel-address, because -fsanitize-recover should be a deprecated option
>> > and kernel-address never used it before.
>>
>> Hm, indeed, I messed it up. In the older thread we agree that plain
>> -f(no-)sanitize-recover
>> should be a deprecated synonym for the current meaning of these flags,
>> which only specify recoverability for UBSan-related checks :-/
>>
>> Has GCC already shipped with existing implementation? I'm just curious
>> if it's convenient to have flags that would enable/disable recovery for all
>> sanitizers (analogous to -Werror flags which exist in a standalone form, but
>> may accept specific warnings, so that one can write
>>   "$(CC) foo.cc -Wno-error -Werror=sign-compare"
>
> Well, no sanitizer is on by default, so you can just use the same
> list for -fno-sanitize-recover= or -fsanitize-recover= as for -fsanitize=
> if you want.

Yeah, but it may look somewhat redundant. Also, re-using the exact
same list may lead to diagnostic messages (if you write
-fsanitize=unreachable,null -fsanitize-recover=unreachable,null).

>> > So, in GCC -fsanitize-recover stands for
>> > -fsanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
>> > and -fno-sanitize-recover stands for
>> > -fno-sanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
>> >
>> > > * -fsanitize-recover=<list>: Enable recovery for all selected checks
>> > > or group of checks. It is forbidden to list unrecoverable sanitizers
>> > > here (e.g., "-fsanitize-recover=address" will produce an error).
>> >
>> > We only error on
>> > -fsanitize-recover=address
>> > -fsanitize-recover=thread
>> > -fsanitize-recover=leak
>>
>> Right, it's a good idea to error on sanitizer kinds we don't have
>> recovery for. I will
>> add the errors for TSan/MSan/LSan etc.
>>
>> > but not say on
>> > -fsanitize-recover=unreachable
>> > which is part of undefined; unreachable isn't recoverable silently.
>> > Likewise -fsanitize-recover=return.  Otherwise one couldn't use
>> > -fsanitize-recover=undefined which is useful.
>>
>> Can't this be fixed by checking the actual values of -fsanitize-recover= flag
>> before expanding the sanitizer groups (i.e. turning "undefined" into
>> "null,unreachable,return,....")?
>>
>> We should probably be able to distinguish between -fsanitize-recover=undefined,
>> and -fsanitize-recover=unreachable,<another checks from undefined>.
>> In the first case we can enable recovery
>> for all parts of "undefined" that support it. In the second, we can
>> produce an error as user
>> explicitly tried to enable recovery for sanitizer that doesn't support it.
>
> But in that case you would need to diagnose it already when parsing the
> option, or add code that would remember what bits have been set from other
> option sets.
> In the former case, you'd diagnose even
> -fsanitize-recover=unreachable -fno-sanitize-recover=undefined
> even when in that case unreachable in the end is not recoverable.
> We don't error out for
> -fsanitize-recover=address -fno-sanitize-recover=address
> because in the end address is not recovered.

OK, that's a good question: should we diagnose -fsanitize-recover=address
if it was later disabled by -fno-sanitize-recover=address? There is an argument
for doing this: "-fsanitize-recover=address" is unsupported, so this
flag shouldn't
have been provided in the first place. It makes much more sense to
delete it rather
than override it. It couldn't be passed down from some global
project-wide configuration
(like CFLAGS), because it wouldn't work anywhere.

The only case where overriding might come in handy is re-using the same list for
-fsanitize= and -fsanitize-recover= flags that you mention:
# SANITIZERS is a list of sanitizers we want to enable.
CFLAGS = "${CFLAGS} -fsanitize=${SANITIZERS} -fsanitize-recover=${SANITIZERS}"
# Oops - we produce an error if ${SANITIZERS} contains "address".

However, even if we decide to *not* diagnose unrecoverable sanitizer
kinds disabled later
in the command line, we can still implement warnings for "unreachable" properly.
You can scan "-fsanitize-recover" flags from right to left and keep
track of all sanitizers that
"would be disabled". When you see "-fsanitize=unreachable" (with
literal "unreachable" as a value),
you diagnose the error only if "unreachable" wouldn't be disabled
later by some -fno-sanitize-recover flag.

FWIW, we implement this logic in Clang for regular -fsanitize= flags.

Hi Jakub,

On Tue, Nov 18, 2014 at 11:36 AM, Alexey Samsonov <samsonov@google.com> wrote:
>
> On Mon, Nov 17, 2014 at 11:53 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Mon, Nov 17, 2014 at 11:39:47PM -0800, Alexey Samsonov wrote:
> >> > That is not what I think we've agreed on and what is implemented in GCC.
> >> > -fsanitize-recover only enables recovery of the undefined plus
> >> > undefined-like sanitizers, in particular it doesn't enable recover from
> >> > kernel-address, because -fsanitize-recover should be a deprecated option
> >> > and kernel-address never used it before.
> >>
> >> Hm, indeed, I messed it up. In the older thread we agree that plain
> >> -f(no-)sanitize-recover
> >> should be a deprecated synonym for the current meaning of these flags,
> >> which only specify recoverability for UBSan-related checks :-/
> >>
> >> Has GCC already shipped with existing implementation? I'm just curious
> >> if it's convenient to have flags that would enable/disable recovery for all
> >> sanitizers (analogous to -Werror flags which exist in a standalone form, but
> >> may accept specific warnings, so that one can write
> >>   "$(CC) foo.cc -Wno-error -Werror=sign-compare"
> >
> > Well, no sanitizer is on by default, so you can just use the same
> > list for -fno-sanitize-recover= or -fsanitize-recover= as for -fsanitize=
> > if you want.
>
> Yeah, but it may look somewhat redundant. Also, re-using the exact
> same list may lead to diagnostic messages (if you write
> -fsanitize=unreachable,null -fsanitize-recover=unreachable,null).

Reviving this thread.

What do you think of the following idea:
1) we keep "-fsanitize-recover" and "-fno-sanitize-recover" as
deprecated synonyms
for "-f(no-)?sanitize=<ubsan-like checks>"
2) we introduce -fsanitize-recover=<list> and
-fno-sanitize-recover=<list>, where list may contain
specific sanitizers ("address") or group of sanitizers ("undefined").
3) we introduce group of sanitizers called "all", which is, well, "all
available sanitizers". It can *not* be
used in -fsanitize=all flag, but can be used to easily disable all
previously enabled sanitizers ("-fno-sanitize=all"),
or to enable/disable recovery for all enabled sanitizers
("-fsanitize-recover=all" / "-fno-sanitize-recover=all").

This would be a handy shortcut, and will save users from copying the
same list twice for
-fsanitize= and -fsanitize-recover= flags, and from potential
diagnostic problems I mention.

>
>
> >> > So, in GCC -fsanitize-recover stands for
> >> > -fsanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
> >> > and -fno-sanitize-recover stands for
> >> > -fno-sanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
> >> >
> >> > > * -fsanitize-recover=<list>: Enable recovery for all selected checks
> >> > > or group of checks. It is forbidden to list unrecoverable sanitizers
> >> > > here (e.g., "-fsanitize-recover=address" will produce an error).
> >> >
> >> > We only error on
> >> > -fsanitize-recover=address
> >> > -fsanitize-recover=thread
> >> > -fsanitize-recover=leak
> >>
> >> Right, it's a good idea to error on sanitizer kinds we don't have
> >> recovery for. I will
> >> add the errors for TSan/MSan/LSan etc.
> >>
> >> > but not say on
> >> > -fsanitize-recover=unreachable
> >> > which is part of undefined; unreachable isn't recoverable silently.
> >> > Likewise -fsanitize-recover=return.  Otherwise one couldn't use
> >> > -fsanitize-recover=undefined which is useful.
> >>
> >> Can't this be fixed by checking the actual values of -fsanitize-recover= flag
> >> before expanding the sanitizer groups (i.e. turning "undefined" into
> >> "null,unreachable,return,....")?
> >>
> >> We should probably be able to distinguish between -fsanitize-recover=undefined,
> >> and -fsanitize-recover=unreachable,<another checks from undefined>.
> >> In the first case we can enable recovery
> >> for all parts of "undefined" that support it. In the second, we can
> >> produce an error as user
> >> explicitly tried to enable recovery for sanitizer that doesn't support it.
> >
> > But in that case you would need to diagnose it already when parsing the
> > option, or add code that would remember what bits have been set from other
> > option sets.
> > In the former case, you'd diagnose even
> > -fsanitize-recover=unreachable -fno-sanitize-recover=undefined
> > even when in that case unreachable in the end is not recoverable.
> > We don't error out for
> > -fsanitize-recover=address -fno-sanitize-recover=address
> > because in the end address is not recovered.
>
> OK, that's a good question: should we diagnose -fsanitize-recover=address
> if it was later disabled by -fno-sanitize-recover=address? There is an argument
> for doing this: "-fsanitize-recover=address" is unsupported, so this
> flag shouldn't
> have been provided in the first place. It makes much more sense to
> delete it rather
> than override it. It couldn't be passed down from some global
> project-wide configuration
> (like CFLAGS), because it wouldn't work anywhere.
>
> The only case where overriding might come in handy is re-using the same list for
> -fsanitize= and -fsanitize-recover= flags that you mention:
> # SANITIZERS is a list of sanitizers we want to enable.
> CFLAGS = "${CFLAGS} -fsanitize=${SANITIZERS} -fsanitize-recover=${SANITIZERS}"
> # Oops - we produce an error if ${SANITIZERS} contains "address".
>
> However, even if we decide to *not* diagnose unrecoverable sanitizer
> kinds disabled later
> in the command line, we can still implement warnings for "unreachable" properly.
> You can scan "-fsanitize-recover" flags from right to left and keep
> track of all sanitizers that
> "would be disabled". When you see "-fsanitize=unreachable" (with
> literal "unreachable" as a value),
> you diagnose the error only if "unreachable" wouldn't be disabled
> later by some -fno-sanitize-recover flag.
>
> FWIW, we implement this logic in Clang for regular -fsanitize= flags.
> --
> Alexey Samsonov, Mountain View, CA

On Thu, Dec 18, 2014 at 05:34:12PM -0800, Alexey Samsonov wrote:
> Reviving this thread.
> 
> What do you think of the following idea:
> 1) we keep "-fsanitize-recover" and "-fno-sanitize-recover" as
> deprecated synonyms
> for "-f(no-)?sanitize=<ubsan-like checks>"
> 2) we introduce -fsanitize-recover=<list> and
> -fno-sanitize-recover=<list>, where list may contain
> specific sanitizers ("address") or group of sanitizers ("undefined").
> 3) we introduce group of sanitizers called "all", which is, well, "all
> available sanitizers". It can *not* be
> used in -fsanitize=all flag, but can be used to easily disable all
> previously enabled sanitizers ("-fno-sanitize=all"),
> or to enable/disable recovery for all enabled sanitizers
> ("-fsanitize-recover=all" / "-fno-sanitize-recover=all").

So in esence a new group (like undefined) that is forbidden for -fsanitize=
option, but allowed for all the others?  Sounds fine to me.
Can one still mix it with others (of course, it doesn't make much sense,
but would mean fewer exceptions in the option parser and thus more easily
understandable), like
-fno-sanitize=enum,all,bool
or do you prefer to only allow it as the sole argument of those options?

	Jakub