correct attribute ifunc C++ type safety (PR 82301)

correct attribute ifunc C++ type safety (PR 82301)

[Martin Sebor]

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

r253041 enhanced type checking for alias and ifunc attributes to
detect declarations of incompatible aliases, or ifunc resolvers
that return pointers to functions of an incompatible type.  More
extensive testing exposed a bug in the implementation of the ifunc
attribute handling in C++ where the checker expected the ifunc
resolver to return a pointer to a member function when the
implementation actually expects it return a pointer to a non-
member function.

In a discussion of the test suite failures, Jakub also suggested
to break the enhanced warning out of -Wattributes and issue it
under a different option.

The attached patch corrects the C++ problem and moves the warning
under -Wincompatible-pointer-types.  Since this is a C-only option,
the patch also enables for it C++.  Since the option is enabled by
default, the patch further requires -Wextra to issue the warning
for ifunc resolvers returning void*.  However, the patched checker
diagnoses other incompatibilities without it.

Martin

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

PR c/82301 - Updated test case g++.dg/ext/attr-ifunc-1.C (and others) in r253041 segfault on powerpc64

gcc/ChangeLog:

	PR other/82301
	* cgraphunit.c (maybe_diag_incompatible_alias): New function.
	(handle_alias_pairs): Call it.
	* doc/extend.texi (ifunc attribute): Discuss C++ specifics.
	* doc/invoke.texi (-Wincompatible-pointer-types): Ditto.

gcc/c-family/ChangeLog:

	PR other/82301
	* c.opt (-Wincompatible-pointer-types): Enable for C++.

gcc/testsuite/ChangeLog:

	PR other/82301
	* g++.dg/ext/attr-ifunc-1.C: Update.
	* g++.dg/ext/attr-ifunc-2.C: Same.
	* g++.dg/ext/attr-ifunc-3.C: Same.
	* g++.dg/ext/attr-ifunc-4.C: Same.
	* g++.dg/ext/attr-ifunc-5.C: Same.
	* g++.dg/ext/attr-ifunc-6.C: New test.
	* gcc.dg/attr-ifunc-6.c: New test.
	* gcc.dg/attr-ifunc-7.c: New test.
	* gcc.dg/pr81854.c: Update.
	* lib/target-supports.exp: Update.

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 253099)
+++ gcc/c-family/c.opt	(working copy)
@@ -592,7 +592,7 @@ C C++ Var(warn_ignored_attributes) Init(1) Warning
 Warn whenever attributes are ignored.
 
 Wincompatible-pointer-types
-C ObjC Var(warn_incompatible_pointer_types) Init(1) Warning
+C C++ ObjC ObjcC++ Var(warn_incompatible_pointer_types) Init(1) Warning
 Warn when there is a conversion between pointers that have incompatible types.
 
 Winit-self
Index: gcc/cgraphunit.c
===================================================================
--- gcc/cgraphunit.c	(revision 253099)
+++ gcc/cgraphunit.c	(working copy)
@@ -1296,6 +1296,106 @@ analyze_functions (bool first_time)
   input_location = saved_loc;
 }
 
+/* Check declaration of the type of ALIAS for compatibility with its TARGET
+   (which may be an ifunc resolver) and issue a diagnostic when they are
+   not compatible according to language rules (plus a C++ extension for
+   non-static member functions).  */
+
+static void
+maybe_diag_incompatible_alias (tree alias, tree target)
+{
+  tree altype = TREE_TYPE (alias);
+  tree targtype = TREE_TYPE (target);
+
+  bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+  if (ifunc)
+    {
+      /* Handle attribute ifunc first.  */
+
+      tree funcptr = altype;
+
+      /* Set FUNCPTR to the type of the alias target.  If the type
+	 is a non-static member function of class C, construct a type
+	 of an ordinary function taking C* as the first argument,
+	 followed by the member function argument list, and use it
+	 instead to check for compatibilties.  FUNCPTR is used only
+	 in diagnostics.  */
+
+      if (TREE_CODE (altype) == METHOD_TYPE)
+	{
+	  tree rettype = TREE_TYPE (altype);
+	  tree args = TYPE_ARG_TYPES (altype);
+	  altype = build_function_type (rettype, args);
+	  funcptr = altype;
+	}
+
+      targtype = TREE_TYPE (targtype);
+
+      if (POINTER_TYPE_P (targtype))
+	{
+	  targtype = TREE_TYPE (targtype);
+
+	  /* Only issue Wincompatible-pointer-types for conversions
+	     to void* with -Wextra.  */
+	  if (VOID_TYPE_P (targtype) && !extra_warnings)
+	    return;
+
+	  if ((!FUNC_OR_METHOD_TYPE_P (targtype)
+	       || (prototype_p (altype)
+		   && prototype_p (targtype)
+		   && !types_compatible_p (altype, targtype))))
+	    {
+	      funcptr = build_pointer_type (funcptr);
+
+	      if (warning_at (DECL_SOURCE_LOCATION (target),
+			      OPT_Wincompatible_pointer_types,
+			      "%<ifunc%> resolver for %qD should return %qT",
+			      alias, funcptr))
+		inform (DECL_SOURCE_LOCATION (alias),
+			"resolver indirect function declared here");
+	    }
+	}
+      else
+	{
+	  /* Deal with static member function pointers.  */
+	  if (TREE_CODE (targtype) != RECORD_TYPE
+	      || TYPE_FIELDS (targtype)
+	      || TREE_CODE (TREE_TYPE (TYPE_FIELDS (targtype))) != POINTER_TYPE
+	      || (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS (targtype))))
+		  != METHOD_TYPE))
+	    {
+	      funcptr = build_pointer_type (funcptr);
+
+	      error ("%<ifunc%> resolver for %qD must return %qT",
+		     alias, funcptr);
+			
+	      inform (DECL_SOURCE_LOCATION (alias),
+		      "resolver indirect function declared here");
+	    }
+	}
+
+      return;
+    }
+
+  /* Handle attribute alias.  */
+
+  if ((!FUNC_OR_METHOD_TYPE_P (targtype)
+       || (prototype_p (altype)
+	   && prototype_p (targtype)
+	   && !types_compatible_p (altype, targtype))))
+    {
+      /* Warn for incompatibilities.  Avoid warning for functions
+	 without a prototype to make it possible to declare aliases
+	 without knowing the exact type, as libstdc++ does.  */
+      if (warning_at (DECL_SOURCE_LOCATION (alias),
+		      OPT_Wincompatible_pointer_types,
+		      "%qD alias between functions of incompatible "
+		      "types %qT and %qT", alias, altype, targtype))
+	inform (DECL_SOURCE_LOCATION (target),
+		"aliased declaration here");
+    }
+}
+
 /* Translate the ugly representation of aliases as alias pairs into nice
    representation in callgraph.  We don't handle all cases yet,
    unfortunately.  */
@@ -1305,7 +1405,7 @@ handle_alias_pairs (void)
 {
   alias_pair *p;
   unsigned i;
-  
+
   for (i = 0; alias_pairs && alias_pairs->iterate (i, &p);)
     {
       symtab_node *target_node = symtab_node::get_for_asmname (p->target);
@@ -1352,66 +1452,8 @@ handle_alias_pairs (void)
       if (TREE_CODE (p->decl) == FUNCTION_DECL
           && target_node && is_a <cgraph_node *> (target_node))
 	{
-	  tree t1 = TREE_TYPE (p->decl);
-	  tree t2 = TREE_TYPE (target_node->decl);
+          maybe_diag_incompatible_alias (p->decl, target_node->decl);
 
-	  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (p->decl)))
-	    {
-	      t2 = TREE_TYPE (t2);
-	      if (POINTER_TYPE_P (t2))
-		{
-		  t2 = TREE_TYPE (t2);
-		  if (!FUNC_OR_METHOD_TYPE_P (t2))
-		    {
-		      if (warning_at (DECL_SOURCE_LOCATION (p->decl),
-				      OPT_Wattributes,
-				      "%q+D %<ifunc%> resolver should return "
-				      "a function pointer",
-				      p->decl))
-			inform (DECL_SOURCE_LOCATION (target_node->decl),
-				"resolver declaration here");
-
-		      t2 = NULL_TREE;
-		    }
-		}
-	      else
-		{
-		  /* Deal with static member function pointers.  */
-		  if (TREE_CODE (t2) == RECORD_TYPE
-		      && TYPE_FIELDS (t2)
-		      && TREE_CODE (TREE_TYPE (TYPE_FIELDS (t2))) == POINTER_TYPE
-		      && (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS (t2))))
-			  == METHOD_TYPE))
-		    t2 = TREE_TYPE (TREE_TYPE (TYPE_FIELDS (t2)));
-		  else
-		    {
-		      error ("%q+D %<ifunc%> resolver must return a function "
-			     "pointer",
-			     p->decl);
-		      inform (DECL_SOURCE_LOCATION (target_node->decl),
-			      "resolver declaration here");
-
-		      t2 = NULL_TREE;
-		    }
-		}
-	    }
-
-	  if (t2
-	      && (!FUNC_OR_METHOD_TYPE_P (t2)
-		  || (prototype_p (t1)
-		      && prototype_p (t2)
-		      && !types_compatible_p (t1, t2))))
-	    {
-	      /* Warn for incompatibilities.  Avoid warning for functions
-		 without a prototype to make it possible to declare aliases
-		 without knowing the exact type, as libstdc++ does.  */
-	      if (warning_at (DECL_SOURCE_LOCATION (p->decl), OPT_Wattributes,
-			      "%q+D alias between functions of incompatible "
-			      "types %qT and %qT", p->decl, t1, t2))
-		inform (DECL_SOURCE_LOCATION (target_node->decl),
-			"aliased declaration here");
-	    }
-
 	  cgraph_node *src_node = cgraph_node::get (p->decl);
 	  if (src_node && src_node->definition)
 	    src_node->reset ();
Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 253099)
+++ gcc/doc/extend.texi	(working copy)
@@ -2801,7 +2801,7 @@ void *my_memcpy (void *dst, const void *src, size_
 
 static void * (*resolve_memcpy (void))(void *, const void *, size_t)
 @{
-  return my_memcpy; // we'll just always select this routine
+  return my_memcpy; // we will just always select this routine
 @}
 @end smallexample
 
@@ -2814,9 +2814,9 @@ extern void *memcpy (void *, const void *, size_t)
 @end smallexample
 
 @noindent
-allowing the user to call this as a regular function, unaware of the
-implementation.  Finally, the indirect function needs to be defined in
-the same translation unit as the resolver function:
+allowing the user to call @code{memcpy} as a regular function, unaware of
+the actual implementation.  Finally, the indirect function needs to be
+defined in the same translation unit as the resolver function:
 
 @smallexample
 void *memcpy (void *, const void *, size_t)
@@ -2823,6 +2823,47 @@ void *memcpy (void *, const void *, size_t)
      __attribute__ ((ifunc ("resolve_memcpy")));
 @end smallexample
 
+In C++, the @code{ifunc} attribute takes a string that is the mangled name
+of the resolver function.  A C++ resolver for a non-static member function
+of class @code{C} should be declared to return a pointer to a non-member
+function taking pointer to @code{C} as the first argument, followed by
+the same arguments as of the implementation function.  G++ checks
+the signatures of the two functions and issues
+a @option{-Wincompatible-pointer-types} warning for mismatches.  To suppress
+a warning for the necessary cast from a pointer to the implementation member
+function to the type of the corresponding non-member function use the
+@option{-Wno-pmf-conversions} option.  For example:
+
+@smallexample
+class S
+@{
+private:
+  int debug_impl (int);
+  int optimized_impl (int);
+
+  typedef int Func (S*, int);
+
+  static Func* resolver ();
+public:
+
+  int interface (int);
+@};
+
+int S::debug_impl (int) @{ /* @r{@dots{}} */ @}
+int S::optimized_impl (int) @{ /* @r{@dots{}} */ @}
+
+S::Func* S::resolver ()
+@{
+  int (S::*pimpl) (int)
+    = getenv ("DEBUG") ? &S::debug_impl : &S::optimized_impl;
+
+  // Cast triggers -Wno-pmf-conversions.
+  return reinterpret_cast<Func*>(pimpl);
+@}
+
+int S::interface (int) __attribute__ ((ifunc ("_ZN1S8resolverEv")));
+@end smallexample
+
 Indirect functions cannot be weak.  Binutils version 2.20.1 or higher
 and GNU C Library version 2.11.1 are required to use this feature.
 
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 253099)
+++ gcc/doc/invoke.texi	(working copy)
@@ -5456,13 +5456,16 @@ are being discarded. Typically, the compiler warns
 takes a @code{int (*)[]} parameter.  This option can be used to
 suppress such a warning.
 
-@item -Wno-incompatible-pointer-types @r{(C and Objective-C only)}
+@item -Wno-incompatible-pointer-types
 @opindex Wno-incompatible-pointer-types
 @opindex Wincompatible-pointer-types
 Do not warn when there is a conversion between pointers that have incompatible
 types.  This warning is for cases not covered by @option{-Wno-pointer-sign},
 which warns for pointer argument passing or assignment with different
-signedness.
+signedness.  In C++ where incompatible pointer conversions ordinarily cause
+errors, the warning detects such conversions in GCC extensions that aren't
+part of the standard language.  An example of a construct that might perform
+such a conversion is the @code{alias} attribute.  @xref{Function Attributes,,Declaring Attributes of Functions}.
 
 @item -Wno-int-conversion @r{(C and Objective-C only)}
 @opindex Wno-int-conversion
