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.129.124]) by sourceware.org (Postfix) with ESMTPS id E07B63858401 for ; Tue, 9 Aug 2022 22:02:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E07B63858401 Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-531-zb_mPw6dNNaRj9LxR_tkaQ-1; Tue, 09 Aug 2022 18:02:11 -0400 X-MC-Unique: zb_mPw6dNNaRj9LxR_tkaQ-1 Received: by mail-qv1-f72.google.com with SMTP id np4-20020a056214370400b00476809b9caeso6923783qvb.0 for ; Tue, 09 Aug 2022 15:02:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:user-agent:references :in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from :to:cc; bh=qkV3fA5/HOtWPCSxoIeKioBLPJtRbrT76RhTQGJfCRE=; b=YLTEAlkDP0Wz+jTjUmw9gjRgv/OGTUqavcjCm55PxSX41T3+XO9cdeovoy4VLBR0GJ N2wjVrMsufUaBSuO03yjOrn+UpLzVsqz2LSGxvKJjP50gTXjWfFGXC9qKopwHPYJuxMV 2pe+lYrPVOqwAsiEP/cGPiDe6p+NhLIUER+EawuVcHFvdobou8w3gYfakS0VA3PDBZWt mYWX2mlWYzRtJ9tTRfNyi5FI3ChVyXa3k03TS6IU2Lea2ph22tmLa7/xBGW++F9BjN52 bjKpZk8En7APr9Ba/BI1xyYQXAJwsdHE6dVUFYL8fgZYjnB8r0sX9vAkM+j7vUdiAPQj 5Chw== X-Gm-Message-State: ACgBeo15ugwGEaIa1QuS5jZMkknG3hggnJ34X+ra0tMByJu2fZZAUGm/ zzBcngjHgjuMS71o8Lra5MPEglOvaQMfQ2V12PsEicMCPrpboCUhyp/3GlaCzZD3RgyY+cg2E1O tzOYVDgfYntWMrrtw/Q== X-Received: by 2002:a0c:a889:0:b0:473:d709:8753 with SMTP id x9-20020a0ca889000000b00473d7098753mr21861006qva.16.1660082530921; Tue, 09 Aug 2022 15:02:10 -0700 (PDT) X-Google-Smtp-Source: AA6agR70pV4OVO4vlMpTqM3jMNdhKWCC5++41iz6vg/rxzIQT8cdZEJamsgfBqRd4VHTnaRSiJt16w== X-Received: by 2002:a0c:a889:0:b0:473:d709:8753 with SMTP id x9-20020a0ca889000000b00473d7098753mr21860943qva.16.1660082530220; Tue, 09 Aug 2022 15:02:10 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.ma.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id j10-20020a37ef0a000000b006b8e049cf08sm5276772qkk.2.2022.08.09.15.02.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 09 Aug 2022 15:02:08 -0700 (PDT) Message-ID: <00236c70e06ebabf7d138ba8852470f70728f2c2.camel@redhat.com> Subject: Re: [PATCH 1/2] analyzer: consider that realloc could shrink the buffer [PR106539] From: David Malcolm To: Tim Lange , gcc-patches@gcc.gnu.org Date: Tue, 09 Aug 2022 18:02:07 -0400 In-Reply-To: <20220809211943.82098-1-mail@tim-lange.me> References: <20220809211943.82098-1-mail@tim-lange.me> 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=-11.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Tue, 09 Aug 2022 22:02:15 -0000 On Tue, 2022-08-09 at 23:19 +0200, Tim Lange wrote: > This patch adds the "shrinks buffer" case to the success_with_move > modelling of realloc. Hi Tim, thanks for the patch. > > 2022-08-09  Tim Lange  > > gcc/analyzer/ChangeLog: > >         PR analyzer/106539 >         * region-model-impl-calls.cc > (region_model::impl_call_realloc): >         Add get_copied_size function and pass the result as the size > of the >         new sized_region. > > --- >  gcc/analyzer/region-model-impl-calls.cc | 37 > ++++++++++++++++++++++++- >  1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/gcc/analyzer/region-model-impl-calls.cc > b/gcc/analyzer/region-model-impl-calls.cc > index 8c38e9206fa..50a19a52a21 100644 > --- a/gcc/analyzer/region-model-impl-calls.cc > +++ b/gcc/analyzer/region-model-impl-calls.cc > @@ -737,9 +737,11 @@ region_model::impl_call_realloc (const > call_details &cd) >                                                   old_size_sval); >               const svalue *buffer_content_sval >                 = model->get_store_value (sized_old_reg, cd.get_ctxt > ()); > +             const svalue *copied_size_sval > +               = get_copied_size (old_size_sval, new_size_sval); >               const region *sized_new_reg >                 = model->m_mgr->get_sized_region (new_reg, NULL, > -                                                 old_size_sval); > +                                                 copied_size_sval); I think that we need to use the same copied size svalue for both sized_old_reg and sized_new_reg, so that we're using a consistent size when getting buffer_content_sval. (I admit my handling of symbolic sizes of svalues is a bit sloppy, but I'd prefer not to add extra inconsistencies here) So the copied_size_sval determination needs to happen before getting sized_old_reg. Also, I think renaming sized_{old,new}_reg to copied_{old,new}_reg might make things clearer (we're copying from a leading subset of the old region to a leading subset of the new region). >               model->set_value (sized_new_reg, buffer_content_sval, >                                 cd.get_ctxt ()); >             } > @@ -774,6 +776,39 @@ region_model::impl_call_realloc (const > call_details &cd) >        else >         return true; >      } > + > +  private: > +    /* Return the size svalue for the new region allocated by > realloc.  */ This comment is misleading - isn't it the size of the existing data to be copied, i.e. the lesser of the old and new sizes? (allowing for the fact that one or both could be symbolic, of course) Please also add a simple explicit test case for the shrinking case (IIRC from our offlist discussion there's already one that happened to cover shrinking, but I think that testcase was doing other things too). Does the patch fix the missing leak warning for the test case in PR106539? If so, please have the patch add that to the test suite. Thanks again Dave > +    const svalue *get_copied_size (const svalue *old_size_sval, > +                                  const svalue *new_size_sval) const > +    { > +      tree old_size_cst = old_size_sval->maybe_get_constant (); > +      tree new_size_cst = new_size_sval->maybe_get_constant (); > + > +      if (old_size_cst && new_size_cst) > +       { > +         /* Both are constants and comparable.  */ > +         tree cmp = fold_binary (LT_EXPR, boolean_type_node, > +                                 old_size_cst, new_size_cst); > + > +         if (cmp == boolean_true_node) > +           return old_size_sval; > +         else > +           return new_size_sval; > +       } > +      else if (new_size_cst) > +       { > +         /* OLD_SIZE_SVAL is symbolic, so return that.  */ > +         return old_size_sval; > +       } > +      else > +       { > +         /* NEW_SIZE_SVAL is symbolic or both are symbolic. > +            Return NEW_SIZE_SVAL, because implementations of realloc > +            probably only moves the buffer if the new size is > larger.  */ > +         return new_size_sval; > +       } > +    } >    }; >   >    /* Body of region_model::impl_call_realloc.  */