From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30206 invoked by alias); 8 Aug 2017 13:08:48 -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 30189 invoked by uid 89); 8 Aug 2017 13:08:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=reaction, mere X-HELO: mail-qk0-f176.google.com Received: from mail-qk0-f176.google.com (HELO mail-qk0-f176.google.com) (209.85.220.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 08 Aug 2017 13:08:41 +0000 Received: by mail-qk0-f176.google.com with SMTP id d145so19120267qkc.2 for ; Tue, 08 Aug 2017 06:08:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=uMFDuDjKQiWsIEKY9wtCY5gnenzSSWpvJdZIpvt/9Kc=; b=anPu8oCrGt4H5uDiGMe+UZ54x1Jv3z19XMnKOSqkyfdomehhkjYRg26v8aOXfN1Ajz HCR1nqPz8IGrnMf4dwZZX0eeuKsXaENajQd+YKopijt7qzshIkRX0PNLVtzAWQvYEnd2 XeEyLwBsCZ2MjoMad+BqchyIF/OGm09ixEgocqzfNdNAn0DMxllkrwURROsLpXOdTl3k TkMEDrjY0JSpU6TzI/MSehNyThiRetaob2YpqHpwwr4Sfy/ghjhrC8+DpfaJwLFQ4gE+ LtJ+MBUl0dD91YyLCioQB2HIPvMTlf3WLk81Ddl/Arb6E4mVUNgL8neKQHOeIYJxZoFB 8s+g== X-Gm-Message-State: AHYfb5iHpYO+VxiUfoHqpT8NtmdIXT/9aLLXD8BB64NUop0GvlGE7HBP l6mqHh2focUcJasc9qcjABmAbMiktQ== X-Received: by 10.55.40.104 with SMTP id o101mr5224239qkh.311.1502197719592; Tue, 08 Aug 2017 06:08:39 -0700 (PDT) MIME-Version: 1.0 Received: by 10.140.44.74 with HTTP; Tue, 8 Aug 2017 06:08:39 -0700 (PDT) In-Reply-To: <703424cb-db8c-8aae-9bd2-cece5883a5de@gmail.com> References: <8a0de82f-d68e-9c37-0406-f216e3f92884@gmail.com> <703424cb-db8c-8aae-9bd2-cece5883a5de@gmail.com> From: Richard Biener Date: Tue, 08 Aug 2017 13:08:00 -0000 Message-ID: Subject: Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918) To: Martin Sebor Cc: Jeff Law , Gcc Patch List Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2017-08/txt/msg00595.txt.bz2 On Mon, Aug 7, 2017 at 1:08 AM, Martin Sebor wrote: > On 08/03/2017 02:45 AM, Richard Biener wrote: >> >> On Wed, Aug 2, 2017 at 7:10 PM, Jeff Law wrote: >>> >>> On 08/01/2017 03:25 AM, Richard Biener wrote: >>>> >>>> On Tue, Aug 1, 2017 at 11:23 AM, Richard Biener >>>> wrote: >>>>> >>>>> On Tue, Aug 1, 2017 at 4:27 AM, Martin Sebor wrote: >>>>>> >>>>>> Richard, >>>>>> >>>>>> in discussing this work Jeff mentioned that your comments on >>>>>> the tree-ssa-alias.c parts would be helpful. When you have >>>>>> a chance could you please give it a once over and let me know >>>>>> if you have any suggestions or concerns? There are no visible >>>>>> changes to existing clients of the pass, just extensions that >>>>>> are relied on only by the new diagnostics. >>>>>> >>>>>> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html >>>>>> >>>>>> I expect to revisit the sprintf %s patch mentioned below and >>>>>> see how to simplify it by taking advantage of the changes >>>>>> implemented here. >>>>> >>>>> >>>>> Your ptr_deref_alias_decl_p returns true when must_alias is true >>>>> but there is no must-alias relationship. >>>>> >>>>> The ao_ref_init_from_ptr_and_size doesn't belong there. >>>>> >>>>> I dislike all of the changes related to returning an alias "size". >>>>> >>>>> Please keep away from the alias machinery. >>>>> >>>>> Warning during folding is similarly bad design. Please don't add >>>>> more such things. >>>>> >>>>> Thanks for putting this on my radar though. >>>>> Richard. >>>> >>>> >>>> Oh, for a constructive comment this all feels like sth for either >>>> sanitizers or a proper static analysis tool. As I outlined repeatedly >>>> I belive GCC could host a static analysis tool but it surely should >>>> not be interwinded into it's optimization passes or tools. >>> >>> I disagree strongly here. >>> >>> Sanitiers catch problems after the fact and only if you've compiled your >>> code with sanitization and manage to find a way to trigger the problem >>> path. >>> >>> Other static analysis tools are useful, but they aren't an integral part >>> of the edit, compile, debug cycle and due to their cost are often run >>> long after code was committed. >>> >>> Finding useful warnings that can be issued as part of the compile, edit, >>> debug cycle is, IMHO, far more useful than sanitizers or independent >>> static checkers. >>> >>> -- >>> >>> >>> I think finding a way to exploit information that our various analyzers >>> can provide to give precise warnings is a good thing. So it seemed >>> natural that if we wanted to ask a must-alias question that we should be >>> querying the alias oracle rather than implementing that kind of query >>> within the sprintf warnings. I'm not sure why you'd say "Please keep >>> away from the alias machinery". >>> >>> I'm a little confused here... >> >> >> Well, simply because the way as implemented isn't a must-alias query >> but maybe one that's good enough for warnings (reduces false positives >> but surely doesn't eliminate them). > > > I'm very interested in reducing the rate of false positives in > these and all other warnings. As I mentioned in my comments, > I did my best to weed them out of the implementation by building > GDB, Glibc, Busybox, and the Linux kernel. That of course isn't > a guarantee that there aren't any. But the first implementation > of any non-trivial feature is never perfect, and hardly any > warning of sufficient complexity is free of false positives, no > matter here it's implemented (the middle-end, front-end, or > a standalone static analysis tool). If you spotted some cases > I had missed I'd certainly be grateful for examples. Otherwise, > they will undoubtedly be reported as more software is exposed > to the warning and, if possible, fixed, as happens with all > other warnings. > >> There's a must alias query already, in stmt_kills_ref_p. It's a matter >> of refactoring to make a refs_must_alias_p. >> >> Then propose that "overlap range" stuff separately. > > > I appreciate constructive suggestions for improvements and I will > look into the stmt_kills_ref_p suggestion. But since my work > elicited such an strong response from you I feel I should explain: > I tried to make my changes as unintrusive as possible. The alias > oracle is a new area for me and I didn't want to make mistakes in > the process of making overly extensive modifications to it. > >> But I'm against lumping this all in as an innocent change suggesting >> the machinery can do sth (must alias) when it really can't. I also >> do not like adding a "must alias" bool to the may-alias APIs as >> the implementation is fundamentally may-alias, must-alias really >> is very different. > > > I certainly want to do the right thing and implement the warning > in a way that makes the most sense. As I said, I'll look into > the refactoring, but since my testing shows the current code to > work well as is, it would be helpful if you could provide more > details about what it is that concerns you with it and what cases > of false positives you are worried about. (Examples of code that > demonstrate the false positives would be especially valuable.) I am not worried about false positive warnings. I am worried about an API used by optimization passes that advertises an ability (must-alias query, whoo!) that may end up producing wrong-code. I believe must-alias is so fundamentally different from may-alias that re-using the same API (with a mere change in one argument) is wrong. We _do_ have a must-alias API as I said. _Any_ change to the alias API needs to give conservatively correct answers. > That being said, after much thought, I do have to let you know > that I take offense at both your tone and your insinuation that > I tried to sneak in some subversive changes. I did what I thought > was right. If it can be done better I'm glad to hear the details > of what's wrong with it and in what ways the approach you prefer > is better. But I would be grateful for a more respectful reply > in the future. Sorry, but the Subject of the mails with the patches say "enhance -Wrestrict" which causes me to look away quickly. But most of the patch was actually changes to the alias API and implementation. You should have sumbitted those separately or more prominetly advertise them. I took an offense on this tactic, if that wasn't your intention then take this reaction as a reason to better separate changes to separate parts of the compiler. Thanks, Richard. > Martin