Index: gcc/testsuite/g++.dg/ext/attr-ifunc-1.C
===================================================================
--- gcc/testsuite/g++.dg/ext/attr-ifunc-1.C	(revision 253099)
+++ gcc/testsuite/g++.dg/ext/attr-ifunc-1.C	(working copy)
@@ -4,26 +4,33 @@
 
 struct Klass
 {
+  int a[4];
+
   int implementation ();
   int magic ();
 
-  typedef int (Klass::*MemFuncPtr)();
+  /* An ifunc resolver must return a pointer to an ordinary (non-member)
+     function.  To make it possible to use ifunc with member functions,
+     the resolver must convert a member function pointer to an ordinary
+     function pointer (slicing off the high word).  */
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
-Klass::MemFuncPtr p = &Klass::implementation;
-
-int Klass::implementation (void)
+int Klass::implementation ()
 {
   __builtin_printf ("'ere I am JH\n");
-  return 1234;
+  return a[0] + a[1] + a[2] + a[3];
 }
 
+Klass::Func* Klass::resolver (void)
+{
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
 
-Klass::MemFuncPtr Klass::resolver (void)
-{
-  return &Klass::implementation;
+  return reinterpret_cast<Func*>(&Klass::implementation);
 }
 
 int f (void) __attribute__ ((ifunc ("foo")));
@@ -32,11 +39,16 @@ typedef int (F)(void);
 extern "C" F* foo () { return 0; }
 
 
-int Klass::magic (void) __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
+int Klass::magic () __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
 
 int main ()
 {
   Klass obj;
 
-  return !(obj.magic () == 1234);
+  obj.a[0] = 1;
+  obj.a[1] = 2;
+  obj.a[2] = 3;
+  obj.a[3] = 4;
+
+  return !(obj.magic () == 10);
 }
Index: gcc/testsuite/g++.dg/ext/attr-ifunc-2.C
===================================================================
--- gcc/testsuite/g++.dg/ext/attr-ifunc-2.C	(revision 253099)
+++ gcc/testsuite/g++.dg/ext/attr-ifunc-2.C	(working copy)
@@ -9,9 +9,9 @@ struct Klass
   int implementation ();
   int magic ();
 
-  typedef int (Klass::*MemFuncPtr)();
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
 int Klass::implementation (void)
@@ -20,9 +20,13 @@ int Klass::implementation (void)
   return 0;
 }
 
-Klass::MemFuncPtr Klass::resolver (void)
+Klass::Func* Klass::resolver (void)
 {
-  return &Klass::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klass::implementation);
 }
 
 int Klass::magic (void) __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
Index: gcc/testsuite/g++.dg/ext/attr-ifunc-3.C
===================================================================
--- gcc/testsuite/g++.dg/ext/attr-ifunc-3.C	(revision 253099)
+++ gcc/testsuite/g++.dg/ext/attr-ifunc-3.C	(working copy)
@@ -6,23 +6,29 @@
 
 struct Klass
 {
+  int a[4];
+
   int implementation ();
   int magic ();
 
-  typedef int (Klass::*MemFuncPtr)();
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
 int Klass::implementation (void)
 {
   printf ("'ere I am JH\n");
-  return 0;
+  return a[0] + a[1] + a[2] + a[3];
 }
 
-Klass::MemFuncPtr Klass::resolver (void)
+Klass::Func* Klass::resolver ()
 {
-  return &Klass::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klass::implementation);
 }
 
 int Klass::magic (void) __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
@@ -36,5 +42,10 @@ int main ()
 {
   Klass obj;
 
-  return Foo (obj, &Klass::magic) != 0;
+  obj.a[0] = 1;
+  obj.a[1] = 2;
+  obj.a[2] = 3;
+  obj.a[3] = 4;
+
+  return Foo (obj, &Klass::magic) != 10;
 }
Index: gcc/testsuite/g++.dg/ext/attr-ifunc-4.C
===================================================================
--- gcc/testsuite/g++.dg/ext/attr-ifunc-4.C	(revision 253099)
+++ gcc/testsuite/g++.dg/ext/attr-ifunc-4.C	(working copy)
@@ -14,9 +14,9 @@ struct Klassier : Klass
   int implementation ();
   int magic ();
 
-  typedef int (Klassier::*MemFuncPtr)();
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
 int Klassier::implementation (void)
@@ -25,9 +25,13 @@ int Klassier::implementation (void)
   return 0;
 }
 
-Klassier::MemFuncPtr Klassier::resolver (void)
+Klassier::Func* Klassier::resolver ()
 {
-  return &Klassier::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klassier::implementation);
 }
 
 int Klassier::magic (void) __attribute__ ((ifunc ("_ZN8Klassier8resolverEv")));
Index: gcc/testsuite/g++.dg/ext/attr-ifunc-5.C
===================================================================
--- gcc/testsuite/g++.dg/ext/attr-ifunc-5.C	(revision 253099)
+++ gcc/testsuite/g++.dg/ext/attr-ifunc-5.C	(working copy)
@@ -1,15 +1,19 @@
 // PR c/81854 - weak alias of an incompatible symbol accepted
 // { dg-do compile }
 // { dg-require-ifunc "" } */
+// { dg-options "-Wno-pmf-conversions" }
 
 struct Klass
 {
   int implementation ();
-  const char* magic ();
+  int good_magic ();
+  const char* bad_magic ();
 
+  typedef int (Func)(Klass*);
   typedef int (Klass::*MemFuncPtr)();
 
-  static MemFuncPtr resolver ();
+  static Func* good_resolver ();
+  static MemFuncPtr bad_resolver ();
 };
 
 int Klass::implementation (void)
@@ -17,13 +21,28 @@ int Klass::implementation (void)
   return 0;
 }
 
-const char* __attribute__ ((ifunc ("_ZN5Klass8resolverEv")))
-  Klass::magic ();   // { dg-warning "alias between functions of incompatible types" }
 
+int __attribute__ ((ifunc ("_ZN5Klass13good_resolverEv")))
+Klass::good_magic ();
 
 
+Klass::Func*
+Klass::good_resolver (void)
+{
+  MemFuncPtr mfp = &Klass::implementation;
+
+  return reinterpret_cast<Func*>(mfp);
+}
+
+
+const char* __attribute__ ((ifunc ("_ZN5Klass12bad_resolverEv")))
+Klass::bad_magic ();   // { dg-message "resolver indirect function declared here" }
+
+
 Klass::MemFuncPtr
-Klass::resolver (void) // { dg-message "aliased declaration here" }
+Klass::bad_resolver (void)
 {
   return &Klass::implementation;
-}
+  // The location of this error is unfortunate.  It would be a lot better
+  // if it were on the next to the prototype...
+}   // { dg-error ".ifunc. resolver for .const char\\* Klass::bad_magic\\(\\). must return .const char\\* \\(\\*\\)\\(Klass\\*\\)." }
Index: gcc/testsuite/g++.dg/ext/attr-ifunc-6.C
===================================================================
--- gcc/testsuite/g++.dg/ext/attr-ifunc-6.C	(nonexistent)
+++ gcc/testsuite/g++.dg/ext/attr-ifunc-6.C	(working copy)
@@ -0,0 +1,81 @@
+// PR c/81854 - weak alias of an incompatible symbol accepted
+// { dg-do run }
+// { dg-require-ifunc "" }
+// { dg-options "-Wno-pmf-conversions" }
+
+struct Klass
+{
+  int a[4];
+
+  int implementation (int);
+  int implementation (int, long, const char*);
+
+  int magic (int);
+  int magic (int, long, const char *);
+
+  /* An ifunc resolver must return a pointer to an ordinary (non-member)
+     function.  To make it possible to use ifunc with member functions,
+     the resolver must convert a member function pointer to an ordinary
+     function pointer (slicing off the high word).  */
+  typedef int Func1 (Klass*, int);
+  typedef int Func3 (Klass*, int, long, const char*);
+
+  template <class Func>
+  static Func* resolver ();
+};
+
+const char *str;
+
+int Klass::implementation (int x)
+{
+  return a[0] + a[1] + a[2] + a[3] + x;
+}
+
+int Klass::implementation (int x, long y, const char *s)
+{
+  return a[0] + a[1] + a[2] + a[3] + x + y + !(s == str);
+}
+
+template <>
+Klass::Func1* Klass::resolver<Klass::Func1> ()
+{
+  int (Klass::*pmf)(int) = &Klass::implementation;
+
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  The cast triggers -Wno-pmf-conversions which
+     the test suppresses.  */
+
+  return reinterpret_cast<Func1*>(pmf);
+}
+
+template <>
+Klass::Func3* Klass::resolver<Klass::Func3> ()
+{
+  int (Klass::*pmf)(int, long, const char*) = &Klass::implementation;
+
+  return reinterpret_cast<Func3*>(pmf);
+}
+
+__attribute__ ((ifunc ("_ZN5Klass8resolverIFiPS_iEEEPT_v")))
+int Klass::magic (int);
+
+__attribute__ ((ifunc ("_ZN5Klass8resolverIFiPS_ilPKcEEEPT_v")))
+int Klass::magic (int, long, const char *);
+
+int main ()
+{
+  Klass obj;
+
+  obj.a[0] = 1;
+  obj.a[1] = 2;
+  obj.a[2] = 3;
+  obj.a[3] = 4;
+
+  str = "teststring";
+
+  // Verify that data members and function arguments are correctly
+  // retrieved.
+  if (obj.magic (5) != 15 || obj.magic (5, 6, str) != 21)
+    __builtin_abort ();
+}
Index: gcc/testsuite/gcc.dg/attr-ifunc-6.c
===================================================================
--- gcc/testsuite/gcc.dg/attr-ifunc-6.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/attr-ifunc-6.c	(working copy)
@@ -0,0 +1,31 @@
+/* { dg-do compile }  */
+/* { dg-require-ifunc "" } */
+/* { dg-options "" } */
+
+static int implementation (void)
+{
+  return 0;
+}
+
+/* Verify that a resolver that returns void* is not diagnosed without
+   -Wextra (see attr-ifunc-7.c for a test that verifies that -Wextra
+   triggers the warning).  */
+static void* resolver_nowarn (void)
+{
+  return implementation;
+}
+
+extern int
+indirect_nowarn (void) __attribute__ ((ifunc ("resolver_nowarn")));
+
+
+/* Verify that a resolver that returns a T* that's not void* is diagnosed
+   even without -Wextra.  */
+static int* resolver_warn (void)   /* { dg-warning ".ifunc. resolver for .indirect_warn. should return 'int \\(\\*\\)\\(void\\)." } */
+{
+  return (int *)implementation;
+}
+
+extern int
+indirect_warn (void)   /* { dg-message "" } */
+__attribute__ ((ifunc ("resolver_warn")));
Index: gcc/testsuite/gcc.dg/attr-ifunc-7.c
===================================================================
--- gcc/testsuite/gcc.dg/attr-ifunc-7.c	(nonexistent)
+++ gcc/testsuite/gcc.dg/attr-ifunc-7.c	(working copy)
@@ -0,0 +1,30 @@
+/* { dg-do compile }  */
+/* { dg-require-ifunc "" } */
+/* { dg-options "-Wextra" } */
+
+static int implementation (void)
+{
+  return 0;
+}
+
+/* Verify that a resolver that returns void* is diagnosed with -Wextra.  */
+static void* resolver_warn (void)   /* { dg-warning ".ifunc. resolver for .indirect_warn. should return .int \\(\\*\\)\\(void\\)." } */
+{
+  return implementation;
+}
+
+extern __attribute__ ((ifunc ("resolver_warn"))) int
+indirect_warn (void);   /* { dg-message "resolver indirect function declared here" } */
+
+
+
+/* Verify that a resolver that returns int* is still diagnosed, even
+   with -Wextra.  */
+static int* resolver_warn_2 (void)   /* { dg-warning ".ifunc. resolver for .indirect_warn_2. should return .int \\(\\*\\)\\(void\\)." } */
+{
+  return (int *)implementation;
+}
+
+extern __attribute__ ((ifunc ("resolver_warn_2"))) int
+indirect_warn_2 (void);   /* { dg-message "resolver indirect function declared here" } */
+
Index: gcc/testsuite/gcc.dg/pr81854.c
===================================================================
--- gcc/testsuite/gcc.dg/pr81854.c	(revision 253099)
+++ gcc/testsuite/gcc.dg/pr81854.c	(working copy)
@@ -1,6 +1,7 @@
 /* PR c/81854 - weak alias of an incompatible symbol accepted
    { dg-do compile }
-   { dg-require-ifunc "" } */
+   { dg-require-ifunc "" }
+   { dg-options "-Wextra" } */
 
 const char* __attribute__ ((weak, alias ("f0_target")))
 f0 (void);          /* { dg-error "alias between function and variable" } */
@@ -26,11 +27,10 @@ const char* f2_target (int i)   /* { dg-message "a
   return 0;
 }
 
-
 int __attribute__ ((ifunc ("f3_resolver")))
-f3 (void);          /* { dg-error ".ifunc. resolver must return a function pointer" } */
+f3 (void);          /* { dg-message "resolver indirect function declared here" } */
 
-int f3_resolver (void)   /* { dg-message "resolver declaration here" } */
+void* f3_resolver (void) /* { dg-warning "ifunc. resolver for .f3. should return .int \\(\\*\\)\\(void\\)." } */
 {
   return 0;
 }
@@ -37,28 +37,32 @@ int __attribute__ ((ifunc ("f3_resolver")))
 
 
 int __attribute__ ((ifunc ("f4_resolver")))
-f4 (void);          /* { dg-warning ".ifunc. resolver should return a function pointer" } */
+f4 (void);          /* { dg-message "resolver indirect function declared here" } */
 
-void* f4_resolver (void) /* { dg-message "resolver declaration here" } */
+typedef void F4 (void);
+F4* f4_resolver (void) /* { dg-warning ".ifunc. resolver for .f4. should return .int \\(\\*\\)\\(void\\)" } */
 {
   return 0;
 }
 
+const char* __attribute__ ((ifunc ("f5_resolver")))
+f5 (void);
 
-int __attribute__ ((ifunc ("f5_resolver")))
-f5 (void);          /* { dg-warning "alias between functions of incompatible types" } */
-
-typedef void F5 (void);
-F5* f5_resolver (void) /* { dg-message "aliased declaration here" } */
+typedef const char* F5 (void);
+F5* f5_resolver (void)
 {
   return 0;
 }
 
-const char* __attribute__ ((ifunc ("f6_resolver")))
-f6 (void);
+int __attribute__ ((ifunc ("f6_resolver")))
+f6 (void);          /* { dg-message "resolver indirect function declared here" } */
 
-typedef const char* F6 (void);
-F6* f6_resolver (void)
+int f6_resolver (void)
 {
   return 0;
 }
