Re: [PATCH v3] c++: implement [[gnu::non_owning]] [PR110358]

[Jason Merrill <jason@redhat.com>]

On 2/29/24 19:12, Marek Polacek wrote:
> On Wed, Feb 28, 2024 at 06:03:54PM -0500, Jason Merrill wrote:
>
>> Hmm, if we're also going to allow the attribute to be applied to a function,
>> the name doesn't make so much sense.  For a class, it says that the class
>> refers to its initializer; for a function, it says that the function return
>> value *doesn't* refer to its argument.
> 
> Yeah, that's a fair point; I guess "non_owning" would be too perplexing.
> 
>> If we want something that can apply to both classes and functions, we're
>> probably back to an attribute that just suppresses the warning, with a
>> different name.
>>
>> Or I guess we could have two attributes, but that seems like a lot.
>>
>> WDYT?
> 
> I think we don't want two separate attributes, and we do want that one
> attribute to apply to both fns and classes.  We could implement something
> like
> 
>    [[gnu::no_warning("Wdangling-reference")]]
>    [[gnu::no_warning("Wdangling-reference", bool)]]
> 
> but first, that's a lot of typing, second, it would be confusing because
> it wouldn't work for any other warning.  We already have [[unused]] and
> [[maybe_unused]] whose effect is to suppress a warning.  It think our
> best bet is to do the most straightforward thing: [[gnu::no_dangling]],
> which this patch implements.  I didn't call it no_dangling_reference in
> the hope that it can, some day, be also used for some -Wdangling-pointer
> purposes.
> 
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, 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                          | 21 +++++++
>   gcc/doc/invoke.texi                          | 21 +++++++
>   gcc/testsuite/g++.dg/ext/attr-no-dangling1.C | 38 ++++++++++++
>   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, 387 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 efd78014d1a..571655bf39a 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -29317,6 +29317,27 @@ 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
> +@item no_dangling
> +
> +This attribute can be applied on a class type, function, or member
> +function.  Entities marked with this attribute will have the
> +@option{-Wdangling-reference} diagnostic suppressed.
> +
> +@smallexample
> +class [[gnu::no_dangling]] S @{ @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 >
>   @cindex @code{warn_unused} type attribute
>   @item warn_unused
>   
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 7862c751801..9022a413dd4 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -3908,6 +3908,9 @@ const T& foo (const T&) @{ @dots{} @}
>   #pragma GCC diagnostic pop
>   @end smallexample
>   
> +The @code{#pragma} can also surround the class; in that case, the warning
> +will be disabled for all the member functions.
> +
>   @option{-Wdangling-reference} also warns about code like
>   
>   @smallexample
> @@ -3932,6 +3935,24 @@ 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,

Can this be an xref, and put all the documentation for the usage and 
effect of the attribute in the attribute section?

> +which can be applied on the enclosing class type (in which case it disables
> +the warning for all its member functions),

I think this misrepresents the effect of putting the attribute on the 
class.  Rather, it means that we won't warn about a dangling reference 
to the class.

> member function, or a regular
> +function.  For example:

Jason