This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 5/3] C++ bits to improve detection of attribute conflicts (PR 81544)


Ping: https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01034.html

The attached C++ only patch is rebased on the top of trunk.

The remaining patches in the series (C FE and the back ends)
have been approved.

Martin

On 08/23/2017 08:36 AM, Martin Sebor wrote:
On 08/22/2017 09:51 PM, Jason Merrill wrote:
The C and C++ front ends already have a diagnose_mismatched_attributes
function, which seems like the natural place to add more conflict
checking.

There are a few major problems with handling attribute conflicts
in diagnose_mismatched_attributes.

The function only diagnoses conflicts, it doesn't resolve them
(choose one attribute or the other).  Currently, when they are
resolved, it's done in each attribute handler.  But this is done
inconsistently because of the limitations of the infrastructure:
no access to the last pushed declaration.  Often, it's not done
at all.  Making the declaration  available to every handler is
one of the design improvements of the patch.

Attributes are defined in the attribute_spec tables in a number
of different places: all the back ends, in addition to the front
ends, but processed by the general infrastructure in
decl_attributes in attribs.c.  The infrastructure already has
all the smarts to either accept or reject a conflicting attribute.
None of this is available in diagnose_mismatched_attributes.
The current implementation of the function hardcodes knowledge
about a small number of attribute relationships in the code.
That's a poor approach given the table-driven attribute_spec
design.  For one, it makes it difficult for back-end maintainers
to add a new attribute that's mutually exclusive with another
(the back-end maintainer would need to change the front end).
It might explain why no back-end attribute conflicts are
diagnosed there.

Some, maybe even most of these problems could be overcome by
moving the conflict resolution from decl_attributes to
diagnose_mismatched_attributes.  But it would mean duplicating
a fair amount of the logic between the two functions.  I think
that would result in an inferior solution.  From both a design
and implementation point of view, I feel the logic fits best
in the attribs.c.

Martin

PR c/81544 - attribute noreturn and warn_unused_result on the same function accepted

gcc/cp/ChangeLog:

	PR c/81544
	* cp-tree.h (decls_match): Add default argument.
	* decl.c (decls_match): Avoid calling into the target back end
	and triggering an error.
	* decl2.c (cplus_decl_attributes): Look up existing declaration and
	pass it to decl_attributes.
	* tree.c (cxx_attribute_table): Initialize new member of struct
	attribute_spec.

gcc/testsuite/ChangeLog:

	PR c/81544
	* g++.dg/Wattributes-2.C: New test.

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index e77241f..736c484 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -6116,7 +6116,7 @@ extern void note_break_stmt			(void);
 extern bool note_iteration_stmt_body_start	(void);
 extern void note_iteration_stmt_body_end	(bool);
 extern tree make_lambda_name			(void);
