Re: [PATCH] attribute copy, leaf, weakref and -Wmisisng-attributes (PR 88546)

[Martin Sebor <msebor@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]

The first revision of the patch was missing a test and didn't
completely or completely correctly handle attribute noreturn.
Attached is an update with the test included and the omission
and bug fixed.

I think it makes sense to consider the patch independently of
the question whether weakrefs should be extern.  That change can
be made separately, with only minor tweaks to the attribute copy
handling and the warning.  None of the other fixes in this patch
(precipitated by more thorough testing) should be affected by it.

Martin

On 12/20/18 8:45 PM, Martin Sebor wrote:
> The enhancement to detect mismatched attributes between function
> aliases and their targets triggers (expected) warnings in GCC
> builds due to aliases being declared with fewer attributes than
> their targets.
> 
> Using attribute copy as recommended to copy the attributes from
> the target to the alias triggers another warning, this time due
> to applying attribute leaf to static functions (the attribute
> only applies to extern functions).  This is due to an oversight
> in both the handler for attribute copy and in
> the -Wmissing-attributes warning.
> 
> In addition, the copy attribute handler doesn't account for C11
> _Noreturn and C++ throw() specifications, both of which set
> the corresponding tree bits but don't attach the synonymous
> attribute to it.  This also leads to warnings in GCC builds
> (in libgfortran).
> 
> The attached patch corrects all of these problems: the attribute
> copy handler to avoid copying attribute leaf to declarations of
> static functions, and to set the noreturn and nonthrow bits, and
> the missing attribute warning to avoid triggering for static
> weakref aliases whose targets are decorated wiwth attribute leaf.
> 
> With this patch, GCC should build with no -Wmissing-attributes
> warnings.
> 
> Tested on x86_64-linux.
> 
> Martin


