Re: [PATCH v3] c++: Implement -Wdangling-reference [PR106393]
On 10/26/22 12:10, Marek Polacek wrote:
On Tue, Oct 25, 2022 at 11:53:51AM -0400, Jason Merrill via Gcc-patches wrote:
On 10/25/22 11:21, Marek Polacek wrote:
On Mon, Oct 24, 2022 at 01:30:42PM -0400, Jason Merrill wrote:
On 10/21/22 19:28, Marek Polacek wrote:
It doesn't warn when the function in question is a member function, otherwise
it'd emit loads of warnings for valid code like obj.emplace({0}, 0).
We had discussed warning if the object argument is a temporary (and for the
above check, the function returns *this)?
Presumably you mean detecting something like this:
struct S {
const S& self () { return *this; }
};
const S& s = S().self();
Yes. Or
struct S {
int ar[4];
int& operator[](int i) { return ar[i]; }
};
const int &r = S()[2];
Ok, I've extended the warning to check if the object argument is a temporary
as well, so we get a warning here now.
I don't currently have a way to detect it, can I steal a METHOD_TYPE flag
that says "this member function returns *this"? Alternatively, walk its
DECL_SAVED_TREE and look for RETURN_EXPR?
Like you limited the above check to TREE_STATIC, let's also forget about
checking for return *this.
I don't follow, but it looks like I don't need to change anything and just
keep the TREE_STATIC check?
It warns in member initializer lists as well:
const int& f(const int& i) { return i; }
struct S {
const int &r; // { dg-warning "dangling reference" }
S() : r(f(10)) { } // { dg-message "destroyed" }
};
I've run the testsuite/bootstrap with the warning enabled by default.
There were just a few FAILs:
* g++.dg/warn/Wdangling-pointer-2.C
* 20_util/any/misc/any_cast.cc
* 20_util/forward/c_neg.cc
* 20_util/forward/f_neg.cc
* experimental/any/misc/any_cast.cc
all of these look like genuine bugs. A bootstrap with the warning
enabled by default passed.
When testing a previous version of the patch, there were many FAILs in
libstdc++'s 22_locale/; all of them because the warning triggered on
const test_type& obj = std::use_facet(std::locale());
but this code looks valid -- std::use_facet doesn't return a reference
to its parameter. Therefore I added code to suppress the warning when
the call is std::use_facet. Now 22_locale/* pass even with the warning
on. We could exclude more std:: functions like this if desirable.
Instead of adding special cases in the compiler, let's disable the warning
around the definition of use_facet (and adjust the compiler as needed so
that avoids the warning).
As I said in
I don't think it's possible without inventing an attribute (?).
Replied.
Fixed.
+ tree fndecl = cp_get_callee_fndecl_nofold (call);
+ if (!fndecl
+ || warning_suppressed_p (fndecl, OPT_Wdangling_reference)
+ /* Don't warn about member functions; the warning would trigger in
+valid code like
+ std::any a(...);
+ S& s = a.emplace({0}, 0);
+which constructs a new object and returns a reference to it. */
+ || DECL_NONSTATIC_MEMBER_FUNCTION_P (fndecl)
+ /* It seems unreasonable to warn about operator functions. */
+ || DECL_OVERLOADED_OPERATOR_P (fndecl)
I guess I'd expect false positives on << and >> because of iostreams, do you
see false positives with other operators?
This was just a guess. The warning triggered in g++.dg/overload/operator6.C.
That looks like a true positive.
Ok, then...
I suppose this could be limited to << and >>? I'm not sure.
I'm not sure either. Another possibility would be to only consider the LHS
of binary operators, since returning a reference to the RHS is very unusual.
...I've removed the DECL_OVERLOADED_OPERATOR_P check altogether and don't
really see any false positives. It's probably better to start with a more
general warning and then tweak it based on empirical evidence.
+// Invalid, but we don't warn here yet.
+// r12 = f (f ((const int &) &TARGET_EXPR ))
+const int& r12 = f(f(1));
This should be a simple recursion?
Hmm, the inner call is just a sub-expression of the full-expression so
there you can still use the returned temporary. But in this case the
temporary is used beyond the full-expression so it's invalid. I've added
if (tree r = find_initializing_call_expr (arg))
if (cp_tree_equal (CALL_EXPR_FN (r), CALL_EXPR_FN (expr)))
return expr;
so that we detect f(f(1)) but I'm dubious that this is actually useful.
Indeed, but why limit it to checking for the same function rather than also
warning for
f(g(1))
or
A().f().g()
?
OK, I've made the warning even more recursive so that now we detect
both of these, see Wdangling-reference3.C. While at it, I've renamed
the functions.
As before, I've tested this patch twice, once with -Wdangling-reference
enabled by default, once with -Wdangling-reference enabled by -Wextra.
The couple of FAILs I saw were true positive
[PATCH v3] c++: Implement -Wdangling-reference [PR106393]
On Tue, Oct 25, 2022 at 11:53:51AM -0400, Jason Merrill via Gcc-patches wrote:
> On 10/25/22 11:21, Marek Polacek wrote:
> > On Mon, Oct 24, 2022 at 01:30:42PM -0400, Jason Merrill wrote:
> > > On 10/21/22 19:28, Marek Polacek wrote:
> > > > It doesn't warn when the function in question is a member function,
> > > > otherwise
> > > > it'd emit loads of warnings for valid code like obj.emplace({0}, 0).
> > >
> > > We had discussed warning if the object argument is a temporary (and for
> > > the
> > > above check, the function returns *this)?
> >
> > Presumably you mean detecting something like this:
> >
> > struct S {
> >const S& self () { return *this; }
> > };
> > const S& s = S().self();
>
> Yes. Or
>
> struct S {
> int ar[4];
> int& operator[](int i) { return ar[i]; }
> };
> const int &r = S()[2];
Ok, I've extended the warning to check if the object argument is a temporary
as well, so we get a warning here now.
> > I don't currently have a way to detect it, can I steal a METHOD_TYPE flag
> > that says "this member function returns *this"? Alternatively, walk its
> > DECL_SAVED_TREE and look for RETURN_EXPR?
>
> Like you limited the above check to TREE_STATIC, let's also forget about
> checking for return *this.
I don't follow, but it looks like I don't need to change anything and just
keep the TREE_STATIC check?
> > > > It warns in member initializer lists as well:
> > > >
> > > > const int& f(const int& i) { return i; }
> > > > struct S {
> > > > const int &r; // { dg-warning "dangling reference" }
> > > > S() : r(f(10)) { } // { dg-message "destroyed" }
> > > > };
> > > >
> > > > I've run the testsuite/bootstrap with the warning enabled by default.
> > > > There were just a few FAILs:
> > > > * g++.dg/warn/Wdangling-pointer-2.C
> > > > * 20_util/any/misc/any_cast.cc
> > > > * 20_util/forward/c_neg.cc
> > > > * 20_util/forward/f_neg.cc
> > > > * experimental/any/misc/any_cast.cc
> > > > all of these look like genuine bugs. A bootstrap with the warning
> > > > enabled by default passed.
> > > >
> > > > When testing a previous version of the patch, there were many FAILs in
> > > > libstdc++'s 22_locale/; all of them because the warning triggered on
> > > >
> > > > const test_type& obj = std::use_facet(std::locale());
> > > >
> > > > but this code looks valid -- std::use_facet doesn't return a reference
> > > > to its parameter. Therefore I added code to suppress the warning when
> > > > the call is std::use_facet. Now 22_locale/* pass even with the warning
> > > > on. We could exclude more std:: functions like this if desirable.
> > >
> > > Instead of adding special cases in the compiler, let's disable the warning
> > > around the definition of use_facet (and adjust the compiler as needed so
> > > that avoids the warning).
> >
> > As I said in
> >
> > I don't think it's possible without inventing an attribute (?).
>
> Replied.
Fixed.
> > > > + tree fndecl = cp_get_callee_fndecl_nofold (call);
> > > > + if (!fndecl
> > > > + || warning_suppressed_p (fndecl, OPT_Wdangling_reference)
> > > > + /* Don't warn about member functions; the warning would trigger
> > > > in
> > > > +valid code like
> > > > + std::any a(...);
> > > > + S& s = a.emplace({0}, 0);
> > > > +which constructs a new object and returns a reference to it.
> > > > */
> > > > + || DECL_NONSTATIC_MEMBER_FUNCTION_P (fndecl)
> > > > + /* It seems unreasonable to warn about operator functions. */
> > > > + || DECL_OVERLOADED_OPERATOR_P (fndecl)
> > >
> > > I guess I'd expect false positives on << and >> because of iostreams, do
> > > you
> > > see false positives with other operators?
> >
> > This was just a guess. The warning triggered in
> > g++.dg/overload/operator6.C.
>
> That looks like a true positive.
Ok, then...
> > I suppose this could be limited to << and >>? I'm not sure.
>
> I'm not sure either. Another possibility would be to only consider the LHS
> of binary operators, since returning a reference to the RHS is very unusual.
...I've removed the DECL_OVERLOADED_OPERATOR_P check altogether and don't
really see any false positives. It's probably better to start with a more
general warning and then tweak it based on empirical evidence.
> > > > +// Invalid, but we don't warn here yet.
> > > > +// r12 = f (f ((const int &) &TARGET_EXPR ))
> > > > +const int& r12 = f(f(1));
> > >
> > > This should be a simple recursion?
> >
> > Hmm, the inner call is just a sub-expression of the full-expression so
> > there you can still use the returned temporary. But in this case the
> > temporary is used beyond the full-expression so it's invalid. I've added
> >
> > if (tree r = find_initializing_call_expr (arg))
> > if (cp_tree_equal (CALL_EXPR_FN (r), CALL_EXPR_FN (expr)))
> > return expr;
> >
> > s
