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 24F723858434 for ; Wed, 1 Sep 2021 21:39:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 24F723858434 Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-170-t0VImiFTP16ioEYpoY8ZTQ-1; Wed, 01 Sep 2021 17:39:22 -0400 X-MC-Unique: t0VImiFTP16ioEYpoY8ZTQ-1 Received: by mail-qt1-f197.google.com with SMTP id c22-20020ac80096000000b0029f6809300eso972682qtg.6 for ; Wed, 01 Sep 2021 14:39:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=NNyTeH7u27xY+VZcvBLdQg7da9p/FfctkWLog0UgOmY=; b=p367oCXvPQqYQEBR9sU4hmGvIRlu0TbOy+TdWTT7hNxc/m2gw0bUt9L23HrCd5MMZx HNlB6NwKDjpsBtjqTz3vEJnhONEwfxN5ETt+lrHkKrORNlzO1XNKC0IlVNtJ0yEDW8L/ rSK+y7wxnyrbmt0EVbASedGH8KBeAe4D7aSzaUkeA+VkIJpEDMwzucM3FlfEJ6HslRNf af9yuKEtrR5YtebbCUqe83RFAtGST+FtoNZlKGRyFM1E1ORPcU83kXkE2ydSgIzWqzfc up4r293TGBneVBlqoCOQe4Od7G5rtt7/jPgVScyHjAhJy8YxxRlQ4dCK/Wt+a6Fxo0UN FPEA== X-Gm-Message-State: AOAM531R2yjN2HMyklZltkW1wJ30JxdDnWrxl9AbeKp/rz3FiAonQ63J 47zkt1bGtaMh6K7azxIb+dKQFfHlwgCnCxIaYFzcYs9Z9DsubQz18bn2IUsP0Z3/aqH7YIgIN9u EVxWZXSVkX8/5nZOP/A== X-Received: by 2002:ac8:6697:: with SMTP id d23mr1580818qtp.34.1630532361497; Wed, 01 Sep 2021 14:39:21 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwaca0h3I+pQCOUpVtrB6AWQym261fX4IPRidrrXA0XkhdudBjg7o4zsTlyAeaYJAptLl0veA== X-Received: by 2002:ac8:6697:: with SMTP id d23mr1580801qtp.34.1630532361192; Wed, 01 Sep 2021 14:39:21 -0700 (PDT) Received: from [192.168.1.149] (130-44-159-43.s11817.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id p187sm596388qkd.101.2021.09.01.14.39.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 01 Sep 2021 14:39:20 -0700 (PDT) Subject: Re: [PATCH] warn for more impossible null pointer tests To: Martin Sebor , gcc-patches , "Joseph S. Myers" References: <0858e35e-3271-0dc2-4376-51228645ce79@redhat.com> From: Jason Merrill Message-ID: <40945df3-b63d-87f5-2b9b-e926a24bb8b5@redhat.com> Date: Wed, 1 Sep 2021 17:39:19 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, 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: Wed, 01 Sep 2021 21:39:26 -0000 On 9/1/21 4:33 PM, Martin Sebor wrote: > On 9/1/21 1:21 PM, Jason Merrill wrote: >> On 8/31/21 10:08 PM, Martin Sebor wrote: >>> A Coverity run recently uncovered a latent bug in GCC that GCC should >>> be able to detect itself: comparing the address of a declared object >>> for equality to null, similar to: >>> >>>    int f (void) >>>    { >>>      int a[2][2]; >>>      return &a == 0; >>>    } >>> >>> GCC issues -Waddress for this code, but the bug Coverity found was >>> actually closer to the following: >>> >>>    int f (void) >>>    { >>>      int a[2][2]; >>>      return a[0] == 0; >>>    } >>> >>> where the hapless author (yours truly) meant to compare the value >>> of a[0][0] (as in r12-3268). >>> >>> This variant is not diagnosed even though the bug in it is the same >>> and I'd expect more likely to occur in practice.  (&a[0] == 0 isn't >>> diagnosed either, though that's a less likely mistake to make). >>> >>> The attached patch enhances -Waddress to detect this variant along >>> with a number of other similar instances of the problem, including >>> comparing the address of array members to null. >>> >>> Besides these, the patch also issues -Waddress for null equality >>> tests of pointer-plus expressions such as in: >>> >>>    int g (int i) >>>    { >>>      return a[0] + i == 0; >>>    } >>> >>> and in C++ more instances of pointers to members. >>> >>> Testing on x86_64-linux, besides a few benign issues in GCC sources >>> a regression test, run shows a failure in gcc.dg/Waddress.c.  That's >>> a test added after GCC for some reason stopped warning for one of >>> the basic cases that other tools warn about (comparing an array to >>> null).  I suspect the change was unintentional because GCC still >>> warns for other very similar expressions.  The reporter who also >>> submitted the test in pr36299 argued that the warning wasn't >>> helpful because tests for arrays sometimes come from macros, and >>> the test was committed after it was noted that GCC no longer warned >>> for the reporter's simple case.  While it's certainly true that >>> the warning can be triggered by the null equality tests in macros >>> (the patch exposed two such instances in GCC) they are easy to >>> avoid (the patch adds a an additional escape hatch).  At the same >>> time, as is evident from the Coverity bug report and from the two >>> issues the enhancement exposes in the FORTRAN front end (even if >>> benign), issuing the warning in these cases does help find bugs >>> or mistaken assumptions.  With that, I've changed the test to >>> expect the restored -Waddress warning instead. >>> >>> Testing with Glibc exposed a couple of harmless comparisons of >>> arrays a large macro in vfprintf-internal.c.  I'll submit a fix >>> to avoid the -Waddress instances if/when this enhancement is >>> approved. >>> >>> Testing with Binutils/GDB also turned up a couple of pointless >>> comparison of arrays to null and a couple of uses in macros that >>> can be trivially suppressed. >>> >>> Martin >>> >>> PS Clang issues a warning for some of the same null pointer tests >>> the patch diagnoses, including gcc.dg/Waddress.c, except under at >>> least three different options: some under -Wpointer-bool-conversion, >>> others under -Wtautological-pointer-compare, and others still under >>> -Wtautological-compare. >> >>> +      while (TREE_CODE (cop) == ARRAY_REF >>> +         || TREE_CODE (cop) == COMPONENT_REF) >>> +    { >>> +      unsigned opno = TREE_CODE (cop) == COMPONENT_REF; >>> +      cop = TREE_OPERAND (cop, opno); >>> +    } >> >> 1) Maybe 'while (handled_component_p (cop))'? >> 2) Why handle COMPONENT_REF differently?  Operand 1 is the FIELD_DECL, >> which doesn't have an address of its own. > > This is because the address of a field is never null, regardless of > what the P in in &P->M points to. True, though I'd change "invalid" to "undefined" in the comment for decl_with_nonnull_addr_p. > (With the caveat mentioned in > the comment further up about the pointer used to access the member > being nonnull.)  So this is diagnosed: > >   extern struct { int m; } *p; >   bool b = &p->m == 0; > > Using handled_component_p() in a loop would prevent that. Would it? p isn't declared weak. > For array_refs, the loop gets us the decl to mention in the warning. > But this should work too and looks cleaner: > >       cop = TREE_OPERAND (cop, 0); > >       /* Get the outermost array.  */ >       while (TREE_CODE (cop) == ARRAY_REF) >     cop = TREE_OPERAND (cop, 0); > >       /* Get the member (its address is never null).  */ >       if (TREE_CODE (cop) == COMPONENT_REF) >     cop = TREE_OPERAND (cop, 1); > > Do you prefer the above instead? Sure. OK with that change and the comment tweak above. Jason