+
+/* Unfortunately, the diagnostic location is either the closing curly
+   of the resolver defintion or the last declaration that comes after
+   it (if one does).  */
+int f6_resolver (void);   /* { dg-error ".ifunc. resolver for 'f6' must return .int \\(\\*\\)\\(void\\)." } */
Index: gcc/testsuite/lib/target-supports.exp
===================================================================
--- gcc/testsuite/lib/target-supports.exp	(revision 253099)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -440,8 +440,8 @@ proc check_ifunc_available { } {
 	extern "C" {
 	#endif
 	typedef void F (void);
-	F* g() {}
-	void f() __attribute__((ifunc("g")));
+	F* g (void) {}
+	void f () __attribute__ ((ifunc ("g")));
 	#ifdef __cplusplus
 	}
 	#endif

Re: correct attribute ifunc C++ type safety (PR 82301)

[Martin Sebor]

Hi Nathan,

This patch is a tweak for the C++ part of the type safety
enhancement to attribute ifunc committed in r253041.  It touches
the C++ ifunc tests you added some years ago and I recently broke
with the initial commits of the feature.  The notable difference
between r253041 and this update (other than correcting the type
expected to be returned by the resolver for a member function)
is issuing the incompatibility warning under -Wincompatible-
pointer-types rather than -Wattributes.  When you and/or Jason
have a minute, can you please review it?

https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01603.html

Thanks
Martin

On 09/24/2017 07:03 PM, Martin Sebor wrote:
> r253041 enhanced type checking for alias and ifunc attributes to
> detect declarations of incompatible aliases, or ifunc resolvers
> that return pointers to functions of an incompatible type.  More
> extensive testing exposed a bug in the implementation of the ifunc
> attribute handling in C++ where the checker expected the ifunc
> resolver to return a pointer to a member function when the
> implementation actually expects it return a pointer to a non-
> member function.
>
> In a discussion of the test suite failures, Jakub also suggested
> to break the enhanced warning out of -Wattributes and issue it
> under a different option.
>
> The attached patch corrects the C++ problem and moves the warning
> under -Wincompatible-pointer-types.  Since this is a C-only option,
> the patch also enables for it C++.  Since the option is enabled by
> default, the patch further requires -Wextra to issue the warning
> for ifunc resolvers returning void*.  However, the patched checker
> diagnoses other incompatibilities without it.
>
> Martin

Re: correct attribute ifunc C++ type safety (PR 82301)

[Pedro Alves]

On 09/25/2017 02:03 AM, Martin Sebor wrote:

> +a @option{-Wincompatible-pointer-types} warning for mismatches.  To suppress
> +a warning for the necessary cast from a pointer to the implementation member
> +function to the type of the corresponding non-member function use the
> +@option{-Wno-pmf-conversions} option.  For example:

FWIW, it seems odd to me to tell users they need to suppress warnings, when
the compiler surely could provide better/safer means to avoid needing
to use the reinterpret_cast hammer.   See below.

> +
> +@smallexample
> +class S
> +@{
> +private:
> +  int debug_impl (int);
> +  int optimized_impl (int);
> +
> +  typedef int Func (S*, int);
> +
> +  static Func* resolver ();
> +public:
> +
> +  int interface (int);
> +@};
> +
> +int S::debug_impl (int) @{ /* @r{@dots{}} */ @}
> +int S::optimized_impl (int) @{ /* @r{@dots{}} */ @}
> +
> +S::Func* S::resolver ()
> +@{
> +  int (S::*pimpl) (int)
> +    = getenv ("DEBUG") ? &S::debug_impl : &S::optimized_impl;
> +
> +  // Cast triggers -Wno-pmf-conversions.
> +  return reinterpret_cast<Func*>(pimpl);
> +@}
> +

If I were writing code like this, I'd write a reinterpret_cast-like
function specifically for pointer-to-member-function to free-function
casting, and only suppress the warning there instead of disabling
the warning for the whole translation unit.  Something like:

#include <type_traits>

template<typename T> struct pmf_as_func;

template<typename Ret, typename S, typename... Args>
struct pmf_as_func<Ret (S::*) (Args...)>
{
  typedef Ret (func_type) (S *, Args...);
  typedef S struct_type;
};

template<typename Pmf>
typename pmf_as_func<Pmf>::func_type *
pmf_as_func_cast (Pmf pmf)
{
  static_assert (!std::is_polymorphic<typename pmf_as_func<Pmf>::struct_type>::value,
		 "");
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpmf-conversions"
  return reinterpret_cast<typename pmf_as_func<Pmf>::func_type *> (pmf);
#pragma GCC diagnostic pop
}


and then write:
 return pmf_as_func_cast (pimpl);

instead of:
  return reinterpret_cast<Func*>(pimpl);

The point being of course to make it harder to misuse the casts.

But that may be a bit too much for the manual.  

It also wouldn't work as is with C++03 (because variatic templates).
Which leads me to think that if GCC guarantees this cast works, then
it'd be nice to have GCC provide it (like a __pmf_as_func_cast function)
as builtin.  Then it'd work on C++03 as well, and the compiler of course
can precisely validate whether the cast is valid.  (It's quite possible
that there's a better check than is_polymorphic as I've written above.)

Just a passing thought.

Thanks,
Pedro Alves

Re: correct attribute ifunc C++ type safety (PR 82301)

[Nathan Sidwell]

On 09/24/2017 06:03 PM, Martin Sebor wrote:
> r253041 enhanced type checking for alias and ifunc attributes to
> detect declarations of incompatible aliases, or ifunc resolvers
> that return pointers to functions of an incompatible type.  More
> extensive testing exposed a bug in the implementation of the ifunc
> attribute handling in C++ where the checker expected the ifunc
> resolver to return a pointer to a member function when the
> implementation actually expects it return a pointer to a non-
> member function.
> 
> In a discussion of the test suite failures, Jakub also suggested
> to break the enhanced warning out of -Wattributes and issue it
> under a different option.
> 
> The attached patch corrects the C++ problem and moves the warning
> under -Wincompatible-pointer-types.  Since this is a C-only option,
> the patch also enables for it C++.  Since the option is enabled by
> default, the patch further requires -Wextra to issue the warning
> for ifunc resolvers returning void*.  However, the patched checker
> diagnoses other incompatibilities without it.
> 
> Martin

I find the maybe_diag_incompatible_alias function confusing.

> +/* Check declaration of the type of ALIAS for compatibility with its TARGET
> +   (which may be an ifunc resolver) and issue a diagnostic when they are
> +   not compatible according to language rules (plus a C++ extension for
> +   non-static member functions).  */
> +
> +static void
> +maybe_diag_incompatible_alias (tree alias, tree target)
> +{
> +  tree altype = TREE_TYPE (alias);
> +  tree targtype = TREE_TYPE (target);
> +
> +  bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
> +  if (ifunc)

I think it might be clearer if this was broken out into a diag_ifunc 
function?  But see below ...

> +    {
> +      /* Handle attribute ifunc first.  */
> +
> +      tree funcptr = altype;
> +
> +      /* Set FUNCPTR to the type of the alias target.  If the type
> +	 is a non-static member function of class C, construct a type
> +	 of an ordinary function taking C* as the first argument,
> +	 followed by the member function argument list, and use it
> +	 instead to check for compatibilties.  FUNCPTR is used only
> +	 in diagnostics.  */

This comment is self-contradictory.
   1 Set FUNCPTR
   2 Do some method-type shenanigans
   3 Use it to check for incompatibilites
   4 FUNCPTR is only used in diags

Which of #3 and #4 is true?


> +
> +      if (TREE_CODE (altype) == METHOD_TYPE)
> +	{

IMHO put the description of the METHOD_TYPE chicanery inside the block 
doing it?  FWIW, although the change being made works on many (most?) 
ABIs, it's not formally correct and I think fails on some where 'this' 
is passed specially. You might want to note that?

> +	  tree rettype = TREE_TYPE (altype);
> +	  tree args = TYPE_ARG_TYPES (altype);
> +	  altype = build_function_type (rettype, args);
> +	  funcptr = altype;
> +	}
> +

> +	  if ((!FUNC_OR_METHOD_TYPE_P (targtype)
> +	       || (prototype_p (altype)
> +		   && prototype_p (targtype)
> +		   && !types_compatible_p (altype, targtype))))
> +	    {
> +	      funcptr = build_pointer_type (funcptr);
> +
> +	      if (warning_at (DECL_SOURCE_LOCATION (target),
> +			      OPT_Wincompatible_pointer_types,
> +			      "%<ifunc%> resolver for %qD should return %qT",
> +			      alias, funcptr))
> +		inform (DECL_SOURCE_LOCATION (alias),
> +			"resolver indirect function declared here");
> +	    }

this block is almost the same as the non-ifunc block.  Surely they can 
be the same code? (by generalizing one of the cases until it turns into 
the other?)


> +	  /* Deal with static member function pointers.  */

I do not understand this comment or condition. We seem to have dealt 
with pointers already and the conditions seem confused.

> +	  if (TREE_CODE (targtype) != RECORD_TYPE
> +	      || TYPE_FIELDS (targtype)
> +	      || TREE_CODE (TREE_TYPE (TYPE_FIELDS (targtype))) != POINTER_TYPE
> +	      || (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS (targtype))))
> +		  != METHOD_TYPE))

if
   not a record,
   or has TYPE_FIELDS non-NULL
   or the first field doesn't have pointer type (we can't get here)
   or something else about the first field

oh, I think it's trying to spot the pointer to NON-static member 
function internal record type.  But brokenly. I think pmf record_types 
have DECL_ARTIFICIAL and BUILTIN_LOCATION, that might be useful.

> +	    {
> +	      funcptr = build_pointer_type (funcptr);
> +
> +	      error ("%<ifunc%> resolver for %qD must return %qT",
> +		     alias, funcptr);
> +			
> +	      inform (DECL_SOURCE_LOCATION (alias),
> +		      "resolver indirect function declared here");
> +	    }
> +	}
> +

> +  if ((!FUNC_OR_METHOD_TYPE_P (targtype)
> +       || (prototype_p (altype)
> +	   && prototype_p (targtype)
> +	   && !types_compatible_p (altype, targtype))))

Similar to above, already noted.

Is this function called before we know whether we've enabled the 
appropriate warnings?  It would be better to bail out sooner if the 
warnings are disabled.

nathan

-- 
Nathan Sidwell

Re: correct attribute ifunc C++ type safety (PR 82301)

[Martin Sebor]

On 09/28/2017 05:23 AM, Pedro Alves wrote:
> On 09/25/2017 02:03 AM, Martin Sebor wrote:
>
>> +a @option{-Wincompatible-pointer-types} warning for mismatches.  To suppress
>> +a warning for the necessary cast from a pointer to the implementation member
>> +function to the type of the corresponding non-member function use the
>> +@option{-Wno-pmf-conversions} option.  For example:
>
> FWIW, it seems odd to me to tell users they need to suppress warnings, when
> the compiler surely could provide better/safer means to avoid needing
> to use the reinterpret_cast hammer.   See below.
>
>> +
>> +@smallexample
>> +class S
>> +@{
>> +private:
>> +  int debug_impl (int);
>> +  int optimized_impl (int);
>> +
>> +  typedef int Func (S*, int);
>> +
>> +  static Func* resolver ();
>> +public:
>> +
>> +  int interface (int);
>> +@};
>> +
>> +int S::debug_impl (int) @{ /* @r{@dots{}} */ @}
>> +int S::optimized_impl (int) @{ /* @r{@dots{}} */ @}
>> +
>> +S::Func* S::resolver ()
>> +@{
>> +  int (S::*pimpl) (int)
>> +    = getenv ("DEBUG") ? &S::debug_impl : &S::optimized_impl;
>> +
>> +  // Cast triggers -Wno-pmf-conversions.
>> +  return reinterpret_cast<Func*>(pimpl);
>> +@}
>> +
>
> If I were writing code like this, I'd write a reinterpret_cast-like
> function specifically for pointer-to-member-function to free-function
> casting, and only suppress the warning there instead of disabling
> the warning for the whole translation unit.  Something like:
>
> #include <type_traits>
>
> template<typename T> struct pmf_as_func;
>
> template<typename Ret, typename S, typename... Args>
> struct pmf_as_func<Ret (S::*) (Args...)>
> {
>   typedef Ret (func_type) (S *, Args...);
>   typedef S struct_type;
> };
>
> template<typename Pmf>
> typename pmf_as_func<Pmf>::func_type *
> pmf_as_func_cast (Pmf pmf)
> {
>   static_assert (!std::is_polymorphic<typename pmf_as_func<Pmf>::struct_type>::value,
> 		 "");
> #pragma GCC diagnostic push
> #pragma GCC diagnostic ignored "-Wpmf-conversions"
>   return reinterpret_cast<typename pmf_as_func<Pmf>::func_type *> (pmf);
> #pragma GCC diagnostic pop
> }
>
>
> and then write:
>  return pmf_as_func_cast (pimpl);
>
> instead of:
>   return reinterpret_cast<Func*>(pimpl);
>
> The point being of course to make it harder to misuse the casts.
>
> But that may be a bit too much for the manual.

Yes, that's a much nicer solution but as you say, more code than
the manual typically includes for warnings.  (It would be a good
example for a Users Guide if GCC had one.)

> It also wouldn't work as is with C++03 (because variatic templates).
> Which leads me to think that if GCC guarantees this cast works, then
> it'd be nice to have GCC provide it (like a __pmf_as_func_cast function)
> as builtin.  Then it'd work on C++03 as well, and the compiler of course
> can precisely validate whether the cast is valid.  (It's quite possible
> that there's a better check than is_polymorphic as I've written above.)

I also don't like the cast and have been thinking about how to
suppress the warning in this case (the ifunc resolver).  But
the only way I can think to make it possible is to defer
-Wno-pmf-conversions until after all function declarations in
the translation unit have been processed.  In other words, run
it in a separate pass and suppress it for functions that have
been aliased via attribute ifunc.  I don't know if there already
is a pass like that it could be hooked into.  If there is, it
should be feasible.

Nathan or Jason, is there something like this?

Martin

Re: correct attribute ifunc C++ type safety (PR 82301)

[Martin Sebor]

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

