[RFC] c++: Minimal handling of carries_dependency attribute

Hi!

A comment in D2552R1:
"The only questionable (but still conforming) case we found was
[[carries_dependency(some_argument)]] on GCC, where the emitted diagnostic said that the
carries_dependency attribute is not supported, but did not specifically call out the syntax error
in the argument clause."
made me try the following patch, where we'll error at least
for arguments to the attribute and for some uses of the attribute
appertaining to something not mentioned in the standard warn
with different diagnostics (or should that be an error?; clang++
does that, but I think we never do for any attribute, standard or not).
The diagnostics on toplevel attribute declaration is still an
attribute ignored warning and on empty statement different wording.

The paper additionally mentions
struct X { [[nodiscard]]; }; // no diagnostic on GCC
and 2 cases of missing diagnostics on [[fallthrough]] (guess I should
file a PR about those; one problem is that do { ... } while (0); there
is replaced during genericization just by ... and another that
[[fallthrough]] there is followed by a label, but not user/case/default
label, but an artificial one created from while loop genericization.

Thoughts on this?

2022-11-08  Jakub Jelinek  <jakub@redhat.com>

	* tree.cc (handle_carries_dependency_attribute): New function.
	(std_attribute_table): Add carries_dependency attribute.
	* parser.cc (cp_parser_check_std_attribute): Add carries_dependency
	attribute.

	* g++.dg/cpp0x/attr-carries_dependency1.C: New test.


	Jakub

On 11/8/22 04:42, Jakub Jelinek wrote:
> Hi!
> 
> A comment in D2552R1:
> "The only questionable (but still conforming) case we found was
> [[carries_dependency(some_argument)]] on GCC, where the emitted diagnostic said that the
> carries_dependency attribute is not supported, but did not specifically call out the syntax error
> in the argument clause."
> made me try the following patch, where we'll error at least
> for arguments to the attribute and for some uses of the attribute
> appertaining to something not mentioned in the standard warn
> with different diagnostics (or should that be an error?; clang++
> does that, but I think we never do for any attribute, standard or not).
> The diagnostics on toplevel attribute declaration is still an
> attribute ignored warning and on empty statement different wording.
> 
> The paper additionally mentions
> struct X { [[nodiscard]]; }; // no diagnostic on GCC
> and 2 cases of missing diagnostics on [[fallthrough]] (guess I should
> file a PR about those; one problem is that do { ... } while (0); there
> is replaced during genericization just by ... and another that
> [[fallthrough]] there is followed by a label, but not user/case/default
> label, but an artificial one created from while loop genericization.
> 
> Thoughts on this?

LGTM.

> 2022-11-08  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tree.cc (handle_carries_dependency_attribute): New function.
> 	(std_attribute_table): Add carries_dependency attribute.
> 	* parser.cc (cp_parser_check_std_attribute): Add carries_dependency
> 	attribute.
> 
> 	* g++.dg/cpp0x/attr-carries_dependency1.C: New test.
> 
> --- gcc/cp/tree.cc.jj	2022-11-07 10:30:42.758629740 +0100
> +++ gcc/cp/tree.cc	2022-11-08 14:45:08.853864684 +0100
> @@ -4923,6 +4923,32 @@ structural_type_p (tree t, bool explain)
>     return true;
>   }
>   
> +/* Partially handle the C++11 [[carries_dependency]] attribute.
> +   Just emit a different diagnostics when it is used on something the
> +   spec doesn't allow vs. where it allows and we just choose to ignore
> +   it.  */
> +
> +static tree
> +handle_carries_dependency_attribute (tree *node, tree name,
> +				     tree ARG_UNUSED (args),
> +				     int ARG_UNUSED (flags),
> +				     bool *no_add_attrs)
> +{
> +  if (TREE_CODE (*node) != FUNCTION_DECL
> +      && TREE_CODE (*node) != PARM_DECL)
> +    {
> +      warning (OPT_Wattributes, "%qE attribute can only be applied to "
> +	       "functions or parameters", name);
> +      *no_add_attrs = true;
> +    }
> +  else
> +    {
> +      warning (OPT_Wattributes, "%qE attribute ignored", name);
> +      *no_add_attrs = true;
> +    }
> +  return NULL_TREE;
> +}
> +
>   /* Handle the C++17 [[nodiscard]] attribute, which is similar to the GNU
>      warn_unused_result attribute.  */
>   
> @@ -5036,6 +5062,8 @@ const struct attribute_spec std_attribut
>       handle_likeliness_attribute, attr_cold_hot_exclusions },
>     { "noreturn", 0, 0, true, false, false, false,
>       handle_noreturn_attribute, attr_noreturn_exclusions },
> +  { "carries_dependency", 0, 0, true, false, false, false,
> +    handle_carries_dependency_attribute, NULL },
>     { NULL, 0, 0, false, false, false, false, NULL, NULL }
>   };
>   
> --- gcc/cp/parser.cc.jj	2022-11-04 18:11:41.523945997 +0100
> +++ gcc/cp/parser.cc	2022-11-08 13:41:35.075135139 +0100
> @@ -29239,8 +29239,7 @@ cp_parser_std_attribute (cp_parser *pars
>   
>   /* Warn if the attribute ATTRIBUTE appears more than once in the
>      attribute-list ATTRIBUTES.  This used to be enforced for certain
> -   attributes, but the restriction was removed in P2156.  Note that
> -   carries_dependency ([dcl.attr.depend]) isn't implemented yet in GCC.
> +   attributes, but the restriction was removed in P2156.
>      LOC is the location of ATTRIBUTE.  Returns true if ATTRIBUTE was not
>      found in ATTRIBUTES.  */
>   
> @@ -29249,7 +29248,7 @@ cp_parser_check_std_attribute (location_
>   {
>     static auto alist = { "noreturn", "deprecated", "nodiscard", "maybe_unused",
>   			"likely", "unlikely", "fallthrough",
> -			"no_unique_address" };
> +			"no_unique_address", "carries_dependency" };
>     if (attributes)
>       for (const auto &a : alist)
>         if (is_attribute_p (a, get_attribute_name (attribute))
> --- gcc/testsuite/g++.dg/cpp0x/attr-carries_dependency1.C.jj	2022-11-08 15:17:43.168238390 +0100
> +++ gcc/testsuite/g++.dg/cpp0x/attr-carries_dependency1.C	2022-11-08 15:16:39.695104787 +0100
> @@ -0,0 +1,17 @@
> +// { dg-do compile { target c++11 } }
> +
> +[[carries_dependency]] int *f1 ();		// { dg-warning "attribute ignored" }
> +int f2 (int *x [[carries_dependency]]);		// { dg-warning "attribute ignored" }
> +[[carries_dependency]] int f3 ();		// { dg-warning "attribute ignored" }
> +int f4 (int x [[carries_dependency]]);		// { dg-warning "attribute ignored" }
> +[[carries_dependency(1)]] int f5 ();		// { dg-error "'carries_dependency' attribute does not take any arguments" }
> +[[carries_dependency]] int v;			// { dg-warning "'carries_dependency' attribute can only be applied to functions or parameters" }
> +[[carries_dependency]];				// { dg-warning "attribute ignored" }
> +void
> +f6 ()
> +{
> +  [[carries_dependency]];			// { dg-warning "attributes at the beginning of statement are ignored" }
> +}
> +#if __has_cpp_attribute(carries_dependency)
> +#error carries_dependency attribute is not actually implemented
> +#endif
> 
> 	Jakub
>

On Tue, Nov 08, 2022 at 01:40:03PM -1000, Jason Merrill wrote:
> > A comment in D2552R1:
> > "The only questionable (but still conforming) case we found was
> > [[carries_dependency(some_argument)]] on GCC, where the emitted diagnostic said that the
> > carries_dependency attribute is not supported, but did not specifically call out the syntax error
> > in the argument clause."
> > made me try the following patch, where we'll error at least
> > for arguments to the attribute and for some uses of the attribute
> > appertaining to something not mentioned in the standard warn
> > with different diagnostics (or should that be an error?; clang++
> > does that, but I think we never do for any attribute, standard or not).
> > The diagnostics on toplevel attribute declaration is still an
> > attribute ignored warning and on empty statement different wording.
> > 
> > The paper additionally mentions
> > struct X { [[nodiscard]]; }; // no diagnostic on GCC
> > and 2 cases of missing diagnostics on [[fallthrough]] (guess I should
> > file a PR about those; one problem is that do { ... } while (0); there
> > is replaced during genericization just by ... and another that
> > [[fallthrough]] there is followed by a label, but not user/case/default
> > label, but an artificial one created from while loop genericization.
> > 
> > Thoughts on this?
> 
> LGTM.

Thanks, committed now.
Given CWG2538, I wonder whether we shouldn't at least pedwarn rather than
warning{,_at} for standard attributes that appertain to wrong entities
(and keep warning{,_at} for non-standard attributes including gnu variants
of standard attributes).
If yes, we'd need to differentiate between the standard attributes
and gnu variants thereof (I think the C FE does, but C++ FE has
      /* We used to treat C++11 noreturn attribute as equivalent to GNU's,
         but no longer: we have to be able to tell [[noreturn]] and
         __attribute__((noreturn)) apart.  */
      /* C++14 deprecated attribute is equivalent to GNU's.  */
      if (is_attribute_p ("deprecated", attr_id))
        TREE_PURPOSE (TREE_PURPOSE (attribute)) = gnu_identifier;
      /* C++17 fallthrough attribute is equivalent to GNU's.  */
      else if (is_attribute_p ("fallthrough", attr_id))
        TREE_PURPOSE (TREE_PURPOSE (attribute)) = gnu_identifier;
      /* C++23 assume attribute is equivalent to GNU's.  */
      else if (is_attribute_p ("assume", attr_id))
        TREE_PURPOSE (TREE_PURPOSE (attribute)) = gnu_identifier;
so we'd need to remove that and make sure those standard attributes
are handled.

	Jakub

On 11/9/22 02:18, Jakub Jelinek wrote:
> On Tue, Nov 08, 2022 at 01:40:03PM -1000, Jason Merrill wrote:
>>> A comment in D2552R1:
>>> "The only questionable (but still conforming) case we found was
>>> [[carries_dependency(some_argument)]] on GCC, where the emitted diagnostic said that the
>>> carries_dependency attribute is not supported, but did not specifically call out the syntax error
>>> in the argument clause."
>>> made me try the following patch, where we'll error at least
>>> for arguments to the attribute and for some uses of the attribute
>>> appertaining to something not mentioned in the standard warn
>>> with different diagnostics (or should that be an error?; clang++
>>> does that, but I think we never do for any attribute, standard or not).
>>> The diagnostics on toplevel attribute declaration is still an
>>> attribute ignored warning and on empty statement different wording.
>>>
>>> The paper additionally mentions
>>> struct X { [[nodiscard]]; }; // no diagnostic on GCC
>>> and 2 cases of missing diagnostics on [[fallthrough]] (guess I should
>>> file a PR about those; one problem is that do { ... } while (0); there
>>> is replaced during genericization just by ... and another that
>>> [[fallthrough]] there is followed by a label, but not user/case/default
>>> label, but an artificial one created from while loop genericization.
>>>
>>> Thoughts on this?
>>
>> LGTM.
> 
> Thanks, committed now.
> Given CWG2538, I wonder whether we shouldn't at least pedwarn rather than
> warning{,_at} for standard attributes that appertain to wrong entities
> (and keep warning{,_at} for non-standard attributes including gnu variants
> of standard attributes).

I don't think that's necessary, but it might be better.  And I guess we 
should separate the warnings for unrecognized attributes vs. misused 
attributes.

> If yes, we'd need to differentiate between the standard attributes
> and gnu variants thereof (I think the C FE does, but C++ FE has
>        /* We used to treat C++11 noreturn attribute as equivalent to GNU's,
>           but no longer: we have to be able to tell [[noreturn]] and
>           __attribute__((noreturn)) apart.  */
>        /* C++14 deprecated attribute is equivalent to GNU's.  */
>        if (is_attribute_p ("deprecated", attr_id))
>          TREE_PURPOSE (TREE_PURPOSE (attribute)) = gnu_identifier;
>        /* C++17 fallthrough attribute is equivalent to GNU's.  */
>        else if (is_attribute_p ("fallthrough", attr_id))
>          TREE_PURPOSE (TREE_PURPOSE (attribute)) = gnu_identifier;
>        /* C++23 assume attribute is equivalent to GNU's.  */
>        else if (is_attribute_p ("assume", attr_id))
>          TREE_PURPOSE (TREE_PURPOSE (attribute)) = gnu_identifier;
> so we'd need to remove that and make sure those standard attributes
> are handled.
> 
> 	Jakub
>