From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 78E5A3858D39 for ; Wed, 28 Feb 2024 23:03:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 78E5A3858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 78E5A3858D39 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709161443; cv=none; b=jO5bWI98UIrXP0Hvqea7eVhWgs1BfJBDF6cTQhI/NtGBKVMcmB/CAB2A7IObdl4jWIV5RQfAwJ6uC9KkkaIgeDe/VXdcNkCbdUWvaoM/lo7nQLsfk7V2KYMs4dWOgIzQP5r9cWvAic3l9cFvQds+lY62o2tr60yxApUNkTcdz08= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1709161443; c=relaxed/simple; bh=2vXjH3M4Zlkh7a5itp3c/wwNWqQoNjf3D2gHrWWhMLU=; h=DKIM-Signature:Message-ID:Date:MIME-Version:From:Subject:To; b=UEKXafNSKoPL7vui+aPwWryBAOwA5VHtu2QZDRnytd9oj3fjmr3AjqhKEygq7QauKxtMaho7fU5BafJlcBNmO9pZSdwLgdgzxGiTuPj5OMDQbJ/rXfs8Ftv/XnTOwCRwNuYgyy3SqvtZXKDgyLBt3k5hdp0U/daqGSMpPpf7fac= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709161439; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sUXFyOoDGXy0eA6Xxz2WteupQJVKLunlNITVWKOGRek=; b=HSts4pRrLfMZ8BRViHlTD6bU+jBbSDGDwi9miI9hSNp7lyXEsAhMI+ArQ7IFrpPGOz7V5v eI4NJdEp3d9uo1fykZLcPHennvoDAbBrOJp4tMoxxlnwnkEcOOO2DsNxsUKEdgzh59pHGK 3ziyoiXOxJtm6/joPK3LcMrgXISUDVc= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-81-0EGgXZ26Mrq1HpBb-X4RxQ-1; Wed, 28 Feb 2024 18:03:57 -0500 X-MC-Unique: 0EGgXZ26Mrq1HpBb-X4RxQ-1 Received: by mail-qk1-f198.google.com with SMTP id af79cd13be357-787b8dd3330so232874685a.1 for ; Wed, 28 Feb 2024 15:03:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709161436; x=1709766236; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=sUXFyOoDGXy0eA6Xxz2WteupQJVKLunlNITVWKOGRek=; b=H2CwS52zfvfY570TbSgXhG7EHfCTKietf9mHo6UUsRKKkbzg1NM8HghLldvwiEBSWW CQIIbTomcUd3yTExVB0cgfJgbJi5GFArPUkc67yfY8t+MbQDwa4WKgrA+lCEbwuwl6aE iZCTiWRxl/G9vWqbLHRVLz+IfKbYvpbw4Em6htMtHbR6rCWBXBV5Ec0IdIVXn9JsEjB1 KQ8m8bYAQRIBJbH3xKVkqiuaQfSHTMuATTIl7jJSPdDrRB2yZmtI6Rsed3xyT/WsH1EV 5dyif8AD6xea+1h+0wDqIY7QTDgHrhyawlXDhYmtK7ci8Hv23a3N2F48w0i6+D2d01r0 50fw== X-Gm-Message-State: AOJu0YwPo6wT+HhWaMznZtT+Poj0IEObXuYT3PUtlQRxpVLHNVy8Gohe 8GeTeTOgW8iJTajcOi05SeNl4XFBf/KM1mW5teRrKLcpi44wUcjuUEx2e+FN682L1ZupTR46fZM z7w8FwAwg5HwR0CzuQ5+Lkt5v/D6weHrJu7ZT1c40BsZuayD5maAlybY5l1JmgiU= X-Received: by 2002:a05:620a:1922:b0:787:a9af:d6b8 with SMTP id bj34-20020a05620a192200b00787a9afd6b8mr336973qkb.8.1709161436483; Wed, 28 Feb 2024 15:03:56 -0800 (PST) X-Google-Smtp-Source: AGHT+IFeXqnU+iKbH+YiULX3bJmJB3yM5vLOWLA7KnG0MIQIrGcFqC2rdFJQFWw2ggblsHA66+U1gQ== X-Received: by 2002:a05:620a:1922:b0:787:a9af:d6b8 with SMTP id bj34-20020a05620a192200b00787a9afd6b8mr336941qkb.8.1709161436086; Wed, 28 Feb 2024 15:03:56 -0800 (PST) Received: from [192.168.1.130] (130-44-146-16.s12558.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.146.16]) by smtp.gmail.com with ESMTPSA id cx16-20020a05620a51d000b00787a1c74595sm82783qkb.105.2024.02.28.15.03.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 28 Feb 2024 15:03:55 -0800 (PST) Message-ID: Date: Wed, 28 Feb 2024 18:03:54 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Jason Merrill Subject: Re: [PATCH v2] c++: implement [[gnu::non_owning]] [PR110358] To: Marek Polacek Cc: GCC Patches References: <20240126013736.70125-1-polacek@redhat.com> <0e4c47b6-604c-4d30-b458-825959c0e1d6@redhat.com> In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2/21/24 19:35, Marek Polacek wrote: > On Fri, Jan 26, 2024 at 04:04:35PM -0500, Jason Merrill wrote: >> On 1/25/24 20:37, Marek Polacek wrote: >>> 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. Such >>> an attribute should not be tied to this particular warning though. [*] >>> >>> The warning bogusly triggers for classes that are like std::span, >>> std::reference_wrapper, and std::ranges::ref_view. The common property >>> seems to be that these classes are only wrappers around some data. So >>> I chose the name non_owning, but I'm not attached to it. I hope that >>> in the future the attribute can be used for something other than this >>> diagnostic. >> >> You decided not to pursue Barry's request for a bool argument to the >> attribute? > > At first I thought it'd be an unnecessary complication but it was actually > pretty easy. Better to accept the optional argument from the get-go > otherwise people would have to add > GCC 14 checks. > >> Might it be more useful for the attribute to make reference_like_class_p >> return true, so that we still warn about a temporary of another type passing >> through it? > > Good point. Fixed. > > 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. Such > an attribute should not be tied to this particular warning though. > > The warning bogusly triggers for classes that are like std::span, > std::reference_wrapper, and std::ranges::ref_view. The common property > seems to be that these classes are only wrappers around some data. So > I chose the name non_owning, but I'm not attached to it. I hope that > in the future the attribute can be used for something other than this > diagnostic. > > This attribute takes an optional bool argument to support cases like: > > template > struct [[gnu::non_owning(std::is_reference_v)]] S { > // ... > }; > > PR c++/110358 > PR c++/109642 > > gcc/cp/ChangeLog: > > * call.cc (non_owning_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::non_owning. > * tree.cc (cxx_gnu_attributes): Add gnu::non_owning. > (handle_non_owning_attribute): New. > > gcc/ChangeLog: > > * doc/extend.texi: Document gnu::non_owning. > * doc/invoke.texi: Mention that gnu::non_owning disables > -Wdangling-reference. > > gcc/testsuite/ChangeLog: > > * g++.dg/ext/attr-non-owning1.C: New test. > * g++.dg/ext/attr-non-owning2.C: New test. > * g++.dg/ext/attr-non-owning3.C: New test. > * g++.dg/ext/attr-non-owning4.C: New test. > * g++.dg/ext/attr-non-owning5.C: New test. > * g++.dg/ext/attr-non-owning6.C: New test. > * g++.dg/ext/attr-non-owning7.C: New test. > * g++.dg/ext/attr-non-owning8.C: New test. > * g++.dg/ext/attr-non-owning9.C: New test. > --- > gcc/cp/call.cc | 38 ++++++++++-- > gcc/cp/tree.cc | 26 +++++++++ > gcc/doc/extend.texi | 25 ++++++++ > gcc/doc/invoke.texi | 21 +++++++ > gcc/testsuite/g++.dg/ext/attr-non-owning1.C | 38 ++++++++++++ > gcc/testsuite/g++.dg/ext/attr-non-owning2.C | 29 +++++++++ > gcc/testsuite/g++.dg/ext/attr-non-owning3.C | 24 ++++++++ > gcc/testsuite/g++.dg/ext/attr-non-owning4.C | 14 +++++ > gcc/testsuite/g++.dg/ext/attr-non-owning5.C | 31 ++++++++++ > gcc/testsuite/g++.dg/ext/attr-non-owning6.C | 65 +++++++++++++++++++++ > gcc/testsuite/g++.dg/ext/attr-non-owning7.C | 31 ++++++++++ > gcc/testsuite/g++.dg/ext/attr-non-owning8.C | 30 ++++++++++ > gcc/testsuite/g++.dg/ext/attr-non-owning9.C | 25 ++++++++ > 13 files changed, 391 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ext/attr-non-owning1.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-non-owning2.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-non-owning3.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-non-owning4.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-non-owning5.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-non-owning6.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-non-owning7.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-non-owning8.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-non-owning9.C > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > index 1dac1470d3b..e4bf9c963bd 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 > + non-owning. */ > + > +static bool > +non_owning_p (tree t) > +{ > + t = lookup_attribute ("non_owning", 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 (non_owning_p (ctype)) > + return true; > + > /* Also accept a std::pair. */ > 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)) > + || non_owning_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..017da7e294a 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_non_owning_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 }, > + { "non_owning", 0, 1, false, true, false, false, > + handle_non_owning_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 "non_owning" attribute; arguments as in > + struct attribute_spec.handler. */ > + > +tree > +handle_non_owning_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 2135dfde9c8..9132add8267 100644 > --- a/gcc/doc/extend.texi > +++ b/gcc/doc/extend.texi > @@ -29320,6 +29320,31 @@ Some_Class B __attribute__ ((init_priority (543))); > Note that the particular values of @var{priority} do not matter; only their > relative ordering. > > +@cindex @code{non_owning} type attribute > +@item non_owning > + > +This attribute can be applied on a class type, function, or member > +function and indicates that it does not own its associated data. For > +example, classes like @code{std::span} or @code{std::reference_wrapper} > +are considered non-owning. > + > +@smallexample > +class [[gnu::non_owning]] S @{ @dots{} @}; > +@end smallexample > + > +This attribute takes an optional argument, which must be an expression that > +evaluates to true or false: > + > +@smallexample > +template > +struct [[gnu::non_owning(std::is_reference_v)]] S @{ > + @dots{} > +@}; > +@end smallexample > + > +Currently, the only effect this attribute has is to suppress the > +@option{-Wdangling-reference} diagnostic. > + > @cindex @code{warn_unused} type attribute > @item warn_unused > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index b4e4ee9fb81..0df2e2ded34 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::non_owning} attribute, > +which can be applied on the enclosing class type (in which case it disables > +the warning for all its member functions), member function, or a regular > +function. For example: 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. 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? Jason