diff mbox series

attribs: Fix and refactor diag_attr_exclusions

Message ID 314be4f3-09a2-f00e-f700-adefa89ccade@e124511.cambridge.arm.com
State New
Headers show
Series attribs: Fix and refactor diag_attr_exclusions | expand

Commit Message

Andrew Carlotti May 16, 2024, 4:18 p.m. UTC 
The existing implementation of this function was convoluted, and had
multiple control flow errors that became apparent to me while reading
the code:

1. The initial early return only checked the properties of the first
exclusion in the list, when these properties could be different for
subsequent exclusions.

2. excl was not reset within the outer loop, so the inner loop body
would only execute during the first iteration of the outer loop.  This
effectively meant that the value of attrs[1] was ignored.

3. The function called itself recursively twice, with both last_decl and
TREE_TYPE (last_decl) as parameters. The second recursive call should
have been redundant, since attrs[1] = TREE_TYPE (last_decl) during the
first recursive call.

This patch eliminated the early return, and combines the checks with
those present within the inner loop.  It also fixes the inner loop
initialisation, and modifies the outer loop to iterate over nodes
instead of their attributes. This latter change allows the recursion to
be eliminated, by extending the new nodes array to include last_decl
(and its type) as well.

This patch provides an alternative fix for PR114634, although I wasn't
aware of that issue until rebasing on top of Jakub's fix.

I am not aware of any other compiler bugs resulting from these issues.
However, if the exclusions for target_clones were listed in the opposite
order, then it would have broken detection of the always_inline
exclusion on aarch64 (where TARGET_HAS_FMV_TARGET_ATTRIBUTE is false).

Is this ok for master?

gcc/ChangeLog:

	* attribs.cc (diag_attr_exclusions): Fix and refactor.

Comments

Richard Sandiford May 28, 2024, 4:39 p.m. UTC | #1 
Andrew Carlotti <andrew.carlotti@arm.com> writes:
> The existing implementation of this function was convoluted, and had
> multiple control flow errors that became apparent to me while reading
> the code:
>
> 1. The initial early return only checked the properties of the first
> exclusion in the list, when these properties could be different for
> subsequent exclusions.
>
> 2. excl was not reset within the outer loop, so the inner loop body
> would only execute during the first iteration of the outer loop.  This
> effectively meant that the value of attrs[1] was ignored.
>
> 3. The function called itself recursively twice, with both last_decl and
> TREE_TYPE (last_decl) as parameters. The second recursive call should
> have been redundant, since attrs[1] = TREE_TYPE (last_decl) during the
> first recursive call.

Thanks for doing this.  Agree with the above.

