From patchwork Fri Jan 20 02:03:23 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 1729227 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=v6tuXcHJ; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4NyjTn235pz23fT for ; Fri, 20 Jan 2023 13:03:55 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 69EF13858031 for ; Fri, 20 Jan 2023 02:03:52 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 69EF13858031 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1674180232; bh=w1UQytJk4D3Vb6ZXtTyYzlJ3ILm+Kj5ATBp4CXDbt2s=; h=Date:To:Cc:Subject:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=v6tuXcHJO6WaMWhruP99GH/HB+zc+detX/pst2Pb4Qqr3ApIciWB+XXPuIO5aJbLM F1OjYq8fgX0Bg5iSOfYGCl12YVdWqISqT+Xq0OfAWwtgcC5WIPlJ98Tawg7vS1mevN jS73/50wCJ+LxKC+nsu0V/MWL2LyCM5x98LOxg20= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org 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 80D973858CDA for ; Fri, 20 Jan 2023 02:03:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 80D973858CDA Received: from mail-yb1-f200.google.com (mail-yb1-f200.google.com [209.85.219.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-246-PrsV81fQNwi4Fj4K6G7_Cg-1; Thu, 19 Jan 2023 21:03:27 -0500 X-MC-Unique: PrsV81fQNwi4Fj4K6G7_Cg-1 Received: by mail-yb1-f200.google.com with SMTP id d17-20020a5b0611000000b00801a0e3e117so238375ybq.13 for ; Thu, 19 Jan 2023 18:03:27 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=w1UQytJk4D3Vb6ZXtTyYzlJ3ILm+Kj5ATBp4CXDbt2s=; b=u172CSwbIuOVmjcpyc4jzT6c4raTaZhbh6gJqF2ZkX1Cy0h7fykAzVrV89zwv0P+9Z A5aZth7ZiQ3xB3M/9dVFtMeDPI4FabYFBseO1q4KF+WYGoTdxXRsFGKx1G+D6evQ+LEz 3xudCr5a+Yj2KDyIckrf92STkNGx1qofoZgnciuKv8f5nqYYlhNPQHw1zCV07U9oRiiu elwIX5G0zdEbt2RRnllo1u5rSvRUlCM5VXIpq5DeVb4EzGX0sKl9LuzVqA44oIGujMVZ i5u2zsTPL5EV/fgcUx88lw4ChXlB67+6sCPyC/5Pq9jQGlbLisVonKuqSZKsG9MjXf1U QOSA== X-Gm-Message-State: AFqh2kq1DSxtwVnlALuod6M2H9kX2vL7u+StbCmKj8URD0X3v+BvqiqN Adt6zvwZClKka2AAIsK6SYw/Yfz1P0ecnFh79G76vcYYXnrrGWAWKBOdGJ36RMynp2IxxmS2rrA 8IEMwUu7e1ZH0nYfnHg== X-Received: by 2002:a0d:dfd5:0:b0:4db:a4f8:1ef8 with SMTP id i204-20020a0ddfd5000000b004dba4f81ef8mr6884604ywe.36.1674180206876; Thu, 19 Jan 2023 18:03:26 -0800 (PST) X-Google-Smtp-Source: AMrXdXsy7lT93QKH4tsCbA0OPTZQEUTLctSQllU3h3IuCDKcvl5mhHxQDbEFXGti77sKCg01PzxGfw== X-Received: by 2002:a0d:dfd5:0:b0:4db:a4f8:1ef8 with SMTP id i204-20020a0ddfd5000000b004dba4f81ef8mr6884573ywe.36.1674180206445; Thu, 19 Jan 2023 18:03:26 -0800 (PST) Received: from redhat.com (2603-7000-9500-34a5-0000-0000-0000-1db4.res6.spectrum.com. [2603:7000:9500:34a5::1db4]) by smtp.gmail.com with ESMTPSA id t17-20020a37ea11000000b006fef157c8aesm25522022qkj.36.2023.01.19.18.03.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Jan 2023 18:03:25 -0800 (PST) Date: Thu, 19 Jan 2023 21:03:23 -0500 To: Jason Merrill Cc: GCC Patches Subject: [PATCH v3] c++: -Wdangling-reference with reference wrapper [PR107532] Message-ID: References: <20230118175200.365397-1-polacek@redhat.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/2.2.9 (2022-11-12) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-12.8 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_H2, SPF_HELO_NONE, SPF_NONE, TXREP 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-Patchwork-Original-From: Marek Polacek via Gcc-patches From: Marek Polacek Reply-To: Marek Polacek Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Sender: "Gcc-patches" On Thu, Jan 19, 2023 at 01:02:02PM -0500, Jason Merrill wrote: > On 1/18/23 20:13, Marek Polacek wrote: > > On Wed, Jan 18, 2023 at 04:07:59PM -0500, Jason Merrill wrote: > > > On 1/18/23 12:52, Marek Polacek wrote: > > > > Here, -Wdangling-reference triggers where it probably shouldn't, causing > > > > some grief. The code in question uses a reference wrapper with a member > > > > function returning a reference to a subobject of a non-temporary object: > > > > > > > > const Plane & meta = fm.planes().inner(); > > > > > > > > I've tried a few approaches, e.g., checking that the member function's > > > > return type is the same as the type of the enclosing class (which is > > > > the case for member functions returning *this), but that then breaks > > > > Wdangling-reference4.C with std::optional. > > > > > > > > So I figured that perhaps we want to look at the object we're invoking > > > > the member function(s) on and see if that is a temporary, as in, don't > > > > warn about > > > > > > > > const Plane & meta = fm.planes().inner(); > > > > > > > > but do warn about > > > > > > > > const Plane & meta = FrameMetadata().planes().inner(); > > > > > > > > It's ugly, but better than asking users to add #pragmas into their code. > > > > > > Hmm, that doesn't seem right; the former is only OK because Ref is in fact a > > > reference-like type. If planes() returned a class that held data, we would > > > want to warn. > > > > Sure, it's always some kind of tradeoff with warnings :/. > > > In this case, we might recognize the reference-like class because it has a > > > reference member and a constructor taking the same reference type. > > > > That occurred to me too, but then I found out that std::reference_wrapper > > actually uses T*, not T&, as you say. But here's a patch to do that > > (I hope). > > > That wouldn't help with std::reference_wrapper or std::ref_view because they > > > have pointer members instead of references, but perhaps loosening the check > > > to include that case would make sense? > > > > Sorry, I don't understand what you mean by loosening the check. I could > > hardcode std::reference_wrapper and std::ref_view but I don't think that's > > what you meant. > > Indeed that's not what I meant, but as I was saying in our meeting I think > it's worth doing; the compiler has various tweaks to handle specific > standard-library classes better. Okay, done in the patch below. Except that I'm not including a test for std::ranges::ref_view because I don't really know how that works. > > Surely I cannot _not_ warn for any class that contains a T*. > > I was thinking if a constructor takes a T& and the class has a T* that would > be close enough, though this also wouldn't handle the standard library > classes so the benefit is questionable. > > > Here's the patch so that we have some actual code to discuss... Thanks. > > > > -- >8 -- > > Here, -Wdangling-reference triggers where it probably shouldn't, causing > > some grief. The code in question uses a reference wrapper with a member > > function returning a reference to a subobject of a non-temporary object: > > > > const Plane & meta = fm.planes().inner(); > > > > I've tried a few approaches, e.g., checking that the member function's > > return type is the same as the type of the enclosing class (which is > > the case for member functions returning *this), but that then breaks > > Wdangling-reference4.C with std::optional. > > > > Perhaps we want to look at the member function's enclosing class > > to see if it's a reference wrapper class (meaning, has a reference > > member and a constructor taking the same reference type) and don't > > warn if so, supposing that the member function returns a reference > > to a non-temporary object. > > > > It's ugly, but better than asking users to add #pragmas into their code. > > > > PR c++/107532 > > > > gcc/cp/ChangeLog: > > > > * call.cc (do_warn_dangling_reference): Don't warn when the > > member function comes from a reference wrapper class. > > Let's factor the new code out into e.g. reference_like_class_p Done. Thanks, Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- Here, -Wdangling-reference triggers where it probably shouldn't, causing some grief. The code in question uses a reference wrapper with a member function returning a reference to a subobject of a non-temporary object: const Plane & meta = fm.planes().inner(); I've tried a few approaches, e.g., checking that the member function's return type is the same as the type of the enclosing class (which is the case for member functions returning *this), but that then breaks Wdangling-reference4.C with std::optional. Perhaps we want to look at the member function's enclosing class to see if it's a reference wrapper class (meaning, has a reference member and a constructor taking the same reference type, or is std::reference_wrapper or std::ranges::ref_view) and don't warn if so, supposing that the member function returns a reference to a non-temporary object. It's ugly, but better than asking users to add #pragmas into their code. PR c++/107532 gcc/cp/ChangeLog: * call.cc (reference_like_class_p): New. (do_warn_dangling_reference): Don't warn when the member function comes from a reference_like_class_p. gcc/testsuite/ChangeLog: * g++.dg/warn/Wdangling-reference8.C: New test. * g++.dg/warn/Wdangling-reference9.C: New test. --- gcc/cp/call.cc | 48 ++++++++++++ .../g++.dg/warn/Wdangling-reference8.C | 77 +++++++++++++++++++ .../g++.dg/warn/Wdangling-reference9.C | 21 +++++ 3 files changed, 146 insertions(+) create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference8.C create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference9.C base-commit: 0846336de56119777861e02bf68f92a6af466000 diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc index 991730713e6..672722998ee 100644 --- a/gcc/cp/call.cc +++ b/gcc/cp/call.cc @@ -13777,6 +13777,45 @@ std_pair_ref_ref_p (tree t) return true; } +/* Return true if a class CTYPE is either std::reference_wrapper or + std::ref_view, or a reference wrapper class. We consider a class + a reference wrapper class if it has a reference member and a + constructor taking the same reference type. */ + +static bool +reference_like_class_p (tree ctype) +{ + tree tdecl = TYPE_NAME (TYPE_MAIN_VARIANT (ctype)); + if (decl_in_std_namespace_p (tdecl)) + { + tree name = DECL_NAME (tdecl); + return (name + && (id_equal (name, "reference_wrapper") + || id_equal (name, "ref_view"))); + } + for (tree fields = TYPE_FIELDS (ctype); + fields; + fields = DECL_CHAIN (fields)) + { + if (TREE_CODE (fields) != FIELD_DECL || DECL_ARTIFICIAL (fields)) + continue; + tree type = TREE_TYPE (fields); + if (!TYPE_REF_P (type)) + continue; + /* OK, the field is a reference member. Do we have a constructor + taking its type? */ + for (tree fn : ovl_range (CLASSTYPE_CONSTRUCTORS (ctype))) + { + tree args = FUNCTION_FIRST_USER_PARMTYPE (fn); + if (args + && same_type_p (TREE_VALUE (args), type) + && TREE_CHAIN (args) == void_list_node) + return true; + } + } + return false; +} + /* Helper for maybe_warn_dangling_reference to find a problematic CALL_EXPR that initializes the LHS (and at least one of its arguments represents a temporary, as outlined in maybe_warn_dangling_reference), or NULL_TREE @@ -13832,6 +13871,15 @@ do_warn_dangling_reference (tree expr) if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype))) return NULL_TREE; + /* An attempt to reduce the number of -Wdangling-reference + false positives concerning reference wrappers (c++/107532). + Here we suppose that a member function of such a reference + wrapper class returns a reference to a non-temporary object. */ + if (DECL_NONSTATIC_MEMBER_FUNCTION_P (fndecl) + && !DECL_OVERLOADED_OPERATOR_P (fndecl) + && reference_like_class_p (CP_DECL_CONTEXT (fndecl))) + return NULL_TREE; + /* Here we're looking to see if any of the arguments is a temporary initializing a reference parameter. */ for (int i = 0; i < call_expr_nargs (expr); ++i) diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference8.C b/gcc/testsuite/g++.dg/warn/Wdangling-reference8.C new file mode 100644 index 00000000000..4d585891fae --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference8.C @@ -0,0 +1,77 @@ +// PR c++/107532 +// { dg-do compile { target c++11 } } +// { dg-options "-Wdangling-reference" } + +struct Plane { unsigned int bytesused; }; + +// Passes a reference through. Does not change lifetime. +template +struct Ref { + const T& i_; + Ref(const T & i) : i_(i) {} + const T & inner(); +}; + +struct FrameMetadata { + Ref planes() const { return p_; } + + Plane p_; +}; + +void bar(const Plane & meta); +void foo(const FrameMetadata & fm) +{ + const Plane & meta = fm.planes().inner(); + bar(meta); + const Plane & meta2 = FrameMetadata().planes().inner(); + bar(meta2); +} + +struct S { + const S& self () { return *this; } +} s; + +const S& r1 = s.self(); +const S& r2 = S().self(); // { dg-warning "dangling reference" } + +struct D { +}; + +struct C { + D d; + Ref get() const { return d; } +}; + +struct B { + C c; + const C& get() const { return c; } + B(); +}; + +struct A { + B b; + const B& get() const { return b; } +}; + +void +g (const A& a) +{ + const auto& d1 = a.get().get().get().inner(); + (void) d1; + const auto& d2 = A().get().get().get().inner(); + (void) d2; + const auto& d3 = A().b.get().get().inner(); + (void) d3; + const auto& d4 = a.b.get().get().inner(); + (void) d4; + const auto& d5 = a.b.c.get().inner(); + (void) d5; + const auto& d6 = A().b.c.get().inner(); + (void) d6; + Plane p; + Ref r(p); + const auto& d7 = r.inner(); + (void) d7; + const auto& d8 = Ref(p).inner(); + (void) d8; +} diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference9.C b/gcc/testsuite/g++.dg/warn/Wdangling-reference9.C new file mode 100644 index 00000000000..15c1f6b9dd2 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference9.C @@ -0,0 +1,21 @@ +// PR c++/107532 +// { dg-do compile { target c++11 } } +// { dg-options "-Wdangling-reference" } + +#include + +struct X { int n; }; + +struct S { + std::reference_wrapper wrapit() const { return x; } + X x; +}; + +void +g (const S& s) +{ + const auto& a1 = s.wrapit().get(); + (void) a1; + const auto& a2 = S().wrapit().get(); + (void) a2; +}