Message ID | 20221110015608.454675-1-polacek@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: Extend -Wdangling-reference for std::minmax | expand |
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
On Thu, Nov 10, 2022 at 08:07:25AM -1000, Jason Merrill wrote: > 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? Thanks. Yes, I plan to do that soon, but I didn't want to do it in a single patch, because I want a dedicated test for 'int&' v. 'const int&' and that felt like a follow-up patch. > > + 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. Right. Marek
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))) + 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)) + || 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; +}