From: Jason Merrill <jason@redhat.com>
To: Marek Polacek <polacek@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH v5] c++: implement [[gnu::non_owning]] [PR110358]
Date: Fri, 1 Mar 2024 15:38:48 -0500 [thread overview]
Message-ID: <584f955c-14eb-40b2-9365-a02279c3a763@redhat.com> (raw)
In-Reply-To: <ZeIresptVf4uV5-T@redhat.com>
On 3/1/24 14:24, Marek Polacek wrote:
> On Fri, Mar 01, 2024 at 01:19:40PM -0500, Jason Merrill wrote:
>> On 3/1/24 12:39, Marek Polacek wrote:
>>> @option{-Wdangling-reference} also warns about code like
>>> @smallexample
>>> @@ -3932,6 +3935,10 @@ struct Span @{
>>> as @code{std::span}-like; that is, the class is a non-union class
>>> that has a pointer data member and a trivial destructor.
>>> +The warning can be disabled by using the @code{gnu::no_dangling} attribute
>>> +on a function (@pxref{Common Function Attributes}), or a class type
>>> +(@pxref{C++ Attributes}).
>>
>> It seems surprising that one is in a generic attributes section and the
>> other in the C++-specific section. Maybe both uses could be covered in the
>> C++ attributes section?
>
> Arg yes, definitely. Done here.
>
>>> This warning is enabled by @option{-Wall}.
>>> @opindex Wdelete-non-virtual-dtor
>>> diff --git a/gcc/testsuite/g++.dg/ext/attr-no-dangling1.C b/gcc/testsuite/g++.dg/ext/attr-no-dangling1.C
>>> new file mode 100644
>>> index 00000000000..02eabbc5003
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/ext/attr-no-dangling1.C
>>> @@ -0,0 +1,38 @@
>>> +// { dg-do compile { target c++11 } }
>>> +// { dg-options "-Wdangling-reference" }
>>> +
>>> +int g = 42;
>>> +
>>> +struct [[gnu::no_dangling]] A {
>>> + int *i;
>>> + int &foo() { return *i; }
>>> +};
>>> +
>>> +struct A2 {
>>> + int *i;
>>> + [[gnu::no_dangling]] int &foo() { return *i; }
>>> + [[gnu::no_dangling]] static int &bar (const int &) { return *&g; }
>>> +};
>>> +
>>> +union [[gnu::no_dangling]] U { };
>>> +
>>> +A a() { return A{&g}; }
>>> +A2 a2() { return A2{&g}; }
>>> +
>>> +class X { };
>>> +const X x1;
>>> +const X x2;
>>> +
>>> +[[gnu::no_dangling]] const X& get(const int& i)
>>> +{
>>> + return i == 0 ? x1 : x2;
>>> +}
>>> +
>>> +void
>>> +test ()
>>> +{
>>> + [[maybe_unused]] const X& x = get (10); // { dg-bogus "dangling" }
>>> + [[maybe_unused]] const int &i = a().foo(); // { dg-bogus "dangling" }
>>> + [[maybe_unused]] const int &j = a2().foo(); // { dg-bogus "dangling" }
>>> + [[maybe_unused]] const int &k = a2().bar(10); // { dg-bogus "dangling" }
>>> +}
>>
>> Do you want to add destructors to A/A2 like you did in other tests?
>
> Added. I think this test predates the recent heuristic.
>
> Ok for trunk?
>
> -- >8 --
> Since -Wdangling-reference has false positives that can't be
> prevented, we should offer an easy way to suppress the warning.
> Currently, that is only possible by using a #pragma, either around the
> enclosing class or around the call site. But #pragma GCC diagnostic tend
> to be onerous. A better solution would be to have an attribute.
>
> To that end, this patch adds a new attribute, [[gnu::no_dangling]].
> This attribute takes an optional bool argument to support cases like:
>
> template <typename T>
> struct [[gnu::no_dangling(std::is_reference_v<T>)]] S {
> // ...
> };
>
> PR c++/110358
> PR c++/109642
>
> gcc/cp/ChangeLog:
>
> * call.cc (no_dangling_p): New.
> (reference_like_class_p): Use it.
> (do_warn_dangling_reference): Use it. Don't warn when the function
> or its enclosing class has attribute gnu::no_dangling.
> * tree.cc (cxx_gnu_attributes): Add gnu::no_dangling.
> (handle_no_dangling_attribute): New.
>
> gcc/ChangeLog:
>
> * doc/extend.texi: Document gnu::no_dangling.
> * doc/invoke.texi: Mention that gnu::no_dangling disables
> -Wdangling-reference.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/ext/attr-no-dangling1.C: New test.
> * g++.dg/ext/attr-no-dangling2.C: New test.
> * g++.dg/ext/attr-no-dangling3.C: New test.
> * g++.dg/ext/attr-no-dangling4.C: New test.
> * g++.dg/ext/attr-no-dangling5.C: New test.
> * g++.dg/ext/attr-no-dangling6.C: New test.
> * g++.dg/ext/attr-no-dangling7.C: New test.
> * g++.dg/ext/attr-no-dangling8.C: New test.
> * g++.dg/ext/attr-no-dangling9.C: New test.
> ---
> gcc/cp/call.cc | 38 ++++++++++--
> gcc/cp/tree.cc | 26 ++++++++
> gcc/doc/extend.texi | 47 ++++++++++++++
> gcc/doc/invoke.texi | 6 ++
> gcc/testsuite/g++.dg/ext/attr-no-dangling1.C | 40 ++++++++++++
> gcc/testsuite/g++.dg/ext/attr-no-dangling2.C | 29 +++++++++
> gcc/testsuite/g++.dg/ext/attr-no-dangling3.C | 24 ++++++++
> gcc/testsuite/g++.dg/ext/attr-no-dangling4.C | 14 +++++
> gcc/testsuite/g++.dg/ext/attr-no-dangling5.C | 31 ++++++++++
> gcc/testsuite/g++.dg/ext/attr-no-dangling6.C | 65 ++++++++++++++++++++
> gcc/testsuite/g++.dg/ext/attr-no-dangling7.C | 31 ++++++++++
> gcc/testsuite/g++.dg/ext/attr-no-dangling8.C | 30 +++++++++
> gcc/testsuite/g++.dg/ext/attr-no-dangling9.C | 25 ++++++++
> 13 files changed, 400 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/ext/attr-no-dangling1.C
> create mode 100644 gcc/testsuite/g++.dg/ext/attr-no-dangling2.C
> create mode 100644 gcc/testsuite/g++.dg/ext/attr-no-dangling3.C
> create mode 100644 gcc/testsuite/g++.dg/ext/attr-no-dangling4.C
> create mode 100644 gcc/testsuite/g++.dg/ext/attr-no-dangling5.C
> create mode 100644 gcc/testsuite/g++.dg/ext/attr-no-dangling6.C
> create mode 100644 gcc/testsuite/g++.dg/ext/attr-no-dangling7.C
> create mode 100644 gcc/testsuite/g++.dg/ext/attr-no-dangling8.C
> create mode 100644 gcc/testsuite/g++.dg/ext/attr-no-dangling9.C
>
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index c40ef2e3028..9e4c8073600 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -14033,11 +14033,7 @@ 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. We no
> - longer check that it has a constructor taking the same reference type
> - since that approach still generated too many false positives. */
> +/* Return true if a class T has a reference member. */
>
> static bool
> class_has_reference_member_p (tree t)
> @@ -14061,12 +14057,41 @@ class_has_reference_member_p_r (tree binfo, void *)
> ? integer_one_node : NULL_TREE);
> }
>
> +
> +/* Return true if T (either a class or a function) has been marked as
> + not-dangling. */
> +
> +static bool
> +no_dangling_p (tree t)
> +{
> + t = lookup_attribute ("no_dangling", TYPE_ATTRIBUTES (t));
> + if (!t)
> + return false;
> +
> + t = TREE_VALUE (t);
> + if (!t)
> + return true;
> +
> + t = build_converted_constant_bool_expr (TREE_VALUE (t), tf_warning_or_error);
> + t = cxx_constant_value (t);
> + return t == boolean_true_node;
> +}
> +
> +/* 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. We no
> + longer check that it has a constructor taking the same reference type
> + since that approach still generated too many false positives. */
> +
> static bool
> reference_like_class_p (tree ctype)
> {
> if (!CLASS_TYPE_P (ctype))
> return false;
>
> + if (no_dangling_p (ctype))
> + return true;
> +
> /* Also accept a std::pair<const T&, const T&>. */
> if (std_pair_ref_ref_p (ctype))
> return true;
> @@ -14173,7 +14198,8 @@ do_warn_dangling_reference (tree expr, bool arg_p)
> but probably not to one of its arguments. */
> || (DECL_OBJECT_MEMBER_FUNCTION_P (fndecl)
> && DECL_OVERLOADED_OPERATOR_P (fndecl)
> - && DECL_OVERLOADED_OPERATOR_IS (fndecl, INDIRECT_REF)))
> + && DECL_OVERLOADED_OPERATOR_IS (fndecl, INDIRECT_REF))
> + || no_dangling_p (TREE_TYPE (fndecl)))
> return NULL_TREE;
>
> tree rettype = TREE_TYPE (TREE_TYPE (fndecl));
> diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
> index ad312710f68..e75be9a4e66 100644
> --- a/gcc/cp/tree.cc
> +++ b/gcc/cp/tree.cc
> @@ -47,6 +47,7 @@ static tree verify_stmt_tree_r (tree *, int *, void *);
> static tree handle_init_priority_attribute (tree *, tree, tree, int, bool *);
> static tree handle_abi_tag_attribute (tree *, tree, tree, int, bool *);
> static tree handle_contract_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_no_dangling_attribute (tree *, tree, tree, int, bool *);
>
> /* If REF is an lvalue, returns the kind of lvalue that REF is.
> Otherwise, returns clk_none. */
> @@ -5102,6 +5103,8 @@ static const attribute_spec cxx_gnu_attributes[] =
> handle_init_priority_attribute, NULL },
> { "abi_tag", 1, -1, false, false, false, true,
> handle_abi_tag_attribute, NULL },
> + { "no_dangling", 0, 1, false, true, false, false,
> + handle_no_dangling_attribute, NULL },
> };
>
> const scoped_attribute_specs cxx_gnu_attribute_table =
> @@ -5391,6 +5394,29 @@ handle_contract_attribute (tree *ARG_UNUSED (node), tree ARG_UNUSED (name),
> return NULL_TREE;
> }
>
> +/* Handle a "no_dangling" attribute; arguments as in
> + struct attribute_spec.handler. */
> +
> +tree
> +handle_no_dangling_attribute (tree *node, tree name, tree args, int,
> + bool *no_add_attrs)
> +{
> + if (args && TREE_CODE (TREE_VALUE (args)) == STRING_CST)
> + {
> + error ("%qE attribute argument must be an expression that evaluates "
> + "to true or false", name);
> + *no_add_attrs = true;
> + }
> + else if (!FUNC_OR_METHOD_TYPE_P (*node)
> + && !RECORD_OR_UNION_TYPE_P (*node))
> + {
> + warning (OPT_Wattributes, "%qE attribute ignored", name);
> + *no_add_attrs = true;
> + }
> +
> + return NULL_TREE;
> +}
> +
> /* Return a new PTRMEM_CST of the indicated TYPE. The MEMBER is the
> thing pointed to by the constant. */
>
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 6c2c7ae5d8a..8e1751eae6c 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -29327,6 +29327,53 @@ Some_Class B __attribute__ ((init_priority (543)));
> Note that the particular values of @var{priority} do not matter; only their
> relative ordering.
>
> +@cindex @code{no_dangling} type attribute
> +@cindex @code{no_dangling} function attribute
> +@item no_dangling
> +
> +This attribute can be applied on a class type, function, or member
> +function. Dangling references to classes marked with this attribute
> +will have the @option{-Wdangling-reference} diagnostic suppressed; so
> +will the @code{gnu::no_dangling}-marked functions. For example:
...; so will references returned from...
> +@smallexample
> +class [[gnu::no_dangling]] S @{ @dots{} @};
> +@end smallexample
> +
> +Or:
> +
> +@smallexample
> +class A @{
> + int *p;
> + [[gnu::no_dangling]] int &foo() @{ return *p; @}
> +@};
> +
> +[[gnu::no_dangling]] const int &
> +foo (const int &i)
> +@{
> + @dots{}
> +@}
> +@end smallexample
> +
> +This attribute takes an optional argument, which must be an expression that
> +evaluates to true or false:
> +
> +@smallexample
> +template <typename T>
> +struct [[gnu::no_dangling(std::is_reference_v<T>)]] S @{
> + @dots{}
> +@};
> +@end smallexample
> +
> +Or:
> +
> +@smallexample
> +template <typename T>
> +[[gnu::no_dangling(std::is_reference_v<T>)]] int foo (T& t) @{
I think this function should return a reference.
OK with those changes, thanks.
Jason
next prev parent reply other threads:[~2024-03-01 20:38 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-26 1:37 [PATCH] " Marek Polacek
2024-01-26 2:34 ` Marek Polacek
2024-01-26 21:04 ` Jason Merrill
2024-02-22 0:35 ` [PATCH v2] " Marek Polacek
2024-02-28 23:03 ` Jason Merrill
2024-03-01 0:12 ` [PATCH v3] " Marek Polacek
2024-03-01 0:30 ` Jason Merrill
2024-03-01 17:39 ` [PATCH v4] " Marek Polacek
2024-03-01 18:19 ` Jason Merrill
2024-03-01 19:24 ` [PATCH v5] " Marek Polacek
2024-03-01 20:38 ` Jason Merrill [this message]
2024-03-01 21:23 ` Patrick Palka
2024-03-01 21:31 ` Jason Merrill
2024-03-04 11:00 ` Jonathan Wakely
2024-03-04 16:30 ` 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=584f955c-14eb-40b2-9365-a02279c3a763@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).