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 ESMTPS id 0F33D385E01D for ; Fri, 21 Jul 2023 20:10:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0F33D385E01D Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1689970244; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wopQcX3h+IGFCaAduiVKAQnOvXdqMjvnSJm3X+DASAE=; b=W0e1XMQqlt1kpczeWscf6SRUbUBEefjnG5aBiVsV5OIgd645kObwXIvqITuFR7ANm4QphE aOyM7xbBZ3VTyrbnHjo9uD13vwgev/PLOxRkhnrFQ/8bo994isZQXKtd3tupyuDXawLDHj jlIXQYoUcOSTgfoXTj8NX4PaQtoRhTk= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-584-ZUIsxR43ONS-6-ECd-I6yg-1; Fri, 21 Jul 2023 16:10:43 -0400 X-MC-Unique: ZUIsxR43ONS-6-ECd-I6yg-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-76735d5eb86so319720685a.3 for ; Fri, 21 Jul 2023 13:10:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689970242; x=1690575042; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=L2KYru2ulMofOO7I2Bp88FWwl5Hvd2L19gt0N0uj4i0=; b=IFnnuxhfP1fZh6PCJl0bPutJkgEjuslY978Aql7B5ovZmlPB+ZH8erXkMRfVqX3HJf 8nPCF5xO05N6X7n5c8DB5YbtWMBl3PBiAeSeBR/UWUg+zgDLuezmr6ZMm31w8WxYE1N3 +n04ASDjQYIAsnsWDGSGIyF1RQMJY4X5mstn7dm2laP2u0qka5jOxSvc0UiOxtvNO2Op x7kofUlYPPgRIAadCm5nsjpW81IXeJC5tHy/yXnG+YPYJPFH7P0CZyLOiXTB6iug0flI zUByQ90Ngmp6CWzdNTfCOwjb49ctGwkI1ZnMtgV0lMreGKUV7AlzYhIWhf9k8y9qCg4r dFeA== X-Gm-Message-State: ABy/qLa8roVSSutZku4lr2eOlRxD7jE3nInWp90KCfY0PavmqyS4p/Hd 3SSFfjTnNsBy5+A+WmqxHmUBChQdScvlctu2zaPPJc2eTf/m7vOsoXyc0DDP5bYrye1xnTx0dTl lpcFs4MqwDUE8eqT4lg== X-Received: by 2002:a05:620a:2454:b0:767:4246:1f83 with SMTP id h20-20020a05620a245400b0076742461f83mr1306501qkn.49.1689970242577; Fri, 21 Jul 2023 13:10:42 -0700 (PDT) X-Google-Smtp-Source: APBJJlEGGhEEeFEoNuItcdStRHeUzsV2sAAYoLGIbiJ5MwGD+2r/L40MwrMj3GlUncr+NmOX4oL8zA== X-Received: by 2002:a05:620a:2454:b0:767:4246:1f83 with SMTP id h20-20020a05620a245400b0076742461f83mr1306480qkn.49.1689970242182; Fri, 21 Jul 2023 13:10:42 -0700 (PDT) Received: from t14s.localdomain (c-76-28-97-5.hsd1.ma.comcast.net. [76.28.97.5]) by smtp.gmail.com with ESMTPSA id s25-20020a05620a16b900b00767d47eb29bsm1330994qkj.119.2023.07.21.13.10.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 21 Jul 2023 13:10:41 -0700 (PDT) Message-ID: <22642df42fda4697f984e5e3cca883d2d3584556.camel@redhat.com> Subject: Re: [PATCH] analyzer: Add support of placement new and improved operator new [PR105948] From: David Malcolm To: priour.be@gmail.com, gcc-patches@gcc.gnu.org Date: Fri, 21 Jul 2023 16:10:40 -0400 In-Reply-To: <20230706144353.2140425-1-vultkayn@gcc.gnu.org> References: <20230706144353.2140425-1-vultkayn@gcc.gnu.org> User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,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 List-Id: On Thu, 2023-07-06 at 16:43 +0200, priour.be@gmail.com wrote: > As per David's suggestion. > - Improved leading comment of "is_placement_new_p" > - "kf_operator_new::matches_call_types_p" now checks that arg 0 is of > =C2=A0 integral type and that arg 1, if any, is of pointer type. > - Changed ambiguous "int" to "int8_t" and "int64_t" in placement-new- > size.C > =C2=A0 to trigger a target independent out-of-bounds warning. > =C2=A0 Other OOB tests were not based on the size of types, but on the > number > =C2=A0 elements, so them using "int" didn't lead to any ambiguity. >=20 > contrib/check_GNU_style.sh still complains about a space before > square > brackets in string "operator new []", but as before, this one space > is > mandatory for a correct recognition of the function. >=20 > Changes succesfully regstrapped on x86_64-linux-gnu against trunk > 3c776fdf1a8. >=20 > Is it OK for trunk ? > Thanks again, > Benjamin. Hi Benjamin, thanks for the updated patch. As before, this looks close to being ready, but I have some further comments: [...snip...] > diff --git a/gcc/analyzer/kf-lang-cp.cc b/gcc/analyzer/kf-lang-cp.cc > index 393b4f25e79..ef057da863f 100644 > --- a/gcc/analyzer/kf-lang-cp.cc > +++ b/gcc/analyzer/kf-lang-cp.cc > @@ -35,6 +35,49 @@ along with GCC; see the file COPYING3. If not see > =20 > #if ENABLE_ANALYZER > =20 > +/* Return true if CALL is a non-allocating operator new or operator new = [] > + that contains no user-defined args, i.e. having any signature of: > + > + - void* operator new (std::size_t count, void* ptr); > + - void* operator new[] (std::size_t count, void* ptr); > + > + See https://en.cppreference.com/w/cpp/memory/new/operator_new. */ > + > +bool is_placement_new_p (const gcall *call) > +{ > + gcc_assert (call); > + > + tree fndecl =3D gimple_call_fndecl (call); > + if (!fndecl) > + return false; > + > + if (!is_named_call_p (fndecl, "operator new", call, 2) > + && !is_named_call_p (fndecl, "operator new []", call, 2)) > + return false; > + tree arg1 =3D gimple_call_arg (call, 1); > + > + if (!POINTER_TYPE_P (TREE_TYPE (arg1))) > + return false; > + > + /* We must distinguish between an allocating non-throwing new > + and a non-allocating new. > + > + The former might have one of the following signatures : > + void* operator new (std::size_t count, const std::nothrow_t& tag); > + void* operator new[] (std::size_t count, const std::nothrow_t& tag); > + > + However, debugging has shown that TAG is actually a POINTER_TYPE, > + not a REFERENCE_TYPE. > + > + Thus, we cannot easily differentiate the types, but we instead have = to > + check if the second argument's type identifies as nothrow_t. */ > + tree identifier =3D TYPE_IDENTIFIER (TREE_TYPE (TREE_TYPE (arg1))); > + if (!identifier) > + return true; > + const char *name =3D IDENTIFIER_POINTER (identifier); > + return 0 !=3D strcmp (name, "nothrow_t"); > +} > + If we're looking for a simple "void *", wouldn't it be simpler and cleaner to check for arg1 being a pointer to a type that's VOID_TYPE_P, rather than this name comparison? [...snip...] > diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc > index a8c63eb1ce8..41c313c07dd 100644 > --- a/gcc/analyzer/sm-malloc.cc > +++ b/gcc/analyzer/sm-malloc.cc > @@ -754,7 +754,7 @@ public: > override > { > if (change.m_old_state =3D=3D m_sm.get_start_state () > -=09&& unchecked_p (change.m_new_state)) > +=09&& (unchecked_p (change.m_new_state) || nonnull_p (change.m_new_state= ))) > // TODO: verify that it's the allocation stmt, not a copy > return label_text::borrow ("allocated here"); > if (unchecked_p (change.m_old_state) > @@ -1910,11 +1910,16 @@ malloc_state_machine::on_stmt (sm_context *sm_ctx= t, > =09 return true; > =09 } > =20 > -=09if (is_named_call_p (callee_fndecl, "operator new", call, 1)) > -=09 on_allocator_call (sm_ctxt, call, &m_scalar_delete); > -=09else if (is_named_call_p (callee_fndecl, "operator new []", call, 1)) > -=09 on_allocator_call (sm_ctxt, call, &m_vector_delete); > -=09else if (is_named_call_p (callee_fndecl, "operator delete", call, 1) > +=09if (!is_placement_new_p (call)) > +=09 { > + bool returns_nonnull =3D !TREE_NOTHROW (callee_fndecl) && flag_excepti= ons; > + if (is_named_call_p (callee_fndecl, "operator new")) > + on_allocator_call (sm_ctxt, call, &m_scalar_delete, returns_nonnull)= ; > + else if (is_named_call_p (callee_fndecl, "operator new []")) > + on_allocator_call (sm_ctxt, call, &m_vector_delete, returns_nonnull)= ; > +=09 } > + > +=09if (is_named_call_p (callee_fndecl, "operator delete", call, 1) > =09=09 || is_named_call_p (callee_fndecl, "operator delete", call, 2)) > =09 { > =09 on_deallocator_call (sm_ctxt, node, call, It looks like something's gone wrong with the indentation in the above: previously we had tab characters, but now I'm seeing a pair of spaces, which means this wouldn't line up properly. This might be a glitch somewhere in our email workflow, but please check it in your editor (with visual whitespace enabled). [...snip...] Some comments on the test cases: > diff --git a/gcc/testsuite/g++.dg/analyzer/new-2.C b/gcc/testsuite/g++.dg/analyzer/new-2.C > new file mode 100644 > index 00000000000..4e696040a54 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/new-2.C > @@ -0,0 +1,50 @@ > +// { dg-additional-options "-O0" } > + > +struct A > +{ > + int x; > + int y; > +}; > + > +void test_spurious_null_warning_throwing () > +{ > + int *x =3D new int; /* { dg-bogus "dereference of possibly-NULL" } */ > + int *y =3D new int (); /* { dg-bogus "dereference of possibly-NULL" "n= on-throwing" } */ > + int *arr =3D new int[3]; /* { dg-bogus "dereference of possibly-NULL" = } */=20 > + A *a =3D new A (); /* { dg-bogus "dereference of possibly-NULL" "throw= ing new cannot be null" } */ > + > + int z =3D *y + 2; > + z =3D *x + 4; /* { dg-bogus "dereference of possibly-NULL 'x'" } */ > + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* = } .-1 } */ > + z =3D arr[0] + 4; /* { dg-bogus "dereference of possibly-NULL" } */ > + > + delete a; > + delete y; > + delete x; > + delete[] arr; > +} > + > +void test_default_initialization () > +{ > + int *y =3D ::new int; > + int *x =3D ::new int (); /* { dg-bogus "dereference of possibly-NULL= 'operator new" } */ > + > + int b =3D *x + 3; /* { dg-bogus "dereference of possibly-NULL" } */ > + /* { dg-bogus "use of uninitialized =E2=80=98*x=E2=80=99" "" { targe= t *-*-* } .-1 } */ > + int a =3D *y + 2; /* { dg-bogus "dereference of possibly-NULL 'y'" }= */ > + /* { dg-warning "use of uninitialized value '\\*y'" "no default init= " { target *-*-* } .-1 } */ > + > + delete x; > + delete y; > +} > + > +/* From clang core.uninitialized.NewArraySize > +new[] should not be called with an undefined size argument */ > + > +void test_garbage_new_array () > +{ > + int n; > + int *arr =3D ::new int[n]; /* { dg-warning "use of uninitialized value= 'n'" } */ > + arr[0] =3D 7; > + ::delete[] arr; /* no warnings emitted here either */ > +} > diff --git a/gcc/testsuite/g++.dg/analyzer/noexcept-new.C b/gcc/testsuite= /g++.dg/analyzer/noexcept-new.C > new file mode 100644 > index 00000000000..7699cd99cff > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C > @@ -0,0 +1,48 @@ > +/* { dg-additional-options "-O0 -fno-exceptions -fno-analyzer-suppress-f= ollowups" } */ > +#include > + > +/* Test non-throwing variants of operator new */ > + > +struct A > +{ > + int x; > + int y; > +}; > + > +void test_throwing () > +{ > + int* x =3D new int; > + int* y =3D new int(); /* { dg-warning "dereference of possibly-NULL" }= */ > + int* arr =3D new int[10]; > + A *a =3D new A(); /* { dg-warning "dereference of possibly-NULL" } */ > + > + int z =3D *y + 2; > + z =3D *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */ > + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* = } .-1 } */ > + z =3D arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'"= } */ > + /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-= * } .-1 } */ > + a->y =3D a->x + 3; > + > + delete a; > + delete y; > + delete x; > + delete[] arr; > +} > + > +void test_nonthrowing () > +{ > + int* x =3D new(std::nothrow) int; > + int* y =3D new(std::nothrow) int(); > + int* arr =3D new(std::nothrow) int[10]; > + > + int z =3D *y + 2; /* { dg-warning "dereference of NULL 'y'" } */ > + /* { dg-warning "use of uninitialized value '\\*y'" "" { target *-*-* = } .-1 } */ > + z =3D *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */ > + /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-* = } .-1 } */ > + z =3D arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'"= } */ > + /* { dg-warning "use of uninitialized value '\\*arr'" "" { target *-*-= * } .-1 } */ > + > + delete y; > + delete x; > + delete[] arr; > +} I see that we have test coverage for: noexcept-new.C: -fno-exceptions with new vs nothrow-new whereas: new-2.C has (implicitly) -fexceptions with new It seems that of the four combinations for: - exceptions enabled or disabled and: - throwing versus non-throwing new this is covering three of the cases=C2=A0but is missing the case of nothrow= - new when exceptions are enabled. Presumably new-2.C should gain test coverage for this case. Or am I missing something here? Am I right in thinking that it's possible for the user to use nothrow new when exceptions are enabled to get a new that can fail and return nullptr? Or is that not possible? [...snip...] Hope this makes sense; thanks again for the patch Dave