On 09/28/2017 08:25 AM, Nathan Sidwell wrote:
> On 09/24/2017 06:03 PM, Martin Sebor wrote:
>> r253041 enhanced type checking for alias and ifunc attributes to
>> detect declarations of incompatible aliases, or ifunc resolvers
>> that return pointers to functions of an incompatible type.  More
>> extensive testing exposed a bug in the implementation of the ifunc
>> attribute handling in C++ where the checker expected the ifunc
>> resolver to return a pointer to a member function when the
>> implementation actually expects it return a pointer to a non-
>> member function.
>>
>> In a discussion of the test suite failures, Jakub also suggested
>> to break the enhanced warning out of -Wattributes and issue it
>> under a different option.
>>
>> The attached patch corrects the C++ problem and moves the warning
>> under -Wincompatible-pointer-types.  Since this is a C-only option,
>> the patch also enables for it C++.  Since the option is enabled by
>> default, the patch further requires -Wextra to issue the warning
>> for ifunc resolvers returning void*.  However, the patched checker
>> diagnoses other incompatibilities without it.
>>
>> Martin
>
> I find the maybe_diag_incompatible_alias function confusing.
>
>> +/* Check declaration of the type of ALIAS for compatibility with its
>> TARGET
>> +   (which may be an ifunc resolver) and issue a diagnostic when they are
>> +   not compatible according to language rules (plus a C++ extension for
>> +   non-static member functions).  */
>> +
>> +static void
>> +maybe_diag_incompatible_alias (tree alias, tree target)
>> +{
>> +  tree altype = TREE_TYPE (alias);
>> +  tree targtype = TREE_TYPE (target);
>> +
>> +  bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
>> +  if (ifunc)
>
> I think it might be clearer if this was broken out into a diag_ifunc
> function?  But see below ...

Thanks for the review.  I've updated the patch to incorporate
your suggestions.  My responses (mostly agreeing with your
comments or clarifying things, plus one question) are inline.

>
>> +    {
>> +      /* Handle attribute ifunc first.  */
>> +
>> +      tree funcptr = altype;
>> +
>> +      /* Set FUNCPTR to the type of the alias target.  If the type
>> +     is a non-static member function of class C, construct a type
>> +     of an ordinary function taking C* as the first argument,
>> +     followed by the member function argument list, and use it
>> +     instead to check for compatibilties.  FUNCPTR is used only
>> +     in diagnostics.  */
>
> This comment is self-contradictory.
>   1 Set FUNCPTR
>   2 Do some method-type shenanigans
>   3 Use it to check for incompatibilites
>   4 FUNCPTR is only used in diags
>
> Which of #3 and #4 is true?

Both.  It's used to control diagnostics (as opposed to something
else).  But the comment is from an earlier version of the patch
where the function body was still a part of its caller, so it's
redundant now that all the code in maybe_diag_incompatible_alias
is only used to control diagnostics.

>> +
>> +      if (TREE_CODE (altype) == METHOD_TYPE)
>> +    {
>
> IMHO put the description of the METHOD_TYPE chicanery inside the block
> doing it?  FWIW, although the change being made works on many (most?)
> ABIs, it's not formally correct and I think fails on some where 'this'
> is passed specially. You might want to note that?

Sure.  I've added a comment.

Since the original tests (where the resolver returns void*) pass
across the board I assume it must work for all supported ABIs.
Or is there some subtlety between the before and after code that
I'm missing?

>
>> +      tree rettype = TREE_TYPE (altype);
>> +      tree args = TYPE_ARG_TYPES (altype);
>> +      altype = build_function_type (rettype, args);
>> +      funcptr = altype;
>> +    }
>> +
>
>> +      if ((!FUNC_OR_METHOD_TYPE_P (targtype)
>> +           || (prototype_p (altype)
>> +           && prototype_p (targtype)
>> +           && !types_compatible_p (altype, targtype))))
>> +        {
>> +          funcptr = build_pointer_type (funcptr);
>> +
>> +          if (warning_at (DECL_SOURCE_LOCATION (target),
>> +                  OPT_Wincompatible_pointer_types,
>> +                  "%<ifunc%> resolver for %qD should return %qT",
>> +                  alias, funcptr))
>> +        inform (DECL_SOURCE_LOCATION (alias),
>> +            "resolver indirect function declared here");
>> +        }
>
> this block is almost the same as the non-ifunc block.  Surely they can
> be the same code? (by generalizing one of the cases until it turns into
> the other?)

The existing code does that but in this patch I made the warnings
and informational notes distinct.  It feels like a tossup between
parameterizing the code and making the flow more complex and harder
to follow and keeping the two cases (ifunc and alias) separate from
one another.  But I don't feel strongly one way or the other so
I changed it as you suggest.

>> +      /* Deal with static member function pointers.  */
>
> I do not understand this comment or condition. We seem to have dealt
> with pointers already and the conditions seem confused.
>
>> +      if (TREE_CODE (targtype) != RECORD_TYPE
>> +          || TYPE_FIELDS (targtype)
>> +          || TREE_CODE (TREE_TYPE (TYPE_FIELDS (targtype))) !=
>> POINTER_TYPE
>> +          || (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS (targtype))))
>> +          != METHOD_TYPE))
>
> if
>   not a record,
>   or has TYPE_FIELDS non-NULL
>   or the first field doesn't have pointer type (we can't get here)
>   or something else about the first field
>
> oh, I think it's trying to spot the pointer to NON-static member
> function internal record type.  But brokenly. I think pmf record_types
> have DECL_ARTIFICIAL and BUILTIN_LOCATION, that might be useful.

It turns out this code was superfluous with the C++ correction
and I was able to remove it with no impact on the tests.

>
>> +        {
>> +          funcptr = build_pointer_type (funcptr);
>> +
>> +          error ("%<ifunc%> resolver for %qD must return %qT",
>> +             alias, funcptr);
>> +
>> +          inform (DECL_SOURCE_LOCATION (alias),
>> +              "resolver indirect function declared here");
>> +        }
>> +    }
>> +
>
>> +  if ((!FUNC_OR_METHOD_TYPE_P (targtype)
>> +       || (prototype_p (altype)
>> +       && prototype_p (targtype)
>> +       && !types_compatible_p (altype, targtype))))
>
> Similar to above, already noted.
>
> Is this function called before we know whether we've enabled the
> appropriate warnings?  It would be better to bail out sooner if the
> warnings are disabled.

I'm not sure I understand the question or suggestion here but
warnings in general are certainly enabled at this point.  The
function issues both errors and warnings so it can't very well
exit early without checking the compatibility.  Let me know if
I misunderstood your comment.

Martin

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

PR c/82301 - Updated test case g++.dg/ext/attr-ifunc-1.C (and others) in r253041 segfault on powerpc64

gcc/ChangeLog:

	PR other/82301
	* cgraphunit.c (maybe_diag_incompatible_alias): New function.
	(handle_alias_pairs): Call it.
	* doc/extend.texi (ifunc attribute): Discuss C++ specifics.
	* doc/invoke.texi (-Wincompatible-pointer-types): Ditto.

gcc/c-family/ChangeLog:

	PR other/82301
	* c.opt (-Wincompatible-pointer-types): Enable for C++.

gcc/testsuite/ChangeLog:

	PR other/82301
	* g++.dg/ext/attr-ifunc-1.C: Update.
	* g++.dg/ext/attr-ifunc-2.C: Same.
	* g++.dg/ext/attr-ifunc-3.C: Same.
	* g++.dg/ext/attr-ifunc-4.C: Same.
	* g++.dg/ext/attr-ifunc-5.C: Same.
	* g++.dg/ext/attr-ifunc-6.C: New test.
	* g++.old-deja/g++.abi/vtable2.C: Update.
	* gcc.dg/attr-ifunc-6.c: New test.
	* gcc.dg/attr-ifunc-7.c: New test.
	* gcc.dg/pr81854.c: Update.
	* lib/target-supports.exp: Update.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 13d2a59..5d788e3 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -592,7 +592,7 @@ C C++ Var(warn_ignored_attributes) Init(1) Warning
 Warn whenever attributes are ignored.
 
 Wincompatible-pointer-types
-C ObjC Var(warn_incompatible_pointer_types) Init(1) Warning
+C C++ ObjC ObjcC++ Var(warn_incompatible_pointer_types) Init(1) Warning
 Warn when there is a conversion between pointers that have incompatible types.
 
 Winit-self
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 8c1acf7..b5076aa6 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1296,6 +1296,93 @@ analyze_functions (bool first_time)
   input_location = saved_loc;
 }
 
+/* Check declaration of the type of ALIAS for compatibility with its TARGET
+   (which may be an ifunc resolver) and issue a diagnostic when they are
+   not compatible according to language rules (plus a C++ extension for
+   non-static member functions).  */
+
+static void
+maybe_diag_incompatible_alias (tree alias, tree target)
+{
+  tree altype = TREE_TYPE (alias);
+  tree targtype = TREE_TYPE (target);
+
+  bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+  tree funcptr = altype;
+
+  if (ifunc)
+    {
+      /* Handle attribute ifunc first.  */
+      if (TREE_CODE (altype) == METHOD_TYPE)
+	{
+	  /* Set FUNCPTR to the type of the alias target.  If the type
+	     is a non-static member function of class C, construct a type
+	     of an ordinary function taking C* as the first argument,
+	     followed by the member function argument list, and use it
+	     instead to check for compatibilties.  This conversion is
+	     is not defined by the language but an extension provided
+	     by G++.  */
+
+	  tree rettype = TREE_TYPE (altype);
+	  tree args = TYPE_ARG_TYPES (altype);
+	  altype = build_function_type (rettype, args);
+	  funcptr = altype;
+	}
+
+      targtype = TREE_TYPE (targtype);
+
+      if (POINTER_TYPE_P (targtype))
+	{
+	  targtype = TREE_TYPE (targtype);
+
+	  /* Only issue Wincompatible-pointer-types for conversions
+	     to void* with -Wextra.  */
+	  if (VOID_TYPE_P (targtype) && !extra_warnings)
+	    return;
+
+	  /* Proceed to handle incompatible ifunc resolvers below.  */
+	}
+      else
+	{
+	  funcptr = build_pointer_type (funcptr);
+
+	  error_at (DECL_SOURCE_LOCATION (target),
+		    "%<ifunc%> resolver for %qD must return %qT",
+		 alias, funcptr);
+	  inform (DECL_SOURCE_LOCATION (alias),
+		  "resolver indirect function declared here");
+	  return;
+	}
+    }
+
+  if ((!FUNC_OR_METHOD_TYPE_P (targtype)
+       || (prototype_p (altype)
+	   && prototype_p (targtype)
+	   && !types_compatible_p (altype, targtype))))
+    {
+      /* Warn for incompatibilities.  Avoid warning for functions
+	 without a prototype to make it possible to declare aliases
+	 without knowing the exact type, as libstdc++ does.  */
+      if (ifunc)
+	{
+	  funcptr = build_pointer_type (funcptr);
+
+	  if (warning_at (DECL_SOURCE_LOCATION (target),
+			  OPT_Wincompatible_pointer_types,
+			  "%<ifunc%> resolver for %qD should return %qT",
+			  alias, funcptr))
+	    inform (DECL_SOURCE_LOCATION (alias),
+		    "resolver indirect function declared here");
+	}
+      else if (warning_at (DECL_SOURCE_LOCATION (alias),
+			   OPT_Wincompatible_pointer_types,
+			   "%qD alias between functions of incompatible "
+			   "types %qT and %qT", alias, altype, targtype))
+	inform (DECL_SOURCE_LOCATION (target),
+		"aliased declaration here");
+    }
+}
+
 /* Translate the ugly representation of aliases as alias pairs into nice
    representation in callgraph.  We don't handle all cases yet,
    unfortunately.  */
@@ -1305,7 +1392,7 @@ handle_alias_pairs (void)
 {
   alias_pair *p;
   unsigned i;
-  
+
   for (i = 0; alias_pairs && alias_pairs->iterate (i, &p);)
     {
       symtab_node *target_node = symtab_node::get_for_asmname (p->target);
@@ -1352,65 +1439,7 @@ handle_alias_pairs (void)
       if (TREE_CODE (p->decl) == FUNCTION_DECL
           && target_node && is_a <cgraph_node *> (target_node))
 	{
-	  tree t1 = TREE_TYPE (p->decl);
-	  tree t2 = TREE_TYPE (target_node->decl);
-
-	  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (p->decl)))
-	    {
-	      t2 = TREE_TYPE (t2);
-	      if (POINTER_TYPE_P (t2))
-		{
-		  t2 = TREE_TYPE (t2);
-		  if (!FUNC_OR_METHOD_TYPE_P (t2))
-		    {
-		      if (warning_at (DECL_SOURCE_LOCATION (p->decl),
-				      OPT_Wattributes,
-				      "%q+D %<ifunc%> resolver should return "
-				      "a function pointer",
-				      p->decl))
-			inform (DECL_SOURCE_LOCATION (target_node->decl),
-				"resolver declaration here");
-
-		      t2 = NULL_TREE;
-		    }
-		}
-	      else
-		{
-		  /* Deal with static member function pointers.  */
-		  if (TREE_CODE (t2) == RECORD_TYPE
-		      && TYPE_FIELDS (t2)
-		      && TREE_CODE (TREE_TYPE (TYPE_FIELDS (t2))) == POINTER_TYPE
-		      && (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS (t2))))
-			  == METHOD_TYPE))
-		    t2 = TREE_TYPE (TREE_TYPE (TYPE_FIELDS (t2)));
-		  else
-		    {
-		      error ("%q+D %<ifunc%> resolver must return a function "
-			     "pointer",
-			     p->decl);
-		      inform (DECL_SOURCE_LOCATION (target_node->decl),
-			      "resolver declaration here");
-
-		      t2 = NULL_TREE;
-		    }
-		}
-	    }
-
-	  if (t2
-	      && (!FUNC_OR_METHOD_TYPE_P (t2)
-		  || (prototype_p (t1)
-		      && prototype_p (t2)
-		      && !types_compatible_p (t1, t2))))
-	    {
-	      /* Warn for incompatibilities.  Avoid warning for functions
-		 without a prototype to make it possible to declare aliases
-		 without knowing the exact type, as libstdc++ does.  */
-	      if (warning_at (DECL_SOURCE_LOCATION (p->decl), OPT_Wattributes,
-			      "%q+D alias between functions of incompatible "
-			      "types %qT and %qT", p->decl, t1, t2))
-		inform (DECL_SOURCE_LOCATION (target_node->decl),
-			"aliased declaration here");
-	    }
+          maybe_diag_incompatible_alias (p->decl, target_node->decl);
 
 	  cgraph_node *src_node = cgraph_node::get (p->decl);
 	  if (src_node && src_node->definition)
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 9064561..e5f2a9a 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2801,7 +2801,7 @@ void *my_memcpy (void *dst, const void *src, size_t len)
 
 static void * (*resolve_memcpy (void))(void *, const void *, size_t)
 @{
-  return my_memcpy; // we'll just always select this routine
+  return my_memcpy; // we will just always select this routine
 @}
 @end smallexample
 
@@ -2814,15 +2814,56 @@ extern void *memcpy (void *, const void *, size_t);
 @end smallexample
 
 @noindent
-allowing the user to call this as a regular function, unaware of the
-implementation.  Finally, the indirect function needs to be defined in
-the same translation unit as the resolver function:
+allowing the user to call @code{memcpy} as a regular function, unaware of
+the actual implementation.  Finally, the indirect function needs to be
+defined in the same translation unit as the resolver function:
 
 @smallexample
 void *memcpy (void *, const void *, size_t)
      __attribute__ ((ifunc ("resolve_memcpy")));
 @end smallexample
 