[-- Attachment #2: gcc-88546.diff --]
[-- Type: text/x-patch, Size: 13002 bytes --]

PR c/88546 - Copy attribute unusable for weakrefs

gcc/c-family/ChangeLog:

	PR c/88546
	* c-attribs.c (handle_copy_attribute): Avoid copying attribute leaf.
	Handle C++ empty throw specification and C11 _Noreturn.
	(has_attribute): Also handle C11 _Noreturn.

gcc/ChangeLog:

	PR c/88546
	* attribs.c (decls_mismatched_attributes): Avoid warning for attribute
	leaf.

libgcc/ChangeLog:

	PR c/88546
	* gthr-posix.h (__gthrw2): Use attribute copy.

libgfortran/ChangeLog:

	PR c/88546
	* libgfortran.h (iexport2): Use attribute copy.

gcc/testsuite/ChangeLog:

	PR c/88546
	* g++.dg/ext/attr-copy.C: New test.
	* gcc.dg/attr-copy-4.c: Disable macro expansion tracking.
	* gcc.dg/attr-copy-6.c: New test.
	* gcc.dg/attr-copy-7.c: New test.

Index: gcc/attribs.c
===================================================================
--- gcc/attribs.c	(revision 267301)
+++ gcc/attribs.c	(working copy)
@@ -1912,6 +1912,12 @@ decls_mismatched_attributes (tree tmpl, tree decl,
 
   for (unsigned i = 0; blacklist[i]; ++i)
     {
+      /* Attribute leaf only applies to extern functions.  Avoid mentioning
+	 it when it's missing from a static declaration.  */
+      if (!TREE_PUBLIC (decl)
+	  && !strcmp ("leaf", blacklist[i]))
+	continue;
+
       for (unsigned j = 0; j != 2; ++j)
 	{
 	  if (!has_attribute (tmpls[j], tmpl_attrs[j], blacklist[i]))
Index: gcc/c-family/c-attribs.c
===================================================================
--- gcc/c-family/c-attribs.c	(revision 267301)
+++ gcc/c-family/c-attribs.c	(working copy)
@@ -2455,6 +2455,12 @@ handle_copy_attribute (tree *node, tree name, tree
 	      || is_attribute_p ("weakref", atname))
 	    continue;
 
+	  /* Aattribute leaf only applies to extern functions.
+	     Avoid copying it to static ones.  */
+	  if (!TREE_PUBLIC (decl)
+	      && is_attribute_p ("leaf", atname))
+	    continue;
+
 	  tree atargs = TREE_VALUE (at);
 	  /* Create a copy of just the one attribute ar AT, including
 	     its argumentsm and add it to DECL.  */
@@ -2472,7 +2478,19 @@ handle_copy_attribute (tree *node, tree name, tree
       return NULL_TREE;
     }
 
+  /* A function declared with attribute nothrow has the attribute
+     attached to it, but a C++ throw() function does not.  */
+  if (TREE_NOTHROW (ref))
+    TREE_NOTHROW (decl) = true;
+
+  /* Similarly, a function declared with attribute noreturn has it
+     attached on to it, but a C11 _Noreturn function does not.  */
   tree reftype = ref;
+  if (DECL_P (ref)
+      && TREE_THIS_VOLATILE (ref)
+      && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))
+    TREE_THIS_VOLATILE (decl) = true;
+
   if (DECL_P (ref) || EXPR_P (ref))
     reftype = TREE_TYPE (ref);
 
@@ -2479,6 +2497,9 @@ handle_copy_attribute (tree *node, tree name, tree
   if (POINTER_TYPE_P (reftype))
     reftype = TREE_TYPE (reftype);
 
+  if (!TYPE_P (reftype))
+    return NULL_TREE;
+
   tree attrs = TYPE_ATTRIBUTES (reftype);
 
   /* Copy type attributes from REF to DECL.  */
@@ -4188,6 +4209,15 @@ has_attribute (location_t atloc, tree t, tree attr
 	      if (expr && DECL_P (expr))
 		found_match = TREE_READONLY (expr);
 	    }
+	  else if (!strcmp ("noreturn", namestr))
+	    {
+	      /* C11 _Noreturn sets the volatile bit without attaching
+		 an attribute to the decl.  */
+	      if (expr
+		  && DECL_P (expr)
+		  && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (expr)))
+		found_match = TREE_THIS_VOLATILE (expr);
+	    }
 	  else if (!strcmp ("pure", namestr))
 	    {
 	      if (expr && DECL_P (expr))
Index: gcc/testsuite/g++.dg/ext/attr-copy.C
===================================================================
--- gcc/testsuite/g++.dg/ext/attr-copy.C	(nonexistent)
+++ gcc/testsuite/g++.dg/ext/attr-copy.C	(working copy)
@@ -0,0 +1,82 @@
+/* PR middle-end/88546 - Copy attribute unusable for weakrefs
+   { dg-do compile }
+   { dg-options "-O1 -Wall -fdump-tree-optimized" }
+   { dg-require-weak "" } */
+
+#define ATTR(...)   __attribute__ ((__VA_ARGS__))
+#define ASRT(expr)   _Static_assert (expr, #expr)
+
+extern "C" {
+
+  ATTR (leaf, nothrow) int
+  fnothrow ()
+  {
+    return 0;
+  }
+
+  static __typeof__ (fnothrow)
+    ATTR (weakref ("fnothrow"), copy (fnothrow))
+    alias_fnothrow;
+
+
+  ATTR (leaf) int
+  fthrow_none () throw ()
+  {
+    return 0;
+  }
+
+  // Verify that no warning is issued for the alias having less
+  // restrictive attributes than the target: nothrow.
+  static __typeof (fthrow_none)
+    ATTR (weakref ("fthrow_none"), copy (fthrow_none))
+    alias_fthrow_none;
+
+  // Same as above but with no definition of the target.
+  ATTR (leaf) int
+  fthrow_none_nodef () throw ();
+
+  static __typeof (fthrow_none_nodef)
+    ATTR (weakref ("fthrow_none_nodef"), copy (fthrow_none_nodef))
+    alias_fthrow_none_nodef;
+
+  // And again but without using typeof to make sure the nothrow
+  // bit is copied by attribute copy alone.
+  static int
+  ATTR (weakref ("fthrow_none_nodef"), copy (fthrow_none_nodef))
+    alias_fthrow_none_nodef_func ();
+}
+
+
+struct UsrClass
+{
+  ~UsrClass ();
+};
+
+// Verify that the nothrow attribute/bit was copied to the alias and
+// that no exception handling code is emitted in any of these calls.
+
+int call_alias_fnothrow ()
+{
+  UsrClass usr;
+  return alias_fnothrow ();
+}
+
+int call_alias_fthrow_none ()
+{
+  UsrClass usr;
+  return alias_fthrow_none ();
+}
+
+int call_alias_fthrow_none_nodef ()
+{
+  UsrClass usr;
+  return alias_fthrow_none_nodef ();
+}
+
+int call_alias_fthrow_none_nodef_func ()
+{
+  UsrClass usr;
+  return alias_fthrow_none_nodef_func ();
+}
+
+// { dg-final { scan-tree-dump-not "__builtin_unwind" "optimized" } }
Index: gcc/testsuite/gcc.dg/attr-copy-4.c
===================================================================
--- gcc/testsuite/gcc.dg/attr-copy-4.c	(revision 267301)
+++ gcc/testsuite/gcc.dg/attr-copy-4.c	(working copy)
@@ -1,7 +1,7 @@
 /* PR middle-end/81824 - Warn for missing attributes with function aliases
    Exercise attribute copy for types.
    { dg-do compile }
-   { dg-options "-O2 -Wall" } */
+   { dg-options "-O2 -Wall -ftrack-macro-expansion=0" } */
 
 #define Assert(expr)   typedef char AssertExpr[2 * !!(expr) - 1]
 
Index: gcc/testsuite/gcc.dg/attr-copy-6.c
===================================================================
--- gcc/testsuite/gcc.dg/attr-copy-6.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/attr-copy-6.c	(working copy)
@@ -0,0 +1,93 @@
+/* PR middle-end/88546 - Copy attribute unusable for weakrefs
+   { dg-do compile }
+   { dg-options "-O2 -Wall" }
+   { dg-require-weak "" } */
+
+#define ATTR(...)   __attribute__ ((__VA_ARGS__))
+#define ASRT(expr)   _Static_assert (expr, #expr)
+
+/* Variable that is local to this translation unit but that can
+   be modified from other units by calling reset_unit_local().  */
+static int unit_local;
+
+void reset_unit_local (void)
+{
+  unit_local = 0;
+}
+
+/* Attribute leaf implies that fleaf() doesn't modify unit_local().  */
+ATTR (leaf, returns_nonnull)
+void* fleaf_retnz (void);
+
+/* Verify both attributes have been applied.  */
+ASRT (__builtin_has_attribute (fleaf_retnz, leaf));
+ASRT (__builtin_has_attribute (fleaf_retnz, returns_nonnull));
+
+/* Verify that attribute leaf has the expected effect.  */
+void call_fleaf_retnz (void)
+{
+  int i = unit_local;
+  void *p = fleaf_retnz ();
+
+  /* Expect both tests to be folded to false and the calls eliminated.  */
+  extern void call_fleaf_retnz_test_leaf_eliminated (void);
+  if (i != unit_local)
+    call_fleaf_retnz_test_leaf_eliminated ();
+
+  extern void call_fleaf_retnz_test_nonnull_eliminated (void);
+  if (p == 0)
+    call_fleaf_retnz_test_nonnull_eliminated ();
+}
+
+
+/* Verify that attribute copy copies the returns_nonnull attribute
+   but doesn't try to copy attribute leaf which only applies to extern
+   function.  */
+static ATTR (copy (fleaf_retnz), weakref ("fleaf_retnz"))
+void* fweakref_fleaf_retnz_copy (void);
+
+ASRT (!__builtin_has_attribute (fweakref_fleaf_retnz_copy, leaf));
+ASRT (__builtin_has_attribute (fweakref_fleaf_retnz_copy, returns_nonnull));
+
+void call_fweakref_fleaf_retnz_copy (void)
+{
+  int i = unit_local;
+  void *p = fweakref_fleaf_retnz_copy ();
+
+  /* Since leaf is not copied, expect the following test not to be
+     folded and the call to be emitted.  */
+  extern void call_fweakref_test_leaf_emitted (void);
+  if (i != unit_local)
+    call_fweakref_test_leaf_emitted ();
+
+  /* Expect the following test to be folded to false and the call
+     eliminated.  */
+  extern void call_fweakref_fleaf_nonnull_eliminated (void);
+  if (p == 0)
+    call_fweakref_fleaf_nonnull_eliminated ();
+}
+
+/* This is reduced from libgfortran/runtime/error.c.  Verify it
+   doesn't trigger warnings and that the noreturn bit is copied
+   to the alias by verifying that calling the alias in a non-void
+   function with no return statement isn't diagnosed.  */
+
+extern _Noreturn void fnoreturn (void);
+
+extern __typeof (fnoreturn)
+  ATTR (visibility ("hidden"))
+  fnoreturn __asm__ ("fnoreturn_name");
+
+void fnoreturn (void)
+{
+  __builtin_abort ();
+}
+
+extern __typeof (fnoreturn)
+  ATTR (alias ("fnoreturn_name"), copy (fnoreturn))
+  fnoreturn_alias;
+
+int call_fnoreturn_alias (void)
+{
+  fnoreturn_alias ();
+}
Index: gcc/testsuite/gcc.dg/attr-copy-7.c
===================================================================
--- gcc/testsuite/gcc.dg/attr-copy-7.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/attr-copy-7.c	(working copy)
@@ -0,0 +1,76 @@
+/* PR middle-end/88546 - Copy attribute unusable for weakrefs
+   Verify that attribute noreturn (represented as volatile on function
+   decls) is interpreted correctly and doesn't affect variables.
+   { dg-do compile }
+   { dg-options "-O1 -Wall -fdump-tree-optimized" }*/
+
+#define ATTR(...)   __attribute__ ((__VA_ARGS__))
+#define ASRT(expr)   _Static_assert (expr, #expr)
+
+ATTR (noreturn) void fnoreturn (void);
+ATTR (copy (fnoreturn)) void fnoreturn_copy (void);
+ASRT (__builtin_has_attribute (fnoreturn_copy, noreturn));
+
+int call_fnoreturn_copy (void)
+{
+  fnoreturn_copy ();
+  fnoreturn_copy ();   // should be eliminated
+}
+
+// { dg-final { scan-tree-dump-times "fnoreturn_copy \\(\\);" 1 "optimized" } }
+
+
+_Noreturn void f_Noreturn (void);
+ATTR (copy (f_Noreturn)) void f_Noreturn_copy (void);
+ASRT (__builtin_has_attribute (f_Noreturn_copy, noreturn));
+
+int call_f_Noreturn_copy (void)
+{
+  f_Noreturn_copy ();
+  f_Noreturn_copy ();   // should be eliminated
+}
+
+// { dg-final { scan-tree-dump-times "f_Noreturn_copy \\(\\);" 1 "optimized" } }
+
+
+// Verify the combination of both is accepted and works too,
+// just for fun.
+ATTR (noreturn) _Noreturn void fnoreturn_Noreturn (void);
+ATTR (copy (fnoreturn_Noreturn)) void fnoreturn_Noreturn_copy (void);
+ASRT (__builtin_has_attribute (fnoreturn_Noreturn_copy, noreturn));
+
+int call_fnoreturn_Noreturn_copy (void)
+{
+  fnoreturn_Noreturn_copy ();
+  fnoreturn_Noreturn_copy ();   // should be eliminated
+}
+
+// { dg-final { scan-tree-dump-times "fnoreturn_Noreturn_copy \\(\\);" 1 "optimized" } }
+
+
+typedef void func_t (void);
+
+ATTR (noreturn) func_t func_noreturn;
+ATTR (copy (func_noreturn)) func_t func_noreturn_copy;
+ASRT (__builtin_has_attribute (func_noreturn_copy, noreturn));
+
+int call_func_noreturn_copy (void)
+{
+  func_noreturn_copy ();
+  func_noreturn_copy ();   // should be eliminated
+}
+
+// { dg-final { scan-tree-dump-times "func_noreturn_copy \\(\\);" 1 "optimized" } }
+
+
+// Finally, verify that the volatile bit isn't copied for variables.
+extern volatile int vi;
+
+int read_nonvolatile (void)
+{
+  ATTR (copy (vi)) int i = 0;
+
+  return i + i;   // should be folded to return 0;
+}
+
+// { dg-final { scan-tree-dump-times "return 0;" 1 "optimized" } }
Index: libgcc/gthr-posix.h
===================================================================
--- libgcc/gthr-posix.h	(revision 267301)
+++ libgcc/gthr-posix.h	(working copy)
@@ -87,7 +87,8 @@ typedef struct timespec __gthread_time_t;
 #  define __gthrw_pragma(pragma)
 # endif
 # define __gthrw2(name,name2,type) \
-  static __typeof(type) name __attribute__ ((__weakref__(#name2))); \
+  static __typeof(type) name \
+    __attribute__ ((__weakref__(#name2), copy (type))); \
   __gthrw_pragma(weak type)
 # define __gthrw_(name) __gthrw_ ## name
 #else
Index: libgfortran/libgfortran.h
===================================================================
--- libgfortran/libgfortran.h	(revision 267301)
+++ libgfortran/libgfortran.h	(working copy)
@@ -202,7 +202,7 @@ extern int __mingw_snprintf (char *, size_t, const
 # define iexport(x)		iexport1(x, IPREFIX(x))
 # define iexport1(x,y)		iexport2(x,y)
 # define iexport2(x,y) \
-	extern __typeof(x) PREFIX(x) __attribute__((__alias__(#y)))
+  extern __typeof(x) PREFIX(x) __attribute__((__alias__(#y), __copy__ (x)))
 #else
 # define export_proto(x)	sym_rename(x, PREFIX(x))
 # define export_proto_np(x)	extern char swallow_semicolon