From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id DE7A93853805 for ; Thu, 27 May 2021 21:55:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DE7A93853805 Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-236-PkS7QMXmN2ChbQQBOZNXoA-1; Thu, 27 May 2021 17:55:16 -0400 X-MC-Unique: PkS7QMXmN2ChbQQBOZNXoA-1 Received: by mail-qk1-f200.google.com with SMTP id o14-20020a05620a130eb02902ea53a6ef80so1513776qkj.6 for ; Thu, 27 May 2021 14:55:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=MqpJO/ElRG14fXtI8FsI80P7sShlU4D/bmwRANaO8K8=; b=Pg5DhU6bTaUUZcOIeKFgxNCUoWd0BBVTuEwas4c5qxGIWpvcDRgHOZ4mh6Wm5kwokn 6GNoBNahEyYIaWzpkMWwq0vleNMHt7sgCgtp/p5dYAFDXJIFYg569k4DOpA/oNEIPWfp I6yJXNfEky7+tjZKw51NC7aV2WJvwQnobGZJDa4tTnbHN2uixUpThCSI7RnRIHHJ/ieV N3VaGTAL9aIXnFGyNIrNmsolHXodahKjl+501Ws7Oxby4bCRNCA7/gMYLjKW9Ws96eek CKwKMnmBc2SL77ZLRuueS/h5K9W4K6TFcw2zPHoQS/7kAIsKyM4QKf1pozMdnC5k27Ao R9Hw== X-Gm-Message-State: AOAM5312xw4vb6wEGrtjTu2ESWssPi9G+2hsHwWcXoEDscWP2xVj0P2X FmEwLSvbvtsS34GpBwsdL+VEo6N7ZqLZKiOID36uXDwSMEpEwc/3stGdGrtFoL0g9E6M7d4DYCy qxYkWiiMNcf+ITqPZjA== X-Received: by 2002:a37:6c03:: with SMTP id h3mr687091qkc.474.1622152516348; Thu, 27 May 2021 14:55:16 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwGvwgaXBvRfXwgWPXJ31X1K7ZLSFn4kCYzfBGWml5Q5W3VTHz8ixqs3+ygPKcdN0LvY00Ddw== X-Received: by 2002:a37:6c03:: with SMTP id h3mr687068qkc.474.1622152516038; Thu, 27 May 2021 14:55:16 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id v10sm2170477qkg.42.2021.05.27.14.55.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 May 2021 14:55:15 -0700 (PDT) Message-ID: <8e75c93bf9a35892612e220242aec84ae12875a3.camel@redhat.com> Subject: Re: [PATCH 0/11] warning control by group and location (PR 74765) From: David Malcolm To: Martin Sebor , Martin Sebor via Gcc-patches , richard.sandiford@arm.com Date: Thu, 27 May 2021 17:55:14 -0400 In-Reply-To: <0e8cc22c-190a-618d-28b4-538bdc432827@gmail.com> References: <92db3776-af59-fa20-483b-aa67b17d0751@gmail.com> <47d06c821a53f5bd2246f0fca2c9a693bff6b882.camel@redhat.com> <0e8cc22c-190a-618d-28b4-538bdc432827@gmail.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 May 2021 21:55:20 -0000 On Thu, 2021-05-27 at 10:41 -0600, Martin Sebor wrote: > On 5/27/21 5:19 AM, Richard Sandiford wrote: > > Thanks for doing this. > > > > Martin Sebor via Gcc-patches writes: > > > […] > > > On 5/24/21 5:08 PM, David Malcolm wrote: > > > > On Mon, 2021-05-24 at 16:02 -0600, Martin Sebor wrote: > > > > > Subsequent patches then replace invocations of the > > > > > TREE_NO_WARNING() > > > > > macro and the gimple_no_warning_p() and > > > > > gimple_set_no_warning() > > > > > functions throughout GCC with those and remove the legacy > > > > > APIs to > > > > > keep them from being accidentally reintroduced along with the > > > > > problem. > > > > > These are mostly mechanical changes, except that most of the > > > > > new > > > > > invocations also specify the option whose disposition to > > > > > query for > > > > > the expression or location, or which to enable or disable[2]. > > > > > The last function, copy_no_warning(), copies the disposition > > > > > from > > > > > one expression or location to another. > > > > > > > > > > A couple of design choices might be helpful to explain: > > > > > > > > > > First, introducing "warning groups" rather than controlling > > > > > each > > > > > individual warning is motivated by a) the fact that the > > > > > latter > > > > > would make avoiding redundant warnings for related problems > > > > > cumbersome (e.g., after issuing a -Warray-bounds we want to > > > > > suppress -Wstringop-overflow as well -Wstringop-overread for > > > > > the same access and vice versa), and b) simplicity and > > > > > efficiency > > > > > of the implementation (mapping each option would require a > > > > > more > > > > > complex data structure like a bitmap). > > > > > > > > > > Second, using location_t to associate expressions/statements > > > > > with > > > > > the warning groups also turns out to be more useful in > > > > > practice > > > > > than a direct association between a tree or gimple*, and also > > > > > simplifies managing the data structure.  Trees and gimple* > > > > > come > > > > > and go across passes, and maintaining a mapping for them that > > > > > accounts for the possibility of them being garbage-collected > > > > > and the same addresses reused is less than straightforward. > > > > > > > > I find some of the terminology rather awkard due to it the > > > > negation > > > > we're already using with TREE_NO_WARNING, in that we're turning > > > > on a > > > > no_warning flag, and that this is a per-location/expr/stmt > > > > thing that's > > > > different from the idea of enabling/disabling a specific > > > > warning > > > > altogether (and the pragmas that control that).   Sometimes the > > > > patches > > > > refer to enabling/disabling warnings and I think I want > > > > "enabling" and > > > > "disabling" to mean the thing the user does with -Wfoo and - > > > > Wno-foo. > > > > > > > > Would calling it a "warning suppression" or somesuch be more or > > > > less > > > > klunky? > > > > > > I like warning suppression :)  But I'm not sure where you suggest > > > I use the phrase. > > > > > > I don't particularly care for the "no" in the API names either > > > (existing or new) and would prefer a positive form.  I considered > > > enable_warning() and warning_enabled() but I chose the names I > > > did > > > because they're close to their established gimple namesakes.  I'm > > > fine changing them to the alternatives, or if you or someone else > > > has a preference for different names I'm open to suggestions.  > > > Let > > > me know. > > > > Not my area, but FWIW, +1 for David's suggestion of > > s/set_no_warning/suppress_warning/ or similar.  As well as the > > problem with the double negative in negated conditions, I always > > have to > > remember whether TREE_NO_WARNING means that hasn't been anything to > > warn > > about yet or whether we shouldn't warn in future. > > Sure.  Jason has a patch out for review to introduce > >    warning_enabled_at (location, int option). CCing Jason. BTW, do you have a URL for that patch? > > With that, how does the following API look? (I'd replace the int > argument above with opt_code myself): > >    void enable_warning (location_t, enum opt_code = N_OPTS); >    void disable_warning (location_t, enum opt_code = N_OPTS); >    void copy_warning (location_t, location_t); >    bool warning_enabled_at (location_t, enum opt_code = N_OPTS). > >    void enable_warning (tree, enum opt_code = N_OPTS); >    void disable_warning (tree, enum opt_code = N_OPTS); >    void copy_warning (tree, const_tree); >    bool warning_enabled_at (const_tree, enum opt_code = N_OPTS). > >    void enable_warning (gimple *, enum opt_code = N_OPTS); >    void disable_warning (gimple *, enum opt_code = N_OPTS); >    void copy_warning (gimple *, const gimple *); >    bool warning_enabled_at (gimple *, enum opt_code = N_OPTS). I'm not so happy with "enable_warning" and "disable_warning" in the above proposal; it seems to me that three things are being conflated: (a) whether the user enabled the warning with -Wfoo, whether at the command line, or by #pragma (and using the location_t to query the pragma information) versus (b) whether we're suppressing a category of warnings for this construct (and using the location_t to stash the flags) versus (c) should the warning actually be emitted at the location Presumably Jason's patch is a way for our warning code (in the FE and middle end) to avoid doing expensive things by querying whether the warning would actually get emitted i.e. it's a way to query (c), so we can have an early-reject if it returns false, where presumably the query is cheap relative to the testing that would otherwise occur. Is that right? Perhaps the terminology should be that: - the user *requests* a warning, (a) above - our code can *suppress* a warning, for (b) above - the combination of the two give (c): whether the warning is *enabled* Or maybe (a) the user *enables* a warning (b) our code can *suppress* a warning (or warning category) (c) the combination gives whether the warning is *active*. or somesuch. Thoughts? Dave