+In C++, the @code{ifunc} attribute takes a string that is the mangled name
+of the resolver function.  A C++ resolver for a non-static member function
+of class @code{C} should be declared to return a pointer to a non-member
+function taking pointer to @code{C} as the first argument, followed by
+the same arguments as of the implementation function.  G++ checks
+the signatures of the two functions and issues
+a @option{-Wincompatible-pointer-types} warning for mismatches.  To suppress
+a warning for the necessary cast from a pointer to the implementation member
+function to the type of the corresponding non-member function use the
+@option{-Wno-pmf-conversions} option.  For example:
+
+@smallexample
+class S
+@{
+private:
+  int debug_impl (int);
+  int optimized_impl (int);
+
+  typedef int Func (S*, int);
+
+  static Func* resolver ();
+public:
+
+  int interface (int);
+@};
+
+int S::debug_impl (int) @{ /* @r{@dots{}} */ @}
+int S::optimized_impl (int) @{ /* @r{@dots{}} */ @}
+
+S::Func* S::resolver ()
+@{
+  int (S::*pimpl) (int)
+    = getenv ("DEBUG") ? &S::debug_impl : &S::optimized_impl;
+
+  // Cast triggers -Wno-pmf-conversions.
+  return reinterpret_cast<Func*>(pimpl);
+@}
+
+int S::interface (int) __attribute__ ((ifunc ("_ZN1S8resolverEv")));
+@end smallexample
+
 Indirect functions cannot be weak.  Binutils version 2.20.1 or higher
 and GNU C Library version 2.11.1 are required to use this feature.
 
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index f862b7f..dbf9d69 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -5456,13 +5456,16 @@ are being discarded. Typically, the compiler warns if a
 takes a @code{int (*)[]} parameter.  This option can be used to
 suppress such a warning.
 
-@item -Wno-incompatible-pointer-types @r{(C and Objective-C only)}
+@item -Wno-incompatible-pointer-types
 @opindex Wno-incompatible-pointer-types
 @opindex Wincompatible-pointer-types
 Do not warn when there is a conversion between pointers that have incompatible
 types.  This warning is for cases not covered by @option{-Wno-pointer-sign},
 which warns for pointer argument passing or assignment with different
-signedness.
+signedness.  In C++ where incompatible pointer conversions ordinarily cause
+errors, the warning detects such conversions in GCC extensions that aren't
+part of the standard language.  An example of a construct that might perform
+such a conversion is the @code{alias} attribute.  @xref{Function Attributes,,Declaring Attributes of Functions}.
 
 @item -Wno-int-conversion @r{(C and Objective-C only)}
 @opindex Wno-int-conversion
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-1.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-1.C
index 2c7bba1..4a29e8b 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-1.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-1.C
@@ -4,26 +4,33 @@
 
 struct Klass
 {
+  int a[4];
+
   int implementation ();
   int magic ();
 
-  typedef int (Klass::*MemFuncPtr)();
+  /* An ifunc resolver must return a pointer to an ordinary (non-member)
+     function.  To make it possible to use ifunc with member functions,
+     the resolver must convert a member function pointer to an ordinary
+     function pointer (slicing off the high word).  */
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
-Klass::MemFuncPtr p = &Klass::implementation;
-
-int Klass::implementation (void)
+int Klass::implementation ()
 {
   __builtin_printf ("'ere I am JH\n");
-  return 1234;
+  return a[0] + a[1] + a[2] + a[3];
 }
 
-
-Klass::MemFuncPtr Klass::resolver (void)
+Klass::Func* Klass::resolver (void)
 {
-  return &Klass::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klass::implementation);
 }
 
 int f (void) __attribute__ ((ifunc ("foo")));
@@ -32,11 +39,16 @@ typedef int (F)(void);
 extern "C" F* foo () { return 0; }
 
 
-int Klass::magic (void) __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
+int Klass::magic () __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
 
 int main ()
 {
   Klass obj;
 
-  return !(obj.magic () == 1234);
+  obj.a[0] = 1;
+  obj.a[1] = 2;
+  obj.a[2] = 3;
+  obj.a[3] = 4;
+
+  return !(obj.magic () == 10);
 }
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-2.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-2.C
index 1fc940b..e5be3d2 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-2.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-2.C
@@ -9,9 +9,9 @@ struct Klass
   int implementation ();
   int magic ();
 
-  typedef int (Klass::*MemFuncPtr)();
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
 int Klass::implementation (void)
@@ -20,9 +20,13 @@ int Klass::implementation (void)
   return 0;
 }
 
-Klass::MemFuncPtr Klass::resolver (void)
+Klass::Func* Klass::resolver (void)
 {
-  return &Klass::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klass::implementation);
 }
 
 int Klass::magic (void) __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-3.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-3.C
index 04206a1..6d49424 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-3.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-3.C
@@ -6,23 +6,29 @@
 
 struct Klass
 {
+  int a[4];
+
   int implementation ();
   int magic ();
 
-  typedef int (Klass::*MemFuncPtr)();
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
 int Klass::implementation (void)
 {
   printf ("'ere I am JH\n");
-  return 0;
+  return a[0] + a[1] + a[2] + a[3];
 }
 
-Klass::MemFuncPtr Klass::resolver (void)
+Klass::Func* Klass::resolver ()
 {
-  return &Klass::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klass::implementation);
 }
 
 int Klass::magic (void) __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
@@ -36,5 +42,10 @@ int main ()
 {
   Klass obj;
 
-  return Foo (obj, &Klass::magic) != 0;
+  obj.a[0] = 1;
+  obj.a[1] = 2;
+  obj.a[2] = 3;
+  obj.a[3] = 4;
+
+  return Foo (obj, &Klass::magic) != 10;
 }
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-4.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-4.C
index 3127193..f71dc3b 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-4.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-4.C
@@ -14,9 +14,9 @@ struct Klassier : Klass
   int implementation ();
   int magic ();
 
-  typedef int (Klassier::*MemFuncPtr)();
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
 int Klassier::implementation (void)
@@ -25,9 +25,13 @@ int Klassier::implementation (void)
   return 0;
 }
 
-Klassier::MemFuncPtr Klassier::resolver (void)
+Klassier::Func* Klassier::resolver ()
 {
-  return &Klassier::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klassier::implementation);
 }
 
 int Klassier::magic (void) __attribute__ ((ifunc ("_ZN8Klassier8resolverEv")));
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-5.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-5.C
index 05855dd..fd8bcff 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-5.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-5.C
@@ -1,15 +1,21 @@
 // PR c/81854 - weak alias of an incompatible symbol accepted
 // { dg-do compile }
 // { dg-require-ifunc "" } */
+// { dg-options "-Wextra -Wno-pmf-conversions" }
 
 struct Klass
 {
   int implementation ();
-  const char* magic ();
+  int good_magic ();
+  int iffy_magic ();
+  const char* bad_magic ();
 
+  typedef int (Func)(Klass*);
   typedef int (Klass::*MemFuncPtr)();
 
-  static MemFuncPtr resolver ();
+  static Func* good_resolver ();
+  static void* iffy_resolver ();
+  static MemFuncPtr bad_resolver ();
 };
 
 int Klass::implementation (void)
@@ -17,13 +23,42 @@ int Klass::implementation (void)
   return 0;
 }
 
-const char* __attribute__ ((ifunc ("_ZN5Klass8resolverEv")))
-  Klass::magic ();   // { dg-warning "alias between functions of incompatible types" }
+// Verify no warning for the expected/compatible declaration.
 
+int __attribute__ ((ifunc ("_ZN5Klass13good_resolverEv")))
+Klass::good_magic ();
+
+Klass::Func*
+Klass::good_resolver (void)
+{
+  MemFuncPtr mfp = &Klass::implementation;
+
+  return reinterpret_cast<Func*>(mfp);
+}
+
+
+// Verify a warning for the unsafe declaration.
+
+int __attribute__ ((ifunc ("_ZN5Klass13iffy_resolverEv")))
+Klass::iffy_magic ();    // { dg-message "resolver indirect function declared here" }
+
+void*
+Klass::iffy_resolver (void)   // { dg-warning ".ifunc. resolver for .int Klass::iffy_magic\\(\\). should return .int \\(\\*\\)\\(Klass\\*\\)." }
+{
+  MemFuncPtr mfp = &Klass::implementation;
+
+  return reinterpret_cast<void*>(mfp);
+}
+
+
+// Verify an error for an incompatible declaration.
+
+const char* __attribute__ ((ifunc ("_ZN5Klass12bad_resolverEv")))
+Klass::bad_magic ();   // { dg-message "resolver indirect function declared here" }
 
 
 Klass::MemFuncPtr
-Klass::resolver (void) // { dg-message "aliased declaration here" }
+Klass::bad_resolver (void)   // { dg-error ".ifunc. resolver for .const char\\* Klass::bad_magic\\(\\). must return .const char\\* \\(\\*\\)\\(Klass\\*\\)." }
 {
   return &Klass::implementation;
 }
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-6.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-6.C
new file mode 100644
index 0000000..918123d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-6.C
@@ -0,0 +1,81 @@
+// PR c/81854 - weak alias of an incompatible symbol accepted
+// { dg-do run }
+// { dg-require-ifunc "" }
+// { dg-options "-Wno-pmf-conversions" }
+
+struct Klass
+{
+  int a[4];
+
+  int implementation (int);
+  int implementation (int, long, const char*);
+
+  int magic (int);
+  int magic (int, long, const char *);
+
+  /* An ifunc resolver must return a pointer to an ordinary (non-member)
+     function.  To make it possible to use ifunc with member functions,
+     the resolver must convert a member function pointer to an ordinary
+     function pointer (slicing off the high word).  */
+  typedef int Func1 (Klass*, int);
+  typedef int Func3 (Klass*, int, long, const char*);
+
+  template <class Func>
+  static Func* resolver ();
+};
+
+const char *str;
+
+int Klass::implementation (int x)
+{
+  return a[0] + a[1] + a[2] + a[3] + x;
+}
+
+int Klass::implementation (int x, long y, const char *s)
+{
+  return a[0] + a[1] + a[2] + a[3] + x + y + !(s == str);
+}
+
+template <>
+Klass::Func1* Klass::resolver<Klass::Func1> ()
+{
+  int (Klass::*pmf)(int) = &Klass::implementation;
+
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  The cast triggers -Wno-pmf-conversions which
+     the test suppresses.  */
+
+  return reinterpret_cast<Func1*>(pmf);
+}
+
+template <>
+Klass::Func3* Klass::resolver<Klass::Func3> ()
+{
+  int (Klass::*pmf)(int, long, const char*) = &Klass::implementation;
+
+  return reinterpret_cast<Func3*>(pmf);
+}
+
+__attribute__ ((ifunc ("_ZN5Klass8resolverIFiPS_iEEEPT_v")))
+int Klass::magic (int);
+
+__attribute__ ((ifunc ("_ZN5Klass8resolverIFiPS_ilPKcEEEPT_v")))
+int Klass::magic (int, long, const char *);
+
+int main ()
+{
+  Klass obj;
+
+  obj.a[0] = 1;
+  obj.a[1] = 2;
+  obj.a[2] = 3;
+  obj.a[3] = 4;
+
+  str = "teststring";
+
+  // Verify that data members and function arguments are correctly
+  // retrieved.
+  if (obj.magic (5) != 15 || obj.magic (5, 6, str) != 21)
+    __builtin_abort ();
+}
diff --git a/gcc/testsuite/g++.old-deja/g++.abi/vtable2.C b/gcc/testsuite/g++.old-deja/g++.abi/vtable2.C
index 2c88a95..804fc4b 100644
--- a/gcc/testsuite/g++.old-deja/g++.abi/vtable2.C
+++ b/gcc/testsuite/g++.old-deja/g++.abi/vtable2.C
@@ -1,5 +1,5 @@
 // { dg-do run  }
-// { dg-options "-Wno-attributes -fno-strict-aliasing" }
+// { dg-options "-Wincompatible-pointer-types -fno-strict-aliasing" }
 // Origin: Mark Mitchell <mark@codesourcery.com>
 
 #if defined (__GXX_ABI_VERSION) && __GXX_ABI_VERSION >= 100
diff --git a/gcc/testsuite/gcc.dg/attr-ifunc-6.c b/gcc/testsuite/gcc.dg/attr-ifunc-6.c
new file mode 100644
index 0000000..2af82ab
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-ifunc-6.c
@@ -0,0 +1,31 @@
+/* { dg-do compile }  */
+/* { dg-require-ifunc "" } */
+/* { dg-options "" } */
+
+static int implementation (void)
+{
+  return 0;
+}
+
+/* Verify that a resolver that returns void* is not diagnosed without
+   -Wextra (see attr-ifunc-7.c for a test that verifies that -Wextra
+   triggers the warning).  */
+static void* resolver_nowarn (void)
+{
+  return implementation;
+}
+
+extern int
+indirect_nowarn (void) __attribute__ ((ifunc ("resolver_nowarn")));
+
+
+/* Verify that a resolver that returns a T* that's not void* is diagnosed
+   even without -Wextra.  */
+static int* resolver_warn (void)   /* { dg-warning ".ifunc. resolver for .indirect_warn. should return 'int \\(\\*\\)\\(void\\)." } */
+{
+  return (int *)implementation;
+}
+
+extern int
+indirect_warn (void)   /* { dg-message "" } */
+__attribute__ ((ifunc ("resolver_warn")));
diff --git a/gcc/testsuite/gcc.dg/attr-ifunc-7.c b/gcc/testsuite/gcc.dg/attr-ifunc-7.c
new file mode 100644
index 0000000..a8e18ed
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-ifunc-7.c
@@ -0,0 +1,30 @@
+/* { dg-do compile }  */
+/* { dg-require-ifunc "" } */
+/* { dg-options "-Wextra" } */
+
+static int implementation (void)
+{
+  return 0;
+}
+
+/* Verify that a resolver that returns void* is diagnosed with -Wextra.  */
+static void* resolver_warn (void)   /* { dg-warning ".ifunc. resolver for .indirect_warn. should return .int \\(\\*\\)\\(void\\)." } */
+{
+  return implementation;
+}
+
+extern __attribute__ ((ifunc ("resolver_warn"))) int
+indirect_warn (void);   /* { dg-message "resolver indirect function declared here" } */
+
+
+
+/* Verify that a resolver that returns int* is still diagnosed, even
+   with -Wextra.  */
+static int* resolver_warn_2 (void)   /* { dg-warning ".ifunc. resolver for .indirect_warn_2. should return .int \\(\\*\\)\\(void\\)." } */
+{
+  return (int *)implementation;
+}
+
+extern __attribute__ ((ifunc ("resolver_warn_2"))) int
+indirect_warn_2 (void);   /* { dg-message "resolver indirect function declared here" } */
+
diff --git a/gcc/testsuite/gcc.dg/pr81854.c b/gcc/testsuite/gcc.dg/pr81854.c
index b8499f8..1021a81 100644
--- a/gcc/testsuite/gcc.dg/pr81854.c
+++ b/gcc/testsuite/gcc.dg/pr81854.c
@@ -1,6 +1,7 @@
 /* PR c/81854 - weak alias of an incompatible symbol accepted
    { dg-do compile }
-   { dg-require-ifunc "" } */
+   { dg-require-ifunc "" }
+   { dg-options "-Wextra" } */
 
 const char* __attribute__ ((weak, alias ("f0_target")))
 f0 (void);          /* { dg-error "alias between function and variable" } */
