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 6C74E3858D28 for ; Mon, 11 Oct 2021 06:54:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6C74E3858D28 Received: from mail-lf1-f72.google.com (mail-lf1-f72.google.com [209.85.167.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-242-oLw_bzrtNpGqm_4xeWrXbA-1; Mon, 11 Oct 2021 02:54:22 -0400 X-MC-Unique: oLw_bzrtNpGqm_4xeWrXbA-1 Received: by mail-lf1-f72.google.com with SMTP id k8-20020a0565123d8800b003fd6e160c77so7067932lfv.17 for ; Sun, 10 Oct 2021 23:54:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=93zFDDiWbk0g51K2e0IfzuHfavJVqKH3FR0OKe/L57Y=; b=slFIIKzI3CvqL/xMgsD1uC/Bl7aq0J4uS1jvTABr8W++ymiXQES+Eld+Mt5ofOM/mR liybvq2wxd0FldbzIhf39TLcQrWNr6wRqTGwEsq3Ee84Uo03qxZqIenfaOjQ6//2Vcip 9/foiCjZtxngEftDIpTR0FYcohbwUvZV4AQuX7YI+xKVwYJFTrtTF1hjgnrZxVjwx0w8 dliDzcVrW5kQlRvXPuBCUcjLmmfpsokj86WXPlmU9WDscwTY90Mfpan7DZNbTdhmJb+b eeQepC9qMgq9wXt0jnDmzfIomUcS3DSEY5Z2kUX3z7OEJwjgiFH6lUvhi3aVrM1FTzlu GCsQ== X-Gm-Message-State: AOAM531omblF2N4iqphsrUjAEIBfDMXJwYNkza43XvMiSbRzTy40JSRI eLgONnWopNil9PVjnW9qjmzGwha88hXc9O57q/Ddk2aNfqAvz65Zqm9eR0udBjOGHyaoR6xv7Rl 5l4v5OfH/bfEW7/u8tSTxrMaF7lOIxw5hxw== X-Received: by 2002:a19:5f0d:: with SMTP id t13mr25679994lfb.592.1633935260865; Sun, 10 Oct 2021 23:54:20 -0700 (PDT) X-Google-Smtp-Source: ABdhPJy70m74SYfob+CuVbl6j4qx03PNUOwiAb6EJCLD4j2zyawK/ViFoecBBkBNsm6WJu0x/NrZLnsWN6NfaqHUpYo= X-Received: by 2002:a19:5f0d:: with SMTP id t13mr25679977lfb.592.1633935260599; Sun, 10 Oct 2021 23:54:20 -0700 (PDT) MIME-Version: 1.0 References: <20211008151222.37790-1-aldyh@redhat.com> <217c8f4c-934f-f20f-b789-f19aa8a303f5@gmail.com> <0118b7f7-921d-11f0-8294-d78a75e07445@gmail.com> In-Reply-To: <0118b7f7-921d-11f0-8294-d78a75e07445@gmail.com> From: Aldy Hernandez Date: Mon, 11 Oct 2021 08:54:09 +0200 Message-ID: Subject: Re: [PATCH] Convert strlen pass from evrp to ranger. To: Martin Sebor Cc: Jakub Jelinek , Martin Sebor , GCC patches X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-6.1 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_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Mon, 11 Oct 2021 06:54:28 -0000 On Sat, Oct 9, 2021 at 7:59 PM Martin Sebor wrote: > > On 10/9/21 10:19 AM, Martin Sebor wrote: > > On 10/9/21 9:04 AM, Aldy Hernandez wrote: > >> On Fri, Oct 8, 2021 at 6:52 PM Martin Sebor wrote: > >>> > >>> On 10/8/21 9:12 AM, Aldy Hernandez via Gcc-patches wrote: > >>>> The following patch converts the strlen pass from evrp to ranger, > >>>> leaving DOM as the last remaining user. > >>> > >>> Thanks for doing this. I know I said I'd work on it but I'm still > >>> bogged down in my stage 1 work that's not going so great :( I just > >>> have a few minor comments/questions on the strlen change (inline) > >>> but I am a bit concerned about the test failure. > >>> > >>>> > >>>> No additional cleanups have been done. For example, the strlen pass > >>>> still has uses of VR_ANTI_RANGE, and the sprintf still passes around > >>>> pairs of integers instead of using a proper range. Fixing this > >>>> could further improve these passes. > >>>> > >>>> As a further enhancement, if the relevant maintainers deem useful, > >>>> the domwalk could be removed from strlen. That is, unless the pass > >>>> needs it for something else. > >>>> > >>>> With ranger we are now able to remove the range calculation from > >>>> before_dom_children entirely. Just working with the ranger on-demand > >>>> catches all the strlen and sprintf testcases with the exception of > >>>> builtin-sprintf-warn-22.c which is due to a limitation of the sprintf > >>>> code. I have XFAILed the test and documented what the problem is. > >>> > >>> builtin-sprintf-warn-22.c is a regression test for a false positive > >>> in Glibc. If it fails we'll have to deal with the Glibc failure > >>> again, which I would rather avoid. Have you checked to see if > >>> Glibc is affected by the change? > > > > Have you tested Glibc with the patch to see if the warning comes > > back? If it does there are other ways to suppress it, and I can > > take care of it. I just want us to avoid reintroducing it without > > doing our due diligence first. > > I've built Glibc with the latest GCC and your patch applied and > the warning is back so we'll need to suppress it there. I opened > Glibc bug 28439 and will submit a patch for it: > > https://sourceware.org/bugzilla/show_bug.cgi?id=28439 The above patch rewrites the sprintf call into a pair of strcpy. This seems like a burdensome thing to ask of our users to silence a false positive, but I leave it to the glibc experts to opine. > > There are other Glibc warnings to deal with so I don't think your > patch needs to wait until this one is resolved. > > I also missed the following in your patch: > > > gcc/ChangeLog: > > > > * Makefile.in: Disable -Wformat-overflow for > > gimple-ssa-warn-access.o. > > Rather than disabling the warning for the whole file (or any > others), unless suppressing it in the code is overly intrusive, > doing it that way is preferable. For %s sprintf directives, > specifying precision can be used to constrain the output. In > this case where each %s output is the result of formatting > an (at most) 64-bit integer, we know that the length of each > %s argument is at most 21 bytes, so specifying a precision as > big as 37 should both make sure the output fits and avoid > the warning. I.e., this should (and in my tests does) work: > > char *s1 = print_generic_expr_to_str (sizrng[1]); > sprintf (sizstr, "[%.37s, %.37s]", s0, s1); > free (s1); Thanks. I've tweaked the patch accordingly. Aldy