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 9A4893858D1E for ; Thu, 10 Nov 2022 18:07:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9A4893858D1E 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=1668103654; 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=gH2+4C54eg20kyEGkscAR1bmRrTkOHezeZbG/8snnek=; b=ij9rGjqHwms7GaEDe712nrFapstPgKNA8h54lxEVlRTO2XxrEgn07DBHEYXhh5xtVAgCPO QokbOWKR2rVNdtU1XhGdJ01bna4xoDH84Y4VMmL8GvzhLONsr0ZAfgyQIhUGauEtOEURqj SGGETcQpdoW7xam0lFZBPwdL9K1cDGQ= Received: from mail-pl1-f198.google.com (mail-pl1-f198.google.com [209.85.214.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-533-OSlrbrLHPqGFTPxkH7qgDA-1; Thu, 10 Nov 2022 13:07:32 -0500 X-MC-Unique: OSlrbrLHPqGFTPxkH7qgDA-1 Received: by mail-pl1-f198.google.com with SMTP id e13-20020a17090301cd00b001871e6f8714so1839724plh.14 for ; Thu, 10 Nov 2022 10:07:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=gH2+4C54eg20kyEGkscAR1bmRrTkOHezeZbG/8snnek=; b=6TZCjt+n5wUgOseBZYfelfo51YUdbM8Qzx53YJVkJUJDEM8zEYE5p0at+zcYT+qD/h YjtLLKcU3UgK9UycjCP1T8NUTLttwGFybWDu6nond2Z07fMjPx/uQ3JyQjoE6zkgcfMF fDgNtYvNVZopugvB4mVgO4fDI6VRk4nNUM0cRjw17kOSdmCoJzei3dye1OuPBLBtqG49 PodUgS042MpfBdz39FJHQTb5/Yq2WuhjzTEDStCit6nA6T8VPrFoF0v+Rn2RlaX33viD fqeJIHUaml9ajQK/iRPTLJ8AObplx3X9XahLt38qkksdrNM+1+Ga2PsskBZlyotJUMf1 pPtw== X-Gm-Message-State: ACrzQf2KUaFYl4WZq5kEs28/9fS8dKmw82+vque8nXJFkgbiKvG+0qGc 2/lR32nmMqdsGhaAuPKhXMJUn1r6pAp2vRmZLnnfcAYdY8UyKikHseL2stTYaQE6xQ0eqPN3tTP 8grNwwmSXLlZNJLty5w== X-Received: by 2002:a17:902:ab91:b0:187:2e6d:a124 with SMTP id f17-20020a170902ab9100b001872e6da124mr1705626plr.43.1668103648937; Thu, 10 Nov 2022 10:07:28 -0800 (PST) X-Google-Smtp-Source: AMsMyM5vUVSRvQ4DEKPAvcRbnLGZV/mIl8JPRcu0yQY1j5EYtau7xXpD7aPD6qd+JIsE/0J61ABNMQ== X-Received: by 2002:a17:902:ab91:b0:187:2e6d:a124 with SMTP id f17-20020a170902ab9100b001872e6da124mr1705603plr.43.1668103648436; Thu, 10 Nov 2022 10:07:28 -0800 (PST) Received: from [172.22.36.61] (rrcs-24-43-233-8.west.biz.rr.com. [24.43.233.8]) by smtp.gmail.com with ESMTPSA id gf3-20020a17090ac7c300b0020a7d076bfesm127134pjb.2.2022.11.10.10.07.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 10 Nov 2022 10:07:27 -0800 (PST) Message-ID: <10294bf6-9aee-763d-4383-46ca9cf60346@redhat.com> Date: Thu, 10 Nov 2022 08:07:25 -1000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH] c++: Extend -Wdangling-reference for std::minmax To: Marek Polacek , GCC Patches References: <20221110015608.454675-1-polacek@redhat.com> From: Jason Merrill In-Reply-To: <20221110015608.454675-1-polacek@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,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 List-Id: On 11/9/22 15:56, Marek Polacek wrote: > This patch extends -Wdangling-reference to also warn for > > auto v = std::minmax(1, 2); > > which dangles because this overload of std::minmax returns > a std::pair where the two references are > bound to the temporaries created for the arguments of std::minmax. > This is a common footgun, also described at > in Notes. > > It works by extending do_warn_dangling_reference to also warn when the > function returns a std::pair. std_pair_ref_ref_p > is a new helper to check that. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > gcc/cp/ChangeLog: > > * call.cc (std_pair_ref_ref_p): New. > (do_warn_dangling_reference): Also warn when the function returns > std::pair. Recurse into TARGET_EXPR_INITIAL. > (maybe_warn_dangling_reference): Don't return early if we're > initializing a std_pair_ref_ref_p. > > gcc/ChangeLog: > > * doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst: > Extend the description of -Wdangling-reference. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wdangling-reference6.C: New test. > --- > gcc/cp/call.cc | 52 ++++++++++++++++--- > .../options-controlling-c++-dialect.rst | 10 ++++ > .../g++.dg/warn/Wdangling-reference6.C | 38 ++++++++++++++ > 3 files changed, 94 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wdangling-reference6.C > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > index 492db9b59ad..bd3b64a7e26 100644 > --- a/gcc/cp/call.cc > +++ b/gcc/cp/call.cc > @@ -13527,6 +13527,34 @@ initialize_reference (tree type, tree expr, > return expr; > } > > +/* Return true if T is std::pair. */ > + > +static bool > +std_pair_ref_ref_p (tree t) > +{ > + /* First, check if we have std::pair. */ > + if (!NON_UNION_CLASS_TYPE_P (t) > + || !CLASSTYPE_TEMPLATE_INSTANTIATION (t)) > + return false; > + tree tdecl = TYPE_NAME (TYPE_MAIN_VARIANT (t)); > + if (!decl_in_std_namespace_p (tdecl)) > + return false; > + tree name = DECL_NAME (tdecl); > + if (!name || !id_equal (name, "pair")) > + return false; > + > + /* Now see if the template arguments are both const T&. */ > + tree args = CLASSTYPE_TI_ARGS (t); > + if (TREE_VEC_LENGTH (args) != 2) > + return false; > + for (int i = 0; i < 2; i++) > + if (!TYPE_REF_OBJ_P (TREE_VEC_ELT (args, i)) > + || !CP_TYPE_CONST_P (TREE_TYPE (TREE_VEC_ELT (args, i)))) > + return false; > + > + return true; > +} > + > /* 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 > @@ -13556,11 +13584,6 @@ do_warn_dangling_reference (tree expr) > || warning_suppressed_p (fndecl, OPT_Wdangling_reference) > || !warning_enabled_at (DECL_SOURCE_LOCATION (fndecl), > OPT_Wdangling_reference) > - /* If the function doesn't return a reference, don't warn. This > - can be e.g. > - const int& z = std::min({1, 2, 3, 4, 5, 6, 7}); > - which doesn't dangle: std::min here returns an int. */ > - || !TYPE_REF_OBJ_P (TREE_TYPE (TREE_TYPE (fndecl))) > /* Don't emit a false positive for: > std::vector v = ...; > std::vector::const_iterator it = v.begin(); > @@ -13573,6 +13596,20 @@ do_warn_dangling_reference (tree expr) > && DECL_OVERLOADED_OPERATOR_IS (fndecl, INDIRECT_REF))) > return NULL_TREE; > > + tree rettype = TREE_TYPE (TREE_TYPE (fndecl)); > + /* If the function doesn't return a reference, don't warn. This > + can be e.g. > + const int& z = std::min({1, 2, 3, 4, 5, 6, 7}); > + which doesn't dangle: std::min here returns an int. > + > + If the function returns a std::pair, we > + warn, to detect e.g. > + std::pair v = std::minmax(1, 2); > + which also creates a dangling reference, because std::minmax > + returns std::pair(b, a). */ > + if (!(TYPE_REF_OBJ_P (rettype) || std_pair_ref_ref_p (rettype))) The patch is OK, but do you want to check reference to const for the single ref case as well, while you're changing this? > + 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) > @@ -13614,6 +13651,8 @@ do_warn_dangling_reference (tree expr) > return do_warn_dangling_reference (TREE_OPERAND (expr, 2)); > case PAREN_EXPR: > return do_warn_dangling_reference (TREE_OPERAND (expr, 0)); > + case TARGET_EXPR: > + return do_warn_dangling_reference (TARGET_EXPR_INITIAL (expr)); > default: > return NULL_TREE; > } > @@ -13640,7 +13679,8 @@ maybe_warn_dangling_reference (const_tree decl, tree init) > { > if (!warn_dangling_reference) > return; > - if (!TYPE_REF_P (TREE_TYPE (decl))) > + if (!(TYPE_REF_OBJ_P (TREE_TYPE (decl)) And here. > + || std_pair_ref_ref_p (TREE_TYPE (decl)))) > return; > /* Don't suppress the diagnostic just because the call comes from > a system header. If the DECL is not in a system header, or if > diff --git a/gcc/doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst b/gcc/doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst > index 5b05d31aae9..8d2a2789ef6 100644 > --- a/gcc/doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst > +++ b/gcc/doc/gcc/gcc-command-options/options-controlling-c++-dialect.rst > @@ -855,6 +855,16 @@ In addition, these warning options have meanings only for C++ programs: > const T& foo (const T&) { ... } > #pragma GCC diagnostic pop > > + :option:`-Wdangling-reference` also warns about code like > + > + .. code-block:: c++ > + > + auto p = std::minmax(1, 2); > + > + where ``std::minmax`` returns ``std::pair``, and > + both references dangle after the end of the full expression that contains > + the call to `std::minmax``. > + > This warning is enabled by :option:`-Wall`. > > .. option:: -Wno-dangling-reference > diff --git a/gcc/testsuite/g++.dg/warn/Wdangling-reference6.C b/gcc/testsuite/g++.dg/warn/Wdangling-reference6.C > new file mode 100644 > index 00000000000..bf849e290d9 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wdangling-reference6.C > @@ -0,0 +1,38 @@ > +// { dg-do compile { target c++17 } } > +// { dg-options "-Wdangling-reference" } > +// Test -Wdangling-reference with std::minmax. > + > +#include > + > +using U = std::pair; > + > +int > +fn1 () > +{ > + std::pair v = std::minmax(1, 2); // { dg-warning "dangling reference" } > + U v2 = std::minmax(1, 2); // { dg-warning "dangling reference" } > + auto v3 = std::minmax(1, 2); // { dg-warning "dangling reference" } > + return v.first + v2.second + v3.first; > +} > + > +int > +fn2 () > +{ > + int n = 1; > + auto p = std::minmax(n, n + 1); // { dg-warning "dangling reference" } > + int m = p.first; // ok > + int x = p.second; // undefined behavior > + > + // Note that structured bindings have the same issue > + auto [mm, xx] = std::minmax(n, n + 1); // { dg-warning "dangling reference" } > + (void) xx; // undefined behavior > + > + return m + x; > +} > + > +int > +fn3 () > +{ > + auto v = std::minmax({1, 2}); > + return v.first; > +} > > base-commit: 1cdfd0e5cd5fc1f493d0832ed65d31320f9585b7