@@ -26,39 +27,37 @@ const char* f2_target (int i)   /* { dg-message "aliased declaration here" } */
   return 0;
 }
 
-
 int __attribute__ ((ifunc ("f3_resolver")))
-f3 (void);          /* { dg-error ".ifunc. resolver must return a function pointer" } */
+f3 (void);          /* { dg-message "resolver indirect function declared here" } */
 
-int f3_resolver (void)   /* { dg-message "resolver declaration here" } */
+void* f3_resolver (void) /* { dg-warning "ifunc. resolver for .f3. should return .int \\(\\*\\)\\(void\\)." } */
 {
   return 0;
 }
 
 
 int __attribute__ ((ifunc ("f4_resolver")))
-f4 (void);          /* { dg-warning ".ifunc. resolver should return a function pointer" } */
+f4 (void);          /* { dg-message "resolver indirect function declared here" } */
 
-void* f4_resolver (void) /* { dg-message "resolver declaration here" } */
+typedef void F4 (void);
+F4* f4_resolver (void) /* { dg-warning ".ifunc. resolver for .f4. should return .int \\(\\*\\)\\(void\\)" } */
 {
   return 0;
 }
 
+const char* __attribute__ ((ifunc ("f5_resolver")))
+f5 (void);
 
-int __attribute__ ((ifunc ("f5_resolver")))
-f5 (void);          /* { dg-warning "alias between functions of incompatible types" } */
-
-typedef void F5 (void);
-F5* f5_resolver (void) /* { dg-message "aliased declaration here" } */
+typedef const char* F5 (void);
+F5* f5_resolver (void)
 {
   return 0;
 }
 
-const char* __attribute__ ((ifunc ("f6_resolver")))
-f6 (void);
+int __attribute__ ((ifunc ("f6_resolver")))
+f6 (void);          /* { dg-message "resolver indirect function declared here" } */
 
-typedef const char* F6 (void);
-F6* f6_resolver (void)
+int f6_resolver (void)   /* { dg-error ".ifunc. resolver for 'f6' must return .int \\(\\*\\)\\(void\\)." } */
 {
   return 0;
 }
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 57f646c..cf312a1 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -440,8 +440,8 @@ proc check_ifunc_available { } {
 	extern "C" {
 	#endif
 	typedef void F (void);
-	F* g() {}
-	void f() __attribute__((ifunc("g")));
+	F* g (void) {}
+	void f () __attribute__ ((ifunc ("g")));
 	#ifdef __cplusplus
 	}
 	#endif

Re: correct attribute ifunc C++ type safety (PR 82301)

[Nathan Sidwell]

On 10/04/2017 03:40 PM, Martin Sebor wrote:
> On 09/28/2017 08:25 AM, Nathan Sidwell wrote:


> Since the original tests (where the resolver returns void*) pass
> across the board I assume it must work for all supported ABIs.
> Or is there some subtlety between the before and after code that
> I'm missing?

I had a vague notion of some (IBM?) target that might do something 
different.  Perhaps it's gone.  Your comment explains it fine.

>> oh, I think it's trying to spot the pointer to NON-static member
>> function internal record type.  But brokenly. I think pmf record_types
>> have DECL_ARTIFICIAL and BUILTIN_LOCATION, that might be useful.
> 
> It turns out this code was superfluous with the C++ correction
> and I was able to remove it with no impact on the tests.

Great.

>> Is this function called before we know whether we've enabled the
>> appropriate warnings?  It would be better to bail out sooner if the
>> warnings are disabled.
> 
> I'm not sure I understand the question or suggestion here but
> warnings in general are certainly enabled at this point.  The
> function issues both errors and warnings so it can't very well
> exit early without checking the compatibility.  Let me know if
> I misunderstood your comment.

Oh, forgot it issued errors too, so my queston is moot.

> -signedness.
> +signedness.  In C++ where incompatible pointer conversions ordinarily cause
Missing comma:  In C++, where incompatible ...

> +errors, the warning detects such conversions in GCC extensions that aren't
> +part of the standard language.  An example of a construct that might perform
> +such a conversion is the @code{alias} attribute.  @xref{Function Attributes,,Declaring Attributes of Functions}.

Looks fine to me.  (pedantically, I don't think I can formally approve 
this, because it's not part of the C++ FE.)

nathan
-- 
Nathan Sidwell

[PING] Re: correct attribute ifunc C++ type safety (PR 82301)

[Martin Sebor]

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

Ping?

Since I submitted the updated patch it has been suggested to me
that providing a new option to control the warning rather than
using an existing one would be preferable (see bug 82435 for
the background).  The attached update to the patch adds
-Wattribute-alias for this purpose and restores the original
disposition for -Wincompatible-pointer-types.

Thanks
Martin

On 10/04/2017 01:40 PM, Martin Sebor wrote:
> On 09/28/2017 08:25 AM, Nathan Sidwell wrote:
>> On 09/24/2017 06:03 PM, Martin Sebor wrote:
>>> r253041 enhanced type checking for alias and ifunc attributes to
>>> detect declarations of incompatible aliases, or ifunc resolvers
>>> that return pointers to functions of an incompatible type.  More
>>> extensive testing exposed a bug in the implementation of the ifunc
>>> attribute handling in C++ where the checker expected the ifunc
>>> resolver to return a pointer to a member function when the
>>> implementation actually expects it return a pointer to a non-
>>> member function.
>>>
>>> In a discussion of the test suite failures, Jakub also suggested
>>> to break the enhanced warning out of -Wattributes and issue it
>>> under a different option.
>>>
>>> The attached patch corrects the C++ problem and moves the warning
>>> under -Wincompatible-pointer-types.  Since this is a C-only option,
>>> the patch also enables for it C++.  Since the option is enabled by
>>> default, the patch further requires -Wextra to issue the warning
>>> for ifunc resolvers returning void*.  However, the patched checker
>>> diagnoses other incompatibilities without it.
>>>
>>> Martin
>>
>> I find the maybe_diag_incompatible_alias function confusing.
>>
>>> +/* Check declaration of the type of ALIAS for compatibility with its
>>> TARGET
>>> +   (which may be an ifunc resolver) and issue a diagnostic when they
>>> are
>>> +   not compatible according to language rules (plus a C++ extension for
>>> +   non-static member functions).  */
>>> +
>>> +static void
>>> +maybe_diag_incompatible_alias (tree alias, tree target)
>>> +{
>>> +  tree altype = TREE_TYPE (alias);
>>> +  tree targtype = TREE_TYPE (target);
>>> +
>>> +  bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
>>> +  if (ifunc)
>>
>> I think it might be clearer if this was broken out into a diag_ifunc
>> function?  But see below ...
>
> Thanks for the review.  I've updated the patch to incorporate
> your suggestions.  My responses (mostly agreeing with your
> comments or clarifying things, plus one question) are inline.
>
>>
>>> +    {
>>> +      /* Handle attribute ifunc first.  */
>>> +
>>> +      tree funcptr = altype;
>>> +
>>> +      /* Set FUNCPTR to the type of the alias target.  If the type
>>> +     is a non-static member function of class C, construct a type
>>> +     of an ordinary function taking C* as the first argument,
>>> +     followed by the member function argument list, and use it
>>> +     instead to check for compatibilties.  FUNCPTR is used only
>>> +     in diagnostics.  */
>>
>> This comment is self-contradictory.
>>   1 Set FUNCPTR
>>   2 Do some method-type shenanigans
>>   3 Use it to check for incompatibilites
>>   4 FUNCPTR is only used in diags
>>
>> Which of #3 and #4 is true?
>
> Both.  It's used to control diagnostics (as opposed to something
> else).  But the comment is from an earlier version of the patch
> where the function body was still a part of its caller, so it's
> redundant now that all the code in maybe_diag_incompatible_alias
> is only used to control diagnostics.
>
>>> +
>>> +      if (TREE_CODE (altype) == METHOD_TYPE)
>>> +    {
>>
>> IMHO put the description of the METHOD_TYPE chicanery inside the block
>> doing it?  FWIW, although the change being made works on many (most?)
>> ABIs, it's not formally correct and I think fails on some where 'this'
>> is passed specially. You might want to note that?
>
> Sure.  I've added a comment.
>
> Since the original tests (where the resolver returns void*) pass
> across the board I assume it must work for all supported ABIs.
> Or is there some subtlety between the before and after code that
> I'm missing?
>
>>
>>> +      tree rettype = TREE_TYPE (altype);
>>> +      tree args = TYPE_ARG_TYPES (altype);
>>> +      altype = build_function_type (rettype, args);
>>> +      funcptr = altype;
>>> +    }
>>> +
>>
>>> +      if ((!FUNC_OR_METHOD_TYPE_P (targtype)
>>> +           || (prototype_p (altype)
>>> +           && prototype_p (targtype)
>>> +           && !types_compatible_p (altype, targtype))))
>>> +        {
>>> +          funcptr = build_pointer_type (funcptr);
>>> +
>>> +          if (warning_at (DECL_SOURCE_LOCATION (target),
>>> +                  OPT_Wincompatible_pointer_types,
>>> +                  "%<ifunc%> resolver for %qD should return %qT",
>>> +                  alias, funcptr))
>>> +        inform (DECL_SOURCE_LOCATION (alias),
>>> +            "resolver indirect function declared here");
>>> +        }
>>
>> this block is almost the same as the non-ifunc block.  Surely they can
>> be the same code? (by generalizing one of the cases until it turns into
>> the other?)
>
> The existing code does that but in this patch I made the warnings
> and informational notes distinct.  It feels like a tossup between
> parameterizing the code and making the flow more complex and harder
> to follow and keeping the two cases (ifunc and alias) separate from
> one another.  But I don't feel strongly one way or the other so
> I changed it as you suggest.
>
>>> +      /* Deal with static member function pointers.  */
>>
>> I do not understand this comment or condition. We seem to have dealt
>> with pointers already and the conditions seem confused.
>>
>>> +      if (TREE_CODE (targtype) != RECORD_TYPE
>>> +          || TYPE_FIELDS (targtype)
>>> +          || TREE_CODE (TREE_TYPE (TYPE_FIELDS (targtype))) !=
>>> POINTER_TYPE
>>> +          || (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS
>>> (targtype))))
>>> +          != METHOD_TYPE))
>>
>> if
>>   not a record,
>>   or has TYPE_FIELDS non-NULL
>>   or the first field doesn't have pointer type (we can't get here)
>>   or something else about the first field
>>
>> oh, I think it's trying to spot the pointer to NON-static member
>> function internal record type.  But brokenly. I think pmf record_types
>> have DECL_ARTIFICIAL and BUILTIN_LOCATION, that might be useful.
>
> It turns out this code was superfluous with the C++ correction
> and I was able to remove it with no impact on the tests.
>
>>
>>> +        {
>>> +          funcptr = build_pointer_type (funcptr);
>>> +
>>> +          error ("%<ifunc%> resolver for %qD must return %qT",
>>> +             alias, funcptr);
>>> +
>>> +          inform (DECL_SOURCE_LOCATION (alias),
>>> +              "resolver indirect function declared here");
>>> +        }
>>> +    }
>>> +
>>
>>> +  if ((!FUNC_OR_METHOD_TYPE_P (targtype)
>>> +       || (prototype_p (altype)
>>> +       && prototype_p (targtype)
>>> +       && !types_compatible_p (altype, targtype))))
>>
>> Similar to above, already noted.
>>
>> Is this function called before we know whether we've enabled the
>> appropriate warnings?  It would be better to bail out sooner if the
>> warnings are disabled.
>
> I'm not sure I understand the question or suggestion here but
> warnings in general are certainly enabled at this point.  The
> function issues both errors and warnings so it can't very well
> exit early without checking the compatibility.  Let me know if
> I misunderstood your comment.
>
> Martin


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

PR c/82301 - Updated test case g++.dg/ext/attr-ifunc-1.C (and others) in r253041 segfault on powerpc64
PR c/82435 - new __attribute__((alias)) warning gets in the way

gcc/ChangeLog:

	PR other/82301
	PR c/82435
	* cgraphunit.c (maybe_diag_incompatible_alias): New function.
	(handle_alias_pairs): Call it.
	* common.opt (-Wattribute-alias): New option.
	* doc/extend.texi (ifunc attribute): Discuss C++ specifics.
	* doc/invoke.texi (-Wattribute-alias): Document.

gcc/testsuite/ChangeLog:

	PR other/82301
	PR c/82435
	* g++.dg/ext/attr-ifunc-1.C: Update.
	* g++.dg/ext/attr-ifunc-2.C: Same.
	* g++.dg/ext/attr-ifunc-3.C: Same.
	* g++.dg/ext/attr-ifunc-4.C: Same.
	* g++.dg/ext/attr-ifunc-5.C: Same.
	* g++.dg/ext/attr-ifunc-6.C: New test.
	* g++.old-deja/g++.abi/vtable2.C: Update.
	* gcc.dg/attr-ifunc-6.c: New test.
	* gcc.dg/attr-ifunc-7.c: New test.
	* gcc.dg/pr81854.c: Update.
	* lib/target-supports.exp: Update.

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 8c1acf7..4f68790 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -1296,6 +1296,93 @@ analyze_functions (bool first_time)
   input_location = saved_loc;
 }
 
