Hi Jakub,
On Tue, Nov 18, 2014 at 11:36 AM, Alexey Samsonov <samso...@google.com> wrote:
>
> On Mon, Nov 17, 2014 at 11:53 PM, Jakub Jelinek <ja...@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
--
Alexey Samsonov, Mountain View, CA
