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

Message ID 9c7862bf-eda8-977a-28b9-65d17c1947cb@gmail.com
State New
Headers show
Series
  • [PING] Re: correct attribute ifunc C++ type safety (PR 82301)
Related show

Commit Message

Martin Sebor Oct. 11, 2017, 4:21 p.m.
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

Comments

Nathan Sidwell Oct. 11, 2017, 4:32 p.m. | #1
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
Martin Sebor Oct. 11, 2017, 4:44 p.m. | #2
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

Patch

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