-extern int decls_match				(tree, tree);
+extern int decls_match				(tree, tree, bool = true);
 extern bool maybe_version_functions		(tree, tree);
 extern tree duplicate_decls			(tree, tree, bool);
 extern tree declare_local_label			(tree);
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 7085d5a..99b22dc 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -992,7 +992,7 @@ push_local_name (tree decl)
    `const int&'.  */
 
 int
-decls_match (tree newdecl, tree olddecl)
+decls_match (tree newdecl, tree olddecl, bool record_versions /* = true */)
 {
   int types_match;
 
@@ -1087,6 +1087,7 @@ decls_match (tree newdecl, tree olddecl)
       if (types_match
 	  && !DECL_EXTERN_C_P (newdecl)
 	  && !DECL_EXTERN_C_P (olddecl)
+	  && record_versions
 	  && maybe_version_functions (newdecl, olddecl))
 	return 0;
     }
diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index 13e7b1d..f20c54d 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -1473,7 +1473,31 @@ cplus_decl_attributes (tree *decl, tree attributes, int flags)
 		       attributes, flags);
     }
   else
-    decl_attributes (decl, attributes, flags);
+    {
+      tree last_decl = (DECL_P (*decl) && DECL_NAME (*decl)
+			? lookup_name (DECL_NAME (*decl)) : NULL_TREE);
+
+      if (last_decl && TREE_CODE (last_decl) == OVERLOAD)
+	for (ovl_iterator iter (last_decl, true); ; ++iter)
+	  {
+	    if (!iter)
+	      {
+		last_decl = NULL_TREE;
+		break;
+	      }
+
+	    if (TREE_CODE (*iter) == OVERLOAD)
+	      continue;
+
+	    if (decls_match (*decl, *iter, /*record_decls=*/false))
+	      {
+		last_decl = *iter;
+		break;
+	      }
+	  }
+
+      decl_attributes (decl, attributes, flags, last_decl);
+    }
 
   if (TREE_CODE (*decl) == TYPE_DECL)
     SET_IDENTIFIER_TYPE_VALUE (DECL_NAME (*decl), TREE_TYPE (*decl));
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index c76dea4..1ebee70 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -4323,24 +4323,24 @@ handle_nodiscard_attribute (tree *node, tree name, tree /*args*/,
 const struct attribute_spec cxx_attribute_table[] =
 {
   /* { name, min_len, max_len, decl_req, type_req, fn_type_req, handler,
-       affects_type_identity } */
+       affects_type_identity, exclusions } */
   { "init_priority",  1, 1, true,  false, false,
-    handle_init_priority_attribute, false },
+    handle_init_priority_attribute, false, NULL },
   { "abi_tag", 1, -1, false, false, false,
-    handle_abi_tag_attribute, true },
-  { NULL,	      0, 0, false, false, false, NULL, false }
+    handle_abi_tag_attribute, true, NULL },
+  { NULL, 0, 0, false, false, false, NULL, false, NULL }
 };
 
 /* Table of C++ standard attributes.  */
 const struct attribute_spec std_attribute_table[] =
 {
   /* { name, min_len, max_len, decl_req, type_req, fn_type_req, handler,
-       affects_type_identity } */
+       affects_type_identity, exclusions } */
   { "maybe_unused", 0, 0, false, false, false,
-    handle_unused_attribute, false },
+    handle_unused_attribute, false, NULL },
   { "nodiscard", 0, 0, false, false, false,
-    handle_nodiscard_attribute, false },
-  { NULL,	      0, 0, false, false, false, NULL, false }
+    handle_nodiscard_attribute, false, NULL },
+  { NULL, 0, 0, false, false, false, NULL, false, NULL }
 };
 
 /* Handle an "init_priority" attribute; arguments as in
diff --git a/gcc/testsuite/g++.dg/Wattributes-2.C b/gcc/testsuite/g++.dg/Wattributes-2.C
new file mode 100644
index 0000000..1470b16
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wattributes-2.C
@@ -0,0 +1,35 @@
+// Test to verify that attributes on distinct overloads of a function
+//  with the same name are properly looked up and applied.
+// { dg-do compile }
+// { dg-options "-Wall" }
+
+int
+foo (int);
+
+int __attribute__ ((noreturn))
+foo (int, int);
+
+int __attribute__ ((warn_unused_result))
+foo (int, int, int);
+
+int call_foo_1 ()
+{
+  foo (1);
+}                       // { dg-warning "\\\[-Wreturn-type]" }
+
+int call_foo_2 ()
+{
+  foo (1, 2);
+}
+
+int call_foo_3 ()
+{
+  foo (1, 2, 3);        // { dg-warning "\\\[-Wunused-result]" }
+}                       // { dg-warning "\\\[-Wreturn-type]" }
+
+int call_foo_4 ()
+{
+  // Make sure an error doesn't trigger bogus warnings or an ICE.
+  foo (1, 2, 3, 4);     // { dg-error "no matching function" }
+  return 0;
+}

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]