> This patch eliminated the early return, and combines the checks with
> those present within the inner loop.  It also fixes the inner loop
> initialisation, and modifies the outer loop to iterate over nodes
> instead of their attributes. This latter change allows the recursion to
> be eliminated, by extending the new nodes array to include last_decl
> (and its type) as well.
>
> This patch provides an alternative fix for PR114634, although I wasn't
> aware of that issue until rebasing on top of Jakub's fix.
>
> I am not aware of any other compiler bugs resulting from these issues.
> However, if the exclusions for target_clones were listed in the opposite
> order, then it would have broken detection of the always_inline
> exclusion on aarch64 (where TARGET_HAS_FMV_TARGET_ATTRIBUTE is false).
>
> Is this ok for master?
>
> gcc/ChangeLog:
>
> 	* attribs.cc (diag_attr_exclusions): Fix and refactor.
>
>
> diff --git a/gcc/attribs.cc b/gcc/attribs.cc
> index 3ab0b0fd87a4404a593b2de365ea5226e31fe24a..431dd4255e68e92dd8d10bbb21ea079e50811faa 100644
> --- a/gcc/attribs.cc
> +++ b/gcc/attribs.cc
> @@ -433,84 +433,69 @@ get_attribute_namespace (const_tree attr)
>     or a TYPE.  */
>  
>  static bool
> -diag_attr_exclusions (tree last_decl, tree node, tree attrname,
> +diag_attr_exclusions (tree last_decl, tree base_node, tree attrname,
>  		      const attribute_spec *spec)
>  {
> -  const attribute_spec::exclusions *excl = spec->exclude;
>  
> -  tree_code code = TREE_CODE (node);
> +  /* BASE_NODE is either the current decl to which the attribute is being
> +     applied, or its type.  For the former, consider the attributes on both the
> +     decl and its type.  Check both LAST_DECL and its type as well.  */
>  
> -  if ((code == FUNCTION_DECL && !excl->function
> -       && (!excl->type || !spec->affects_type_identity))
> -      || (code == VAR_DECL && !excl->variable
> -	  && (!excl->type || !spec->affects_type_identity))
> -      || (((code == TYPE_DECL || RECORD_OR_UNION_TYPE_P (node)) && !excl->type)))
> -    return false;
> +  tree nodes[4] = { NULL_TREE, NULL_TREE, NULL_TREE, NULL_TREE };
>  
> -  /* True if an attribute that's mutually exclusive with ATTRNAME
> -     has been found.  */
> -  bool found = false;
> +  nodes[0] = base_node;
> +  if (DECL_P (base_node))
> +      nodes[1] = (TREE_TYPE (base_node));

Nit: too much indentation.

> -  if (last_decl && last_decl != node && TREE_TYPE (last_decl) != node)
> +  if (last_decl)
>      {
> -      /* Check both the last DECL and its type for conflicts with
> -	 the attribute being added to the current decl or type.  */
> -      found |= diag_attr_exclusions (last_decl, last_decl, attrname, spec);
> -      tree decl_type = TREE_TYPE (last_decl);
> -      found |= diag_attr_exclusions (last_decl, decl_type, attrname, spec);
> +      nodes[2] = last_decl;
> +      if (DECL_P (last_decl))
> +	  nodes[3] = TREE_TYPE (last_decl);
>      }
>  
> -  /* NODE is either the current DECL to which the attribute is being
> -     applied or its TYPE.  For the former, consider the attributes on
> -     both the DECL and its type.  */
> -  tree attrs[2];
> -
> -  if (DECL_P (node))
> -    {
> -      attrs[0] = DECL_ATTRIBUTES (node);
> -      if (TREE_TYPE (node))
> -	attrs[1] = TYPE_ATTRIBUTES (TREE_TYPE (node));
> -      else
> -	/* TREE_TYPE can be NULL e.g. while processing attributes on
> -	   enumerators.  */
> -	attrs[1] = NULL_TREE;
> -    }
> -  else
> -    {
> -      attrs[0] = TYPE_ATTRIBUTES (node);
> -      attrs[1] = NULL_TREE;
> -    }
> +  /* True if an attribute that's mutually exclusive with ATTRNAME
> +     has been found.  */
> +  bool found = false;
>  
>    /* Iterate over the mutually exclusive attribute names and verify
>       that the symbol doesn't contain it.  */
> -  for (unsigned i = 0; i != ARRAY_SIZE (attrs); ++i)
> +  for (unsigned i = 0; i != ARRAY_SIZE (nodes); ++i)
>      {
> -      if (!attrs[i])
> +      tree node = nodes[i];
> +
> +      if (!node)
>  	continue;
>  
> -      for ( ; excl->name; ++excl)
> +      tree attr;
> +      if DECL_P (node)

Missing parens.  (Works because DECL_P is a macro. :))

> +	attr = DECL_ATTRIBUTES (node);
> +      else
> +	attr = TYPE_ATTRIBUTES (node);
> +
> +      tree_code code = TREE_CODE (node);
> +
> +      for (auto excl = spec->exclude; excl->name; ++excl)
>  	{
>  	  /* Avoid checking the attribute against itself.  */
>  	  if (is_attribute_p (excl->name, attrname))
>  	    continue;
>  
> -	  if (!lookup_attribute (excl->name, attrs[i]))
> +	  if (!lookup_attribute (excl->name, attr))
>  	    continue;
>  
>  	  /* An exclusion may apply either to a function declaration,
>  	     type declaration, or a field/variable declaration, or
>  	     any subset of the three.  */
> -	  if (TREE_CODE (node) == FUNCTION_DECL
> -	      && !excl->function)
> +	  if (code == FUNCTION_DECL && !excl->function
> +	       && (!excl->type || !spec->affects_type_identity))
>  	    continue;

I think the original instance of this check was saying "if NODE is a
function decl, we need to check for function exclusions against NODE
and for type exclusions against NODE's type".  By doing the check
in the inner loop, the gating on:

  (!excl->type || !spec->affects_type_identity)

doesn't have the same effect.  The function type will be processed
by a different iteration of the outer loop and the exclusion will
be checked regardless of spec->affects_type_identity (or, indeed,
excl->type, although that's pre-existing).  The same thing applies...

>  
> -	  if (TREE_CODE (node) == TYPE_DECL
> -	      && !excl->type)
> +	  if ((code == VAR_DECL || code == FIELD_DECL) && !excl->variable
> +	       && (!excl->type || !spec->affects_type_identity))
>  	    continue;

...here.  

Maybe a more conservative alternative would be to switch the loops
so that the outer one iterates over the exclusions.  The current:

  if ((code == FUNCTION_DECL && !excl->function
       && (!excl->type || !spec->affects_type_identity))
      || (code == VAR_DECL && !excl->variable
	  && (!excl->type || !spec->affects_type_identity))
      || (((code == TYPE_DECL || RECORD_OR_UNION_TYPE_P (node)) && !excl->type)))
    return false;

  [...]

  /* NODE is either the current DECL to which the attribute is being
     applied or its TYPE.  For the former, consider the attributes on
     both the DECL and its type.  */
  tree attrs[2];

  if (DECL_P (node))
    {
      attrs[0] = DECL_ATTRIBUTES (node);
      if (TREE_TYPE (node))
	attrs[1] = TYPE_ATTRIBUTES (TREE_TYPE (node));
      else
	/* TREE_TYPE can be NULL e.g. while processing attributes on
	   enumerators.  */
	attrs[1] = NULL_TREE;
    }
  else
    {
      attrs[0] = TYPE_ATTRIBUTES (node);
      attrs[1] = NULL_TREE;
    }

could then be done in that loop (continuing rather than returning false).

The recursion would then stay, but like you say, would be restricted to
one call rather than two.

WDYT?

Richard

>  
> -	  if ((TREE_CODE (node) == FIELD_DECL
> -	       || VAR_P (node))
> -	      && !excl->variable)
> +	  if (((code == TYPE_DECL || RECORD_OR_UNION_TYPE_P (node)) && !excl->type))
>  	    continue;
>  
>  	  found = true;
diff mbox series

Patch

diff --git a/gcc/attribs.cc b/gcc/attribs.cc
index 3ab0b0fd87a4404a593b2de365ea5226e31fe24a..431dd4255e68e92dd8d10bbb21ea079e50811faa 100644
--- a/gcc/attribs.cc
+++ b/gcc/attribs.cc
@@ -433,84 +433,69 @@  get_attribute_namespace (const_tree attr)
    or a TYPE.  */
 
 static bool
-diag_attr_exclusions (tree last_decl, tree node, tree attrname,
+diag_attr_exclusions (tree last_decl, tree base_node, tree attrname,
 		      const attribute_spec *spec)
 {
-  const attribute_spec::exclusions *excl = spec->exclude;
 
-  tree_code code = TREE_CODE (node);
+  /* BASE_NODE is either the current decl to which the attribute is being
+     applied, or its type.  For the former, consider the attributes on both the
+     decl and its type.  Check both LAST_DECL and its type as well.  */
 
-  if ((code == FUNCTION_DECL && !excl->function
-       && (!excl->type || !spec->affects_type_identity))
-      || (code == VAR_DECL && !excl->variable
-	  && (!excl->type || !spec->affects_type_identity))
-      || (((code == TYPE_DECL || RECORD_OR_UNION_TYPE_P (node)) && !excl->type)))
-    return false;
+  tree nodes[4] = { NULL_TREE, NULL_TREE, NULL_TREE, NULL_TREE };
 
-  /* True if an attribute that's mutually exclusive with ATTRNAME
-     has been found.  */
-  bool found = false;
+  nodes[0] = base_node;
+  if (DECL_P (base_node))
+      nodes[1] = (TREE_TYPE (base_node));
 
-  if (last_decl && last_decl != node && TREE_TYPE (last_decl) != node)
+  if (last_decl)
     {
-      /* Check both the last DECL and its type for conflicts with
-	 the attribute being added to the current decl or type.  */
-      found |= diag_attr_exclusions (last_decl, last_decl, attrname, spec);
-      tree decl_type = TREE_TYPE (last_decl);
-      found |= diag_attr_exclusions (last_decl, decl_type, attrname, spec);
+      nodes[2] = last_decl;
+      if (DECL_P (last_decl))
+	  nodes[3] = TREE_TYPE (last_decl);
     }
 
-  /* NODE is either the current DECL to which the attribute is being
-     applied or its TYPE.  For the former, consider the attributes on
-     both the DECL and its type.  */
-  tree attrs[2];
-
-  if (DECL_P (node))
-    {
-      attrs[0] = DECL_ATTRIBUTES (node);
-      if (TREE_TYPE (node))
-	attrs[1] = TYPE_ATTRIBUTES (TREE_TYPE (node));
-      else
-	/* TREE_TYPE can be NULL e.g. while processing attributes on
-	   enumerators.  */
-	attrs[1] = NULL_TREE;
-    }
-  else
-    {
-      attrs[0] = TYPE_ATTRIBUTES (node);
-      attrs[1] = NULL_TREE;
-    }
+  /* True if an attribute that's mutually exclusive with ATTRNAME
+     has been found.  */
+  bool found = false;
 
   /* Iterate over the mutually exclusive attribute names and verify
      that the symbol doesn't contain it.  */
-  for (unsigned i = 0; i != ARRAY_SIZE (attrs); ++i)
+  for (unsigned i = 0; i != ARRAY_SIZE (nodes); ++i)
     {
-      if (!attrs[i])
+      tree node = nodes[i];
+
+      if (!node)
 	continue;
 
-      for ( ; excl->name; ++excl)
+      tree attr;
+      if DECL_P (node)
+	attr = DECL_ATTRIBUTES (node);
+      else
+	attr = TYPE_ATTRIBUTES (node);
+
+      tree_code code = TREE_CODE (node);
+
+      for (auto excl = spec->exclude; excl->name; ++excl)
 	{
 	  /* Avoid checking the attribute against itself.  */
 	  if (is_attribute_p (excl->name, attrname))
 	    continue;
 
-	  if (!lookup_attribute (excl->name, attrs[i]))
+	  if (!lookup_attribute (excl->name, attr))
 	    continue;
 
 	  /* An exclusion may apply either to a function declaration,
 	     type declaration, or a field/variable declaration, or
 	     any subset of the three.  */
-	  if (TREE_CODE (node) == FUNCTION_DECL
-	      && !excl->function)
+	  if (code == FUNCTION_DECL && !excl->function
+	       && (!excl->type || !spec->affects_type_identity))
 	    continue;
 
-	  if (TREE_CODE (node) == TYPE_DECL
-	      && !excl->type)
+	  if ((code == VAR_DECL || code == FIELD_DECL) && !excl->variable
+	       && (!excl->type || !spec->affects_type_identity))
 	    continue;
 
-	  if ((TREE_CODE (node) == FIELD_DECL
-	       || VAR_P (node))
-	      && !excl->variable)
+	  if (((code == TYPE_DECL || RECORD_OR_UNION_TYPE_P (node)) && !excl->type))
 	    continue;
 
 	  found = true;