+/* Check declaration of the type of ALIAS for compatibility with its TARGET
+   (which may be an ifunc resolver) and issue a diagnostic when they are
+   not compatible according to language rules (plus a C++ extension for
+   non-static member functions).  */
+
+static void
+maybe_diag_incompatible_alias (tree alias, tree target)
+{
+  tree altype = TREE_TYPE (alias);
+  tree targtype = TREE_TYPE (target);
+
+  bool ifunc = lookup_attribute ("ifunc", DECL_ATTRIBUTES (alias));
+  tree funcptr = altype;
+
+  if (ifunc)
+    {
+      /* Handle attribute ifunc first.  */
+      if (TREE_CODE (altype) == METHOD_TYPE)
+	{
+	  /* Set FUNCPTR to the type of the alias target.  If the type
+	     is a non-static member function of class C, construct a type
+	     of an ordinary function taking C* as the first argument,
+	     followed by the member function argument list, and use it
+	     instead to check for incompatibility.  This conversion is
+	     not defined by the language but an extension provided by
+	     G++.  */
+
+	  tree rettype = TREE_TYPE (altype);
+	  tree args = TYPE_ARG_TYPES (altype);
+	  altype = build_function_type (rettype, args);
+	  funcptr = altype;
+	}
+
+      targtype = TREE_TYPE (targtype);
+
+      if (POINTER_TYPE_P (targtype))
+	{
+	  targtype = TREE_TYPE (targtype);
+
+	  /* Only issue Wattribute-alias for conversions to void* with
+	     -Wextra.  */
+	  if (VOID_TYPE_P (targtype) && !extra_warnings)
+	    return;
+
+	  /* Proceed to handle incompatible ifunc resolvers below.  */
+	}
+      else
+	{
+	  funcptr = build_pointer_type (funcptr);
+
+	  error_at (DECL_SOURCE_LOCATION (target),
+		    "%<ifunc%> resolver for %qD must return %qT",
+		 alias, funcptr);
+	  inform (DECL_SOURCE_LOCATION (alias),
+		  "resolver indirect function declared here");
+	  return;
+	}
+    }
+
+  if ((!FUNC_OR_METHOD_TYPE_P (targtype)
+       || (prototype_p (altype)
+	   && prototype_p (targtype)
+	   && !types_compatible_p (altype, targtype))))
+    {
+      /* Warn for incompatibilities.  Avoid warning for functions
+	 without a prototype to make it possible to declare aliases
+	 without knowing the exact type, as libstdc++ does.  */
+      if (ifunc)
+	{
+	  funcptr = build_pointer_type (funcptr);
+
+	  if (warning_at (DECL_SOURCE_LOCATION (target),
+			  OPT_Wattribute_alias,
+			  "%<ifunc%> resolver for %qD should return %qT",
+			  alias, funcptr))
+	    inform (DECL_SOURCE_LOCATION (alias),
+		    "resolver indirect function declared here");
+	}
+      else if (warning_at (DECL_SOURCE_LOCATION (alias),
+			   OPT_Wattribute_alias,
+			   "%qD alias between functions of incompatible "
+			   "types %qT and %qT", alias, altype, targtype))
+	inform (DECL_SOURCE_LOCATION (target),
+		"aliased declaration here");
+    }
+}
+
 /* Translate the ugly representation of aliases as alias pairs into nice
    representation in callgraph.  We don't handle all cases yet,
    unfortunately.  */
@@ -1305,7 +1392,7 @@ handle_alias_pairs (void)
 {
   alias_pair *p;
   unsigned i;
-  
+
   for (i = 0; alias_pairs && alias_pairs->iterate (i, &p);)
     {
       symtab_node *target_node = symtab_node::get_for_asmname (p->target);
@@ -1352,65 +1439,7 @@ handle_alias_pairs (void)
       if (TREE_CODE (p->decl) == FUNCTION_DECL
           && target_node && is_a <cgraph_node *> (target_node))
 	{
-	  tree t1 = TREE_TYPE (p->decl);
-	  tree t2 = TREE_TYPE (target_node->decl);
-
-	  if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (p->decl)))
-	    {
-	      t2 = TREE_TYPE (t2);
-	      if (POINTER_TYPE_P (t2))
-		{
-		  t2 = TREE_TYPE (t2);
-		  if (!FUNC_OR_METHOD_TYPE_P (t2))
-		    {
-		      if (warning_at (DECL_SOURCE_LOCATION (p->decl),
-				      OPT_Wattributes,
-				      "%q+D %<ifunc%> resolver should return "
-				      "a function pointer",
-				      p->decl))
-			inform (DECL_SOURCE_LOCATION (target_node->decl),
-				"resolver declaration here");
-
-		      t2 = NULL_TREE;
-		    }
-		}
-	      else
-		{
-		  /* Deal with static member function pointers.  */
-		  if (TREE_CODE (t2) == RECORD_TYPE
-		      && TYPE_FIELDS (t2)
-		      && TREE_CODE (TREE_TYPE (TYPE_FIELDS (t2))) == POINTER_TYPE
-		      && (TREE_CODE (TREE_TYPE (TREE_TYPE (TYPE_FIELDS (t2))))
-			  == METHOD_TYPE))
-		    t2 = TREE_TYPE (TREE_TYPE (TYPE_FIELDS (t2)));
-		  else
-		    {
-		      error ("%q+D %<ifunc%> resolver must return a function "
-			     "pointer",
-			     p->decl);
-		      inform (DECL_SOURCE_LOCATION (target_node->decl),
-			      "resolver declaration here");
-
-		      t2 = NULL_TREE;
-		    }
-		}
-	    }
-
-	  if (t2
-	      && (!FUNC_OR_METHOD_TYPE_P (t2)
-		  || (prototype_p (t1)
-		      && prototype_p (t2)
-		      && !types_compatible_p (t1, t2))))
-	    {
-	      /* Warn for incompatibilities.  Avoid warning for functions
-		 without a prototype to make it possible to declare aliases
-		 without knowing the exact type, as libstdc++ does.  */
-	      if (warning_at (DECL_SOURCE_LOCATION (p->decl), OPT_Wattributes,
-			      "%q+D alias between functions of incompatible "
-			      "types %qT and %qT", p->decl, t1, t2))
-		inform (DECL_SOURCE_LOCATION (target_node->decl),
-			"aliased declaration here");
-	    }
+          maybe_diag_incompatible_alias (p->decl, target_node->decl);
 
 	  cgraph_node *src_node = cgraph_node::get (p->decl);
 	  if (src_node && src_node->definition)
diff --git a/gcc/common.opt b/gcc/common.opt
index dfde6ad..69abe10 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -562,6 +562,10 @@ Wattributes
 Common Var(warn_attributes) Init(1) Warning
 Warn about inappropriate attribute usage.
 
+Wattribute-alias
+Common Var(warn_attributes) Init(1) Warning
+Warn about type safety and similar errors in attribute alias and related.
+
 Wcast-align
 Common Var(warn_cast_align) Warning
 Warn about pointer casts which increase alignment.
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 9064561..ecef39a 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2801,7 +2801,7 @@ void *my_memcpy (void *dst, const void *src, size_t len)
 
 static void * (*resolve_memcpy (void))(void *, const void *, size_t)
 @{
-  return my_memcpy; // we'll just always select this routine
+  return my_memcpy; // we will just always select this routine
 @}
 @end smallexample
 
@@ -2814,15 +2814,56 @@ extern void *memcpy (void *, const void *, size_t);
 @end smallexample
 
 @noindent
-allowing the user to call this as a regular function, unaware of the
-implementation.  Finally, the indirect function needs to be defined in
-the same translation unit as the resolver function:
+allowing the user to call @code{memcpy} as a regular function, unaware of
+the actual implementation.  Finally, the indirect function needs to be
+defined in the same translation unit as the resolver function:
 
 @smallexample
 void *memcpy (void *, const void *, size_t)
      __attribute__ ((ifunc ("resolve_memcpy")));
 @end smallexample
 
+In C++, the @code{ifunc} attribute takes a string that is the mangled name
+of the resolver function.  A C++ resolver for a non-static member function
+of class @code{C} should be declared to return a pointer to a non-member
+function taking pointer to @code{C} as the first argument, followed by
+the same arguments as of the implementation function.  G++ checks
+the signatures of the two functions and issues
+a @option{-Wattribute-alias} warning for mismatches.  To suppress a warning
+for the necessary cast from a pointer to the implementation member function
+to the type of the corresponding non-member function use
+the @option{-Wno-pmf-conversions} option.  For example:
+
+@smallexample
+class S
+@{
+private:
+  int debug_impl (int);
+  int optimized_impl (int);
+
+  typedef int Func (S*, int);
+
+  static Func* resolver ();
+public:
+
+  int interface (int);
+@};
+
+int S::debug_impl (int) @{ /* @r{@dots{}} */ @}
+int S::optimized_impl (int) @{ /* @r{@dots{}} */ @}
+
+S::Func* S::resolver ()
+@{
+  int (S::*pimpl) (int)
+    = getenv ("DEBUG") ? &S::debug_impl : &S::optimized_impl;
+
+  // Cast triggers -Wno-pmf-conversions.
+  return reinterpret_cast<Func*>(pimpl);
+@}
+
+int S::interface (int) __attribute__ ((ifunc ("_ZN1S8resolverEv")));
+@end smallexample
+
 Indirect functions cannot be weak.  Binutils version 2.20.1 or higher
 and GNU C Library version 2.11.1 are required to use this feature.
 
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index f862b7f..de1943f 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -5382,6 +5382,11 @@ pointers. This warning level may give a larger number of
 false positives and is deactivated by default.
 @end table
 
+@item -Wattribute-alias
+Warn about declarations using the @code{alias} and similar attributes whose
+target is incompatible with the type of the alias.  @xref{Function Attributes,
+,Declaring Attributes of Functions}.
+
 @item -Wbool-compare
 @opindex Wno-bool-compare
 @opindex Wbool-compare
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-1.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-1.C
index 2c7bba1..4a29e8b 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-1.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-1.C
@@ -4,26 +4,33 @@
 
 struct Klass
 {
+  int a[4];
+
   int implementation ();
   int magic ();
 
-  typedef int (Klass::*MemFuncPtr)();
+  /* An ifunc resolver must return a pointer to an ordinary (non-member)
+     function.  To make it possible to use ifunc with member functions,
+     the resolver must convert a member function pointer to an ordinary
+     function pointer (slicing off the high word).  */
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
-Klass::MemFuncPtr p = &Klass::implementation;
-
-int Klass::implementation (void)
+int Klass::implementation ()
 {
   __builtin_printf ("'ere I am JH\n");
-  return 1234;
+  return a[0] + a[1] + a[2] + a[3];
 }
 
-
-Klass::MemFuncPtr Klass::resolver (void)
+Klass::Func* Klass::resolver (void)
 {
-  return &Klass::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klass::implementation);
 }
 
 int f (void) __attribute__ ((ifunc ("foo")));
@@ -32,11 +39,16 @@ typedef int (F)(void);
 extern "C" F* foo () { return 0; }
 
 
-int Klass::magic (void) __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
+int Klass::magic () __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
 
 int main ()
 {
   Klass obj;
 
-  return !(obj.magic () == 1234);
+  obj.a[0] = 1;
+  obj.a[1] = 2;
+  obj.a[2] = 3;
+  obj.a[3] = 4;
+
+  return !(obj.magic () == 10);
 }
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-2.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-2.C
index 1fc940b..e5be3d2 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-2.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-2.C
@@ -9,9 +9,9 @@ struct Klass
   int implementation ();
   int magic ();
 
-  typedef int (Klass::*MemFuncPtr)();
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
 int Klass::implementation (void)
@@ -20,9 +20,13 @@ int Klass::implementation (void)
   return 0;
 }
 
-Klass::MemFuncPtr Klass::resolver (void)
+Klass::Func* Klass::resolver (void)
 {
-  return &Klass::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klass::implementation);
 }
 
 int Klass::magic (void) __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-3.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-3.C
index 04206a1..6d49424 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-3.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-3.C
@@ -6,23 +6,29 @@
 
 struct Klass
 {
+  int a[4];
+
   int implementation ();
   int magic ();
 
-  typedef int (Klass::*MemFuncPtr)();
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
 int Klass::implementation (void)
 {
   printf ("'ere I am JH\n");
-  return 0;
+  return a[0] + a[1] + a[2] + a[3];
 }
 
-Klass::MemFuncPtr Klass::resolver (void)
+Klass::Func* Klass::resolver ()
 {
-  return &Klass::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klass::implementation);
 }
 
 int Klass::magic (void) __attribute__ ((ifunc ("_ZN5Klass8resolverEv")));
@@ -36,5 +42,10 @@ int main ()
 {
   Klass obj;
 
-  return Foo (obj, &Klass::magic) != 0;
+  obj.a[0] = 1;
+  obj.a[1] = 2;
+  obj.a[2] = 3;
+  obj.a[3] = 4;
+
+  return Foo (obj, &Klass::magic) != 10;
 }
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-4.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-4.C
index 3127193..f71dc3b 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-4.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-4.C
@@ -14,9 +14,9 @@ struct Klassier : Klass
   int implementation ();
   int magic ();
 
-  typedef int (Klassier::*MemFuncPtr)();
+  typedef int Func (Klass*);
 
-  static MemFuncPtr resolver ();
+  static Func* resolver ();
 };
 
 int Klassier::implementation (void)
@@ -25,9 +25,13 @@ int Klassier::implementation (void)
   return 0;
 }
 
-Klassier::MemFuncPtr Klassier::resolver (void)
+Klassier::Func* Klassier::resolver ()
 {
-  return &Klassier::implementation;
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  */
+
+  return reinterpret_cast<Func*>(&Klassier::implementation);
 }
 
 int Klassier::magic (void) __attribute__ ((ifunc ("_ZN8Klassier8resolverEv")));
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-5.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-5.C
index 05855dd..fd8bcff 100644
--- a/gcc/testsuite/g++.dg/ext/attr-ifunc-5.C
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-5.C
@@ -1,15 +1,21 @@
 // PR c/81854 - weak alias of an incompatible symbol accepted
 // { dg-do compile }
 // { dg-require-ifunc "" } */
+// { dg-options "-Wextra -Wno-pmf-conversions" }
 
 struct Klass
 {
   int implementation ();
-  const char* magic ();
+  int good_magic ();
+  int iffy_magic ();
+  const char* bad_magic ();
 
+  typedef int (Func)(Klass*);
   typedef int (Klass::*MemFuncPtr)();
 
-  static MemFuncPtr resolver ();
+  static Func* good_resolver ();
+  static void* iffy_resolver ();
+  static MemFuncPtr bad_resolver ();
 };
 
 int Klass::implementation (void)
