From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5771 invoked by alias); 19 Dec 2014 01:34:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 5760 invoked by uid 89); 19 Dec 2014 01:34:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail-ob0-f181.google.com Received: from mail-ob0-f181.google.com (HELO mail-ob0-f181.google.com) (209.85.214.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 19 Dec 2014 01:34:15 +0000 Received: by mail-ob0-f181.google.com with SMTP id gq1so9146909obb.12 for ; Thu, 18 Dec 2014 17:34:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=lCm0+c4atjzFazYfn++B5lv03JftqTSRBWicG62PamA=; b=TjlUAc/SObK6Ar624tL/QqkqSqE9DOfv0wzh7JxhtzH0+n1bl4IjAewhGwX3v4tzDZ 3o0Vc0pMg76HDkzMq8zwbfmTPNsircGI2lVOC8p8KgCw+FyqrMmYZVKY58NceF4fEL26 M2sFuh21w/Dn+7JIaSwc23qE3ZbTjy71QmQGSZ1baacvalY2taMNUz8Wj5cfKiMTGzOO CWXGt69ZNZzz4/AUhQskQXSvkosv+TtR2T0xFtfKlHXOZTxvESMWrIN+heh/LJYhcKfw TAp5ocLVFqy1uMIyQZkgasw0mlf4VfUPLodOlUt136GybO+m5RrgP7yM++NvLFlejZvj mcsg== X-Gm-Message-State: ALoCoQldwabw/tgxv0vqIRd7IzDvCfTwPyrAp8v1KtW4WDujFEqg5EHiMyCztT8bbaAah7sIlYBt MIME-Version: 1.0 X-Received: by 10.60.45.167 with SMTP id o7mr3309457oem.70.1418952852999; Thu, 18 Dec 2014 17:34:12 -0800 (PST) Received: by 10.182.232.225 with HTTP; Thu, 18 Dec 2014 17:34:12 -0800 (PST) In-Reply-To: References: <542A56C0.2030506@samsung.com> <20140930173340.GI1986@tucnak.redhat.com> <20140930173913.GJ1986@tucnak.redhat.com> <543BADAB.4090000@samsung.com> <20141017161334.GF10376@tucnak.redhat.com> <20141118064740.GE1745@tucnak.redhat.com> <20141118075317.GF1745@tucnak.redhat.com> Date: Fri, 19 Dec 2014 02:47:00 -0000 Message-ID: Subject: Re: [PATCH] -fsanitize-recover=list From: Alexey Samsonov To: Jakub Jelinek Cc: Yury Gribov , Marek Polacek , GCC Patches , Dmitry Vyukov , Konstantin Serebryany Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2014-12/txt/msg01605.txt.bz2 Hi Jakub, On Tue, Nov 18, 2014 at 11:36 AM, Alexey Samsonov wrote: > > On Mon, Nov 17, 2014 at 11:53 PM, Jakub Jelinek 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=" 2) we introduce -fsanitize-recover= and -fno-sanitize-recover=, 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=: 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,. > >> 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