From: Jason Merrill <jason@redhat.com>
To: Marek Polacek <polacek@redhat.com>,
GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] c++: Extend -Wdangling-reference for std::minmax
Date: Thu, 10 Nov 2022 08:07:25 -1000 [thread overview]
Message-ID: <10294bf6-9aee-763d-4383-46ca9cf60346@redhat.com> (raw)
In-Reply-To: <20221110015608.454675-1-polacek@redhat.com>
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<const int&, const int&> where the two references are
> bound to the temporaries created for the arguments of std::minmax.
> This is a common footgun, also described at
> <https://en.cppreference.com/w/cpp/algorithm/minmax> in Notes.
>
> It works by extending do_warn_dangling_reference to also warn when the
> function returns a std::pair<const T&, const T&>. 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<const T&, const T&>. 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<const T&, const T&>. */
> +
> +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<int> v = ...;
> std::vector<int>::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<const T&, const T&>, we
> + warn, to detect e.g.
> + std::pair<const int&, const int&> v = std::minmax(1, 2);
> + which also creates a dangling reference, because std::minmax
> + returns std::pair<const T&, const T&>(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<const int&, const int&>``, 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 <algorithm>
> +
> +using U = std::pair<const int&, const int&>;
> +
> +int
> +fn1 ()
> +{
> + std::pair<const int&, const int&> 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
next prev parent reply other threads:[~2022-11-10 18:07 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-10 1:56 Marek Polacek
2022-11-10 18:07 ` Jason Merrill [this message]
2022-11-10 18:16 ` Marek Polacek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=10294bf6-9aee-763d-4383-46ca9cf60346@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=polacek@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).