From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 35269 invoked by alias); 26 Apr 2017 07:59:16 -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 35125 invoked by uid 89); 26 Apr 2017 07:59:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=warned, literally X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 26 Apr 2017 07:59:09 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 92C90AB09; Wed, 26 Apr 2017 07:59:09 +0000 (UTC) Date: Wed, 26 Apr 2017 09:13:00 -0000 From: Richard Biener To: Martin Sebor cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH][RFC] Enable -fstrict-overflow by default In-Reply-To: <02095fbe-b1db-4b68-cd35-17cc8cde8ae6@gmail.com> Message-ID: References: <02095fbe-b1db-4b68-cd35-17cc8cde8ae6@gmail.com> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2017-04/txt/msg01264.txt.bz2 On Tue, 25 Apr 2017, Martin Sebor wrote: > On 04/24/2017 05:25 AM, Richard Biener wrote: > > > > The following makes signed overflow undefined for all (non-)optimization > > levels. The intent is to remove -fno-strict-overflow signed overflow > > behavior as that is not a sensible option to the user (it ends up > > with the worst of both -fwrapv and -fno-wrapv). The implementation > > details need to be preserved for the forseeable future to not wreck > > UBSAN with either associating (-fwrapv behavior) or optimizing > > (-fno-wrapv behavior). > > > > The other choice would be to make -fwrapv the default for -O[01]. > > > > A second patch in this series would unify -f[no-]wrapv, -f[no-]trapv > > and -f[no-]strict-overflow with a > > -fsigned-integer-overflow={undefined,wrapping,trapping[,sanitized]} > > option, making conflicts amongst the options explicit (and reduce > > the number of flag_ variables). 'sanitized' would essentially map > > to todays flag_strict_overflow = 0. There's another sole user > > of flag_strict_overflow, POINTER_TYPE_OVERFLOW_UNDEFINED - not sure > > what to do about that, apart from exposing it as different flag > > alltogether. > > > > Further patches in the series would remove -Wstrict-overflow (and > > cleanup VRP for example). > > Minimizing the differences between the guarantees provided at > different optimization levels is a good thing. It will help > uncover bugs that would go undetected during development (with > -O0) and only manifest when building with optimization (which > may be less frequent). > > I find the -Wstrict-overflow warning with all its levels over- > engineered but I'm not sure I'm in favor of completely eliminating > it. It has helped illuminate the signed integer overflow problem > for many users who were otherwise completely unaware of it. I'd > be concerned that by getting rid of it users might be lulled back > into assuming that it has the same wrapping semantics as common > hardware (or simply doesn't happen). It sounds like you'd like > to get rid of it to simplify GCC code. Would it make sense to > preserve it for at least the most egregious instances of overflow > (like in 'if (i + 1 < i)' and similar)? Such cases can (and should!) be certainly warned for but in the frontend. The current implementation which warns at the point of simplification isn't too useful given it warns even when the above doesn't appear literally but through complex optimization. The most complaint about the warning is that it warns about perfectly valid code -- there's nothing invalid in a program doing if (i + 1 < i) and the compiler optimizing this is good. The warning was supposed to be a hint that maybe the programmer wasn't aware of undefined signed integer overflow and thus the code didn't do what he expected. We do have -fsanitize=undefined now to better catch all these cases. Btw, the above should warn under one of the various -W... that warn about expressions always evaluating to true/false (didn't spot one that's right on, so maybe we need to add a new one - or re-use -Wstrict-overflow). Richard. > Martin > > > > > Anyway, most controversical part(?) below. > > > > Any comments on this particular patch (and the overall proposal)? > > > > Cleaning up the options is probably a no-brainer anyways. > > > > Thanks, > > Richard. > > > > 2017-04-24 Richard Biener > > > > * common.opt (fstrict-overflow): Enable by default. > > * opts.c (default_options_table): Remove OPT_fstrict_overflow entry. > > > > Index: gcc/common.opt > > =================================================================== > > --- gcc/common.opt (revision 247091) > > +++ gcc/common.opt (working copy) > > @@ -2342,7 +2342,7 @@ Common Report Var(flag_strict_aliasing) > > Assume strict aliasing rules apply. > > > > fstrict-overflow > > -Common Report Var(flag_strict_overflow) Optimization > > +Common Report Var(flag_strict_overflow) Init(1) Optimization > > Treat signed overflow as undefined. > > > > fsync-libcalls > > Index: gcc/opts.c > > =================================================================== > > --- gcc/opts.c (revision 247091) > > +++ gcc/opts.c (working copy) > > @@ -496,7 +496,6 @@ static const struct default_options defa > > { OPT_LEVELS_2_PLUS, OPT_fschedule_insns2, NULL, 1 }, > > #endif > > { OPT_LEVELS_2_PLUS, OPT_fstrict_aliasing, NULL, 1 }, > > - { OPT_LEVELS_2_PLUS, OPT_fstrict_overflow, NULL, 1 }, > > { OPT_LEVELS_2_PLUS_SPEED_ONLY, OPT_freorder_blocks_algorithm_, NULL, > > REORDER_BLOCKS_ALGORITHM_STC }, > > { OPT_LEVELS_2_PLUS, OPT_freorder_functions, NULL, 1 }, > > > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)