@@ -17,13 +23,42 @@ int Klass::implementation (void)
   return 0;
 }
 
-const char* __attribute__ ((ifunc ("_ZN5Klass8resolverEv")))
-  Klass::magic ();   // { dg-warning "alias between functions of incompatible types" }
+// Verify no warning for the expected/compatible declaration.
 
+int __attribute__ ((ifunc ("_ZN5Klass13good_resolverEv")))
+Klass::good_magic ();
+
+Klass::Func*
+Klass::good_resolver (void)
+{
+  MemFuncPtr mfp = &Klass::implementation;
+
+  return reinterpret_cast<Func*>(mfp);
+}
+
+
+// Verify a warning for the unsafe declaration.
+
+int __attribute__ ((ifunc ("_ZN5Klass13iffy_resolverEv")))
+Klass::iffy_magic ();    // { dg-message "resolver indirect function declared here" }
+
+void*
+Klass::iffy_resolver (void)   // { dg-warning ".ifunc. resolver for .int Klass::iffy_magic\\(\\). should return .int \\(\\*\\)\\(Klass\\*\\)." }
+{
+  MemFuncPtr mfp = &Klass::implementation;
+
+  return reinterpret_cast<void*>(mfp);
+}
+
+
+// Verify an error for an incompatible declaration.
+
+const char* __attribute__ ((ifunc ("_ZN5Klass12bad_resolverEv")))
+Klass::bad_magic ();   // { dg-message "resolver indirect function declared here" }
 
 
 Klass::MemFuncPtr
-Klass::resolver (void) // { dg-message "aliased declaration here" }
+Klass::bad_resolver (void)   // { dg-error ".ifunc. resolver for .const char\\* Klass::bad_magic\\(\\). must return .const char\\* \\(\\*\\)\\(Klass\\*\\)." }
 {
   return &Klass::implementation;
 }
diff --git a/gcc/testsuite/g++.dg/ext/attr-ifunc-6.C b/gcc/testsuite/g++.dg/ext/attr-ifunc-6.C
new file mode 100644
index 0000000..918123d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/attr-ifunc-6.C
@@ -0,0 +1,81 @@
+// PR c/81854 - weak alias of an incompatible symbol accepted
+// { dg-do run }
+// { dg-require-ifunc "" }
+// { dg-options "-Wno-pmf-conversions" }
+
+struct Klass
+{
+  int a[4];
+
+  int implementation (int);
+  int implementation (int, long, const char*);
+
+  int magic (int);
+  int magic (int, long, const char *);
+
+  /* An ifunc resolver must return a pointer to an ordinary (non-member)
+     function.  To make it possible to use ifunc with member functions,
+     the resolver must convert a member function pointer to an ordinary
+     function pointer (slicing off the high word).  */
+  typedef int Func1 (Klass*, int);
+  typedef int Func3 (Klass*, int, long, const char*);
+
+  template <class Func>
+  static Func* resolver ();
+};
+
+const char *str;
+
+int Klass::implementation (int x)
+{
+  return a[0] + a[1] + a[2] + a[3] + x;
+}
+
+int Klass::implementation (int x, long y, const char *s)
+{
+  return a[0] + a[1] + a[2] + a[3] + x + y + !(s == str);
+}
+
+template <>
+Klass::Func1* Klass::resolver<Klass::Func1> ()
+{
+  int (Klass::*pmf)(int) = &Klass::implementation;
+
+  /* GCC guarantees this conversion to be safe and the resulting pointer
+     usable to call the member function using ordinary (i.e., non-member)
+     function call syntax.  The cast triggers -Wno-pmf-conversions which
+     the test suppresses.  */
+
+  return reinterpret_cast<Func1*>(pmf);
+}
+
+template <>
+Klass::Func3* Klass::resolver<Klass::Func3> ()
+{
+  int (Klass::*pmf)(int, long, const char*) = &Klass::implementation;
+
+  return reinterpret_cast<Func3*>(pmf);
+}
+
+__attribute__ ((ifunc ("_ZN5Klass8resolverIFiPS_iEEEPT_v")))
+int Klass::magic (int);
+
+__attribute__ ((ifunc ("_ZN5Klass8resolverIFiPS_ilPKcEEEPT_v")))
+int Klass::magic (int, long, const char *);
+
+int main ()
+{
+  Klass obj;
+
+  obj.a[0] = 1;
+  obj.a[1] = 2;
+  obj.a[2] = 3;
+  obj.a[3] = 4;
+
+  str = "teststring";
+
+  // Verify that data members and function arguments are correctly
+  // retrieved.
+  if (obj.magic (5) != 15 || obj.magic (5, 6, str) != 21)
+    __builtin_abort ();
+}
diff --git a/gcc/testsuite/g++.old-deja/g++.abi/vtable2.C b/gcc/testsuite/g++.old-deja/g++.abi/vtable2.C
index 2c88a95..96533e0 100644
--- a/gcc/testsuite/g++.old-deja/g++.abi/vtable2.C
+++ b/gcc/testsuite/g++.old-deja/g++.abi/vtable2.C
@@ -1,5 +1,5 @@
 // { dg-do run  }
-// { dg-options "-Wno-attributes -fno-strict-aliasing" }
+// { dg-options "-Wno-attribute-alias -fno-strict-aliasing" }
 // Origin: Mark Mitchell <mark@codesourcery.com>
 
 #if defined (__GXX_ABI_VERSION) && __GXX_ABI_VERSION >= 100
diff --git a/gcc/testsuite/gcc.dg/attr-ifunc-6.c b/gcc/testsuite/gcc.dg/attr-ifunc-6.c
new file mode 100644
index 0000000..2af82ab
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-ifunc-6.c
@@ -0,0 +1,31 @@
+/* { dg-do compile }  */
+/* { dg-require-ifunc "" } */
+/* { dg-options "" } */
+
+static int implementation (void)
+{
+  return 0;
+}
+
+/* Verify that a resolver that returns void* is not diagnosed without
+   -Wextra (see attr-ifunc-7.c for a test that verifies that -Wextra
+   triggers the warning).  */
+static void* resolver_nowarn (void)
+{
+  return implementation;
+}
+
+extern int
+indirect_nowarn (void) __attribute__ ((ifunc ("resolver_nowarn")));
+
+
+/* Verify that a resolver that returns a T* that's not void* is diagnosed
+   even without -Wextra.  */
+static int* resolver_warn (void)   /* { dg-warning ".ifunc. resolver for .indirect_warn. should return 'int \\(\\*\\)\\(void\\)." } */
+{
+  return (int *)implementation;
+}
+
+extern int
+indirect_warn (void)   /* { dg-message "" } */
+__attribute__ ((ifunc ("resolver_warn")));
diff --git a/gcc/testsuite/gcc.dg/attr-ifunc-7.c b/gcc/testsuite/gcc.dg/attr-ifunc-7.c
new file mode 100644
index 0000000..a8e18ed
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-ifunc-7.c
@@ -0,0 +1,30 @@
+/* { dg-do compile }  */
+/* { dg-require-ifunc "" } */
+/* { dg-options "-Wextra" } */
+
+static int implementation (void)
+{
+  return 0;
+}
+
+/* Verify that a resolver that returns void* is diagnosed with -Wextra.  */
+static void* resolver_warn (void)   /* { dg-warning ".ifunc. resolver for .indirect_warn. should return .int \\(\\*\\)\\(void\\)." } */
+{
+  return implementation;
+}
+
+extern __attribute__ ((ifunc ("resolver_warn"))) int
+indirect_warn (void);   /* { dg-message "resolver indirect function declared here" } */
+
+
+
+/* Verify that a resolver that returns int* is still diagnosed, even
+   with -Wextra.  */
+static int* resolver_warn_2 (void)   /* { dg-warning ".ifunc. resolver for .indirect_warn_2. should return .int \\(\\*\\)\\(void\\)." } */
+{
+  return (int *)implementation;
+}
+
+extern __attribute__ ((ifunc ("resolver_warn_2"))) int
+indirect_warn_2 (void);   /* { dg-message "resolver indirect function declared here" } */
+
diff --git a/gcc/testsuite/gcc.dg/pr81854.c b/gcc/testsuite/gcc.dg/pr81854.c
index b8499f8..1021a81 100644
--- a/gcc/testsuite/gcc.dg/pr81854.c
+++ b/gcc/testsuite/gcc.dg/pr81854.c
@@ -1,6 +1,7 @@
 /* PR c/81854 - weak alias of an incompatible symbol accepted
    { dg-do compile }
-   { dg-require-ifunc "" } */
+   { dg-require-ifunc "" }
+   { dg-options "-Wextra" } */
 
 const char* __attribute__ ((weak, alias ("f0_target")))
 f0 (void);          /* { dg-error "alias between function and variable" } */
@@ -26,39 +27,37 @@ const char* f2_target (int i)   /* { dg-message "aliased declaration here" } */
   return 0;
 }
 
-
 int __attribute__ ((ifunc ("f3_resolver")))
-f3 (void);          /* { dg-error ".ifunc. resolver must return a function pointer" } */
+f3 (void);          /* { dg-message "resolver indirect function declared here" } */
 
-int f3_resolver (void)   /* { dg-message "resolver declaration here" } */
+void* f3_resolver (void) /* { dg-warning "ifunc. resolver for .f3. should return .int \\(\\*\\)\\(void\\)." } */
 {
   return 0;
 }
 
 
 int __attribute__ ((ifunc ("f4_resolver")))
-f4 (void);          /* { dg-warning ".ifunc. resolver should return a function pointer" } */
+f4 (void);          /* { dg-message "resolver indirect function declared here" } */
 
-void* f4_resolver (void) /* { dg-message "resolver declaration here" } */
+typedef void F4 (void);
+F4* f4_resolver (void) /* { dg-warning ".ifunc. resolver for .f4. should return .int \\(\\*\\)\\(void\\)" } */
 {
   return 0;
 }
 
+const char* __attribute__ ((ifunc ("f5_resolver")))
+f5 (void);
 
-int __attribute__ ((ifunc ("f5_resolver")))
-f5 (void);          /* { dg-warning "alias between functions of incompatible types" } */
-
-typedef void F5 (void);
-F5* f5_resolver (void) /* { dg-message "aliased declaration here" } */
+typedef const char* F5 (void);
+F5* f5_resolver (void)
 {
   return 0;
 }
 
-const char* __attribute__ ((ifunc ("f6_resolver")))
-f6 (void);
+int __attribute__ ((ifunc ("f6_resolver")))
+f6 (void);          /* { dg-message "resolver indirect function declared here" } */
 
-typedef const char* F6 (void);
-F6* f6_resolver (void)
+int f6_resolver (void)   /* { dg-error ".ifunc. resolver for 'f6' must return .int \\(\\*\\)\\(void\\)." } */
 {
   return 0;
 }
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 57f646c..cf312a1 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -440,8 +440,8 @@ proc check_ifunc_available { } {
 	extern "C" {
 	#endif
 	typedef void F (void);
-	F* g() {}
-	void f() __attribute__((ifunc("g")));
+	F* g (void) {}
+	void f () __attribute__ ((ifunc ("g")));
 	#ifdef __cplusplus
 	}
 	#endif

Re: [PING] Re: correct attribute ifunc C++ type safety (PR 82301)

[Nathan Sidwell]

On 10/11/2017 12:21 PM, Martin Sebor wrote:
> Ping?
> 
> Since I submitted the updated patch it has been suggested to me
> that providing a new option to control the warning rather than
> using an existing one would be preferable (see bug 82435 for
> the background).  The attached update to the patch adds
> -Wattribute-alias for this purpose and restores the original
> disposition for -Wincompatible-pointer-types.

Makes sense to me. Did you mis my review earlier today?

nathan

-- 
Nathan Sidwell

Re: [PING] Re: correct attribute ifunc C++ type safety (PR 82301)

[Martin Sebor]

On 10/11/2017 10:32 AM, Nathan Sidwell wrote:
> On 10/11/2017 12:21 PM, Martin Sebor wrote:
>> Ping?
>>
>> Since I submitted the updated patch it has been suggested to me
>> that providing a new option to control the warning rather than
>> using an existing one would be preferable (see bug 82435 for
>> the background).  The attached update to the patch adds
>> -Wattribute-alias for this purpose and restores the original
>> disposition for -Wincompatible-pointer-types.
>
> Makes sense to me. Did you mis my review earlier today?

Thanks.  For some reason I got both your replies at the same
time.  Since you're comfortable with the C++ aspects let me
see if Joseph is willing to approve the updated patch.

Martin

Re: correct attribute ifunc C++ type safety (PR 82301)

[Jeff Law]

On 10/11/2017 05:14 AM, Nathan Sidwell wrote:
> On 10/04/2017 03:40 PM, Martin Sebor wrote:
>> On 09/28/2017 08:25 AM, Nathan Sidwell wrote:
> 
> 
>> Since the original tests (where the resolver returns void*) pass
>> across the board I assume it must work for all supported ABIs.
>> Or is there some subtlety between the before and after code that
>> I'm missing?
> 
> I had a vague notion of some (IBM?) target that might do something
> different.  Perhaps it's gone.  Your comment explains it fine.
> 
>>> oh, I think it's trying to spot the pointer to NON-static member
>>> function internal record type.  But brokenly. I think pmf record_types
>>> have DECL_ARTIFICIAL and BUILTIN_LOCATION, that might be useful.
>>
>> It turns out this code was superfluous with the C++ correction
>> and I was able to remove it with no impact on the tests.
> 
> Great.
> 
>>> Is this function called before we know whether we've enabled the
>>> appropriate warnings?  It would be better to bail out sooner if the
>>> warnings are disabled.
>>
>> I'm not sure I understand the question or suggestion here but
>> warnings in general are certainly enabled at this point.  The
>> function issues both errors and warnings so it can't very well
>> exit early without checking the compatibility.  Let me know if
>> I misunderstood your comment.
> 
> Oh, forgot it issued errors too, so my queston is moot.
> 
>> -signedness.
>> +signedness.  In C++ where incompatible pointer conversions ordinarily
>> cause
> Missing comma:  In C++, where incompatible ...
> 
>> +errors, the warning detects such conversions in GCC extensions that
>> aren't
>> +part of the standard language.  An example of a construct that might
>> perform
>> +such a conversion is the @code{alias} attribute.  @xref{Function
>> Attributes,,Declaring Attributes of Functions}.
> 
> Looks fine to me.  (pedantically, I don't think I can formally approve
> this, because it's not part of the C++ FE.)
That's good enough for me :-)  So I'll make it an official ACK.

jeff