[patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

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

Hi,  this patch fixes the bug described at
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15774 in bugzilla.
It takes care that for function pointer types also the
calling-convention modifying attributes can be
displayed.  I added an implementation for the i386 architecture.

Tested for i686-pc-cygwin, i686-pc-mingw32, and x86_64-w64-mingw32.

ChangeLog

2010-12-22  Kai Tietz

	PR c++/15774
	* c-family/c-pretty-print.c: Add target.h include.
	(pp_c_specifier_qualifier_list): Call
	pp_c_attributes_calling_convention for (*).
	(pp_c_attributes_calling_convention): New.
	* c-family/c-pretty-print.h (pp_c_attributes_calling_convention):
	New prototype.
	* cp/error.c (dump_type_prefix): Call
	pp_c_attributes_calling_convention for (*).
	* config/i386/i386.c (ix86_attribute_affects_calling_convention):
	New hook.
	(TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Define.
	* doc/tm.texi.in (TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Add.
	* doc/tm.texi: Regenerated.

Ok for apply?

Regards,
Kai

[-- Attachment #2: dispcallcvt.txt --]
[-- Type: text/plain, Size: 6407 bytes --]

Index: gcc/gcc/c-family/c-pretty-print.c
===================================================================
--- gcc.orig/gcc/c-family/c-pretty-print.c	2010-12-22 13:43:53.651439000 +0100
+++ gcc/gcc/c-family/c-pretty-print.c	2010-12-22 14:04:32.807689000 +0100
@@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.  
 #include "tree-pretty-print.h"
 #include "tree-iterator.h"
 #include "diagnostic.h"
+#include "target.h"
 
 /* Translate if being used for diagnostics, but not for dump files or
    __PRETTY_FUNCTION.  */
@@ -460,6 +461,7 @@ pp_c_specifier_qualifier_list (c_pretty_
 	  {
 	    pp_c_whitespace (pp);
 	    pp_c_left_paren (pp);
+	    pp_c_attributes_calling_convention (pp, TYPE_ATTRIBUTES (pointee));
 	  }
 	else if (!c_dialect_cxx ())
 	  pp_c_whitespace (pp);
@@ -790,6 +792,45 @@ pp_c_attributes (c_pretty_printer *pp, t
   pp_c_right_paren (pp);
 }
 
+/* Pretty-print ATTRIBUTES using GNU C extension syntax for calling
+   convention affecting attributes.  */
+
+void
+pp_c_attributes_calling_convention (c_pretty_printer *pp, tree a)
+{
+  int is_first = 1;
+
+  if (a == NULL_TREE)
+    return;
+
+  for (; a != NULL_TREE; a = TREE_CHAIN (a))
+    {
+      if (!targetm.attribute_affects_calling_convention (TREE_PURPOSE (a)))
+        continue;
+      if (is_first)
+	{
+	  pp_c_ws_string (pp, "__attribute__");
+	  pp_c_left_paren (pp);
+	  pp_c_left_paren (pp);
+	  is_first = 0;
+	}
+      else
+	{
+	  pp_separate_with (pp, ',');
+	}
+      pp_tree_identifier (pp, TREE_PURPOSE (a));
+      if (TREE_VALUE (a))
+	pp_c_call_argument_list (pp, TREE_VALUE (a));
+    }
+
+  if (!is_first)
+    {
+      pp_c_right_paren (pp);
+      pp_c_right_paren (pp);
+      pp_c_whitespace (pp);
+    }
+}
+
 /* function-definition:
       declaration-specifiers declarator compound-statement  */
 
Index: gcc/gcc/cp/error.c
===================================================================
--- gcc.orig/gcc/cp/error.c	2010-12-22 13:43:53.681439000 +0100
+++ gcc/gcc/cp/error.c	2010-12-22 14:05:38.526439000 +0100
@@ -661,6 +661,8 @@ dump_type_prefix (tree t, int flags)
 	  {
 	    pp_cxx_whitespace (cxx_pp);
 	    pp_cxx_left_paren (cxx_pp);
+	    pp_c_attributes_calling_convention (pp_c_base (cxx_pp),
+						TYPE_ATTRIBUTES (sub));
 	  }
 	if (TREE_CODE (t) == POINTER_TYPE)
 	  pp_character(cxx_pp, '*');
Index: gcc/gcc/c-family/c-pretty-print.h
===================================================================
--- gcc.orig/gcc/c-family/c-pretty-print.h	2010-12-22 13:43:53.676439000 +0100
+++ gcc/gcc/c-family/c-pretty-print.h	2010-12-22 13:45:49.479564000 +0100
@@ -176,6 +176,7 @@ void pp_c_space_for_pointer_operator (c_
 void pp_c_tree_decl_identifier (c_pretty_printer *, tree);
 void pp_c_function_definition (c_pretty_printer *, tree);
 void pp_c_attributes (c_pretty_printer *, tree);
+void pp_c_attributes_calling_convention (c_pretty_printer *, tree);
 void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type);
 void pp_c_type_qualifier_list (c_pretty_printer *, tree);
 void pp_c_parameter_type_list (c_pretty_printer *, tree);
Index: gcc/gcc/config/i386/i386.c
===================================================================
--- gcc.orig/gcc/config/i386/i386.c	2010-12-22 13:43:53.679439000 +0100
+++ gcc/gcc/config/i386/i386.c	2010-12-22 14:19:21.091890600 +0100
@@ -5116,6 +5116,46 @@ ix86_function_ok_for_sibcall (tree decl,
   return true;
 }
 
+static bool
+ix86_attribute_affects_calling_convention (tree name)
+{
+  int ident_len;
+  const char *p;
+
+  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
+  p = IDENTIFIER_POINTER (name);
+  ident_len = IDENTIFIER_LENGTH (name);
+
+  if (ident_len > 4 && p[0] == '_' && p[1] == '_' && p[ident_len - 1] == '_'
+      && p[ident_len - 2] == '_')
+    {
+      ident_len -= 4;
+      p += 2;
+    }
+  switch (ident_len)
+    {
+    case 5:
+      if (!strncmp (p, "cdecl", 5))
+        return true;
+      break;
+    case 6:
+      if (!strncmp (p, "ms_abi", 6))
+        return true;
+      break;
+    case 7:
+      if (!strncmp (p, "regparm", 7) || !strncmp (p, "stdcall", 7))
+        return true;
+      break;
+    case 8:
+      if (!strncmp (p, "thiscall", 8) || !strncmp (p, "fastcall", 8)
+          || !strncmp (p, "sysv_abi", 8))
+        return true;
+      break;
+    }
+
+  return false;
+}
+
 /* Handle "cdecl", "stdcall", "fastcall", "regparm", "thiscall",
    and "sseregparm" calling convention attributes;
    arguments as in struct attribute_spec.handler.  */
@@ -34813,6 +34853,10 @@ ix86_autovectorize_vector_sizes (void)
 #undef TARGET_CONDITIONAL_REGISTER_USAGE
 #define TARGET_CONDITIONAL_REGISTER_USAGE ix86_conditional_register_usage
 
+#undef TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION
+#define TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION \
+  ix86_attribute_affects_calling_convention
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 #include "gt-i386.h"
Index: gcc/gcc/doc/tm.texi
===================================================================
--- gcc.orig/gcc/doc/tm.texi	2010-12-22 13:43:53.683439000 +0100
+++ gcc/gcc/doc/tm.texi	2010-12-22 14:18:28.230485800 +0100
@@ -9752,6 +9752,10 @@ when this is needed are when one attribu
 attribute is nullified by a subsequent definition.  This function may
 call @code{merge_attributes} to handle machine-independent merging.
 
+@deftypefn {Target Hook} bool TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION (const_tree @var{name})
+Returns @code{true} if given attribute by its @var{name} affects calling convention, otherwise @code{false}.
+@end deftypefn
+
 @findex TARGET_DLLIMPORT_DECL_ATTRIBUTES
 If the only target-specific handling you require is @samp{dllimport}
 for Microsoft Windows targets, you should define the macro
Index: gcc/gcc/doc/tm.texi.in
===================================================================
--- gcc.orig/gcc/doc/tm.texi.in	2010-12-22 13:43:53.000000000 +0100
+++ gcc/gcc/doc/tm.texi.in	2010-12-22 14:16:28.788399400 +0100
@@ -9716,6 +9716,8 @@ when this is needed are when one attribu
 attribute is nullified by a subsequent definition.  This function may
 call @code{merge_attributes} to handle machine-independent merging.
 
+@hook TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION
+
 @findex TARGET_DLLIMPORT_DECL_ATTRIBUTES
 If the only target-specific handling you require is @samp{dllimport}
 for Microsoft Windows targets, you should define the macro

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Dave Korn]

On 22/12/2010 13:25, Kai Tietz wrote:

> 	PR c++/15774
> 	* c-family/c-pretty-print.c: Add target.h include.
> 	(pp_c_specifier_qualifier_list): Call
> 	pp_c_attributes_calling_convention for (*).
> 	(pp_c_attributes_calling_convention): New.
> 	* c-family/c-pretty-print.h (pp_c_attributes_calling_convention):
> 	New prototype.
> 	* cp/error.c (dump_type_prefix): Call
> 	pp_c_attributes_calling_convention for (*).
> 	* config/i386/i386.c (ix86_attribute_affects_calling_convention):
> 	New hook.
> 	(TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Define.
> 	* doc/tm.texi.in (TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Add.
> 	* doc/tm.texi: Regenerated.

  You've omitted the diffs for target.def et. al.

    cheers,
      DaveK

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

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

2010/12/22 Dave Korn <dave.korn.cygwin@gmail.com>:
> On 22/12/2010 13:25, Kai Tietz wrote:
>
>>       PR c++/15774
>>       * c-family/c-pretty-print.c: Add target.h include.
>>       (pp_c_specifier_qualifier_list): Call
>>       pp_c_attributes_calling_convention for (*).
>>       (pp_c_attributes_calling_convention): New.
>>       * c-family/c-pretty-print.h (pp_c_attributes_calling_convention):
>>       New prototype.
>>       * cp/error.c (dump_type_prefix): Call
>>       pp_c_attributes_calling_convention for (*).
>>       * config/i386/i386.c (ix86_attribute_affects_calling_convention):
>>       New hook.
>>       (TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Define.
>>       * doc/tm.texi.in (TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Add.
>>       * doc/tm.texi: Regenerated.
>
>  You've omitted the diffs for target.def et. al.
>
>    cheers,
>      DaveK
>
>

Yes, thanks. I missed to attached it to patch, well busy times.
Additional I corrected the place of documentation in tm.texi

ChangeLog

2010-12-22  Kai Tietz

	PR c++/15774
	* c-family/c-pretty-print.c: Add target.h include.
	(pp_c_specifier_qualifier_list): Call
	pp_c_attributes_calling_convention for (*).
	(pp_c_attributes_calling_convention): New.
	* c-family/c-pretty-print.h (pp_c_attributes_calling_convention):
	New prototype.
	* cp/error.c (dump_type_prefix): Call
	pp_c_attributes_calling_convention for (*).
	* config/i386/i386.c (ix86_attribute_affects_calling_convention):
	New hook.
	(TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Define.
	* target.def (attribute_affects_calling_convention): New hook
	definition.
	* doc/tm.texi.in (TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION): Add.
	* doc/tm.texi: Regenerated.

Regards,
Kai

[-- Attachment #2: dispcallcvt.txt --]
[-- Type: text/plain, Size: 6953 bytes --]

Index: gcc/gcc/c-family/c-pretty-print.c
===================================================================
--- gcc.orig/gcc/c-family/c-pretty-print.c	2010-12-22 13:43:53.651439000 +0100
+++ gcc/gcc/c-family/c-pretty-print.c	2010-12-22 14:04:32.807689000 +0100
@@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.  
 #include "tree-pretty-print.h"
 #include "tree-iterator.h"
 #include "diagnostic.h"
+#include "target.h"
 
 /* Translate if being used for diagnostics, but not for dump files or
    __PRETTY_FUNCTION.  */
@@ -460,6 +461,7 @@ pp_c_specifier_qualifier_list (c_pretty_
 	  {
 	    pp_c_whitespace (pp);
 	    pp_c_left_paren (pp);
+	    pp_c_attributes_calling_convention (pp, TYPE_ATTRIBUTES (pointee));
 	  }
 	else if (!c_dialect_cxx ())
 	  pp_c_whitespace (pp);
@@ -790,6 +792,45 @@ pp_c_attributes (c_pretty_printer *pp, t
   pp_c_right_paren (pp);
 }
 
+/* Pretty-print ATTRIBUTES using GNU C extension syntax for calling
+   convention affecting attributes.  */
+
+void
+pp_c_attributes_calling_convention (c_pretty_printer *pp, tree a)
+{
+  int is_first = 1;
+
+  if (a == NULL_TREE)
+    return;
+
+  for (; a != NULL_TREE; a = TREE_CHAIN (a))
+    {
+      if (!targetm.attribute_affects_calling_convention (TREE_PURPOSE (a)))
+        continue;
+      if (is_first)
+	{
+	  pp_c_ws_string (pp, "__attribute__");
+	  pp_c_left_paren (pp);
+	  pp_c_left_paren (pp);
+	  is_first = 0;
+	}
+      else
+	{
+	  pp_separate_with (pp, ',');
+	}
+      pp_tree_identifier (pp, TREE_PURPOSE (a));
+      if (TREE_VALUE (a))
+	pp_c_call_argument_list (pp, TREE_VALUE (a));
+    }
+
+  if (!is_first)
+    {
+      pp_c_right_paren (pp);
+      pp_c_right_paren (pp);
+      pp_c_whitespace (pp);
+    }
+}
+
 /* function-definition:
       declaration-specifiers declarator compound-statement  */
 
Index: gcc/gcc/cp/error.c
===================================================================
--- gcc.orig/gcc/cp/error.c	2010-12-22 13:43:53.681439000 +0100
+++ gcc/gcc/cp/error.c	2010-12-22 14:05:38.526439000 +0100
@@ -661,6 +661,8 @@ dump_type_prefix (tree t, int flags)
 	  {
 	    pp_cxx_whitespace (cxx_pp);
 	    pp_cxx_left_paren (cxx_pp);
+	    pp_c_attributes_calling_convention (pp_c_base (cxx_pp),
+						TYPE_ATTRIBUTES (sub));
 	  }
 	if (TREE_CODE (t) == POINTER_TYPE)
 	  pp_character(cxx_pp, '*');
Index: gcc/gcc/c-family/c-pretty-print.h
===================================================================
--- gcc.orig/gcc/c-family/c-pretty-print.h	2010-12-22 13:43:53.676439000 +0100
+++ gcc/gcc/c-family/c-pretty-print.h	2010-12-22 13:45:49.479564000 +0100
@@ -176,6 +176,7 @@ void pp_c_space_for_pointer_operator (c_
 void pp_c_tree_decl_identifier (c_pretty_printer *, tree);
 void pp_c_function_definition (c_pretty_printer *, tree);
 void pp_c_attributes (c_pretty_printer *, tree);
+void pp_c_attributes_calling_convention (c_pretty_printer *, tree);
 void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type);
 void pp_c_type_qualifier_list (c_pretty_printer *, tree);
 void pp_c_parameter_type_list (c_pretty_printer *, tree);
Index: gcc/gcc/config/i386/i386.c
===================================================================
--- gcc.orig/gcc/config/i386/i386.c	2010-12-22 13:43:53.679439000 +0100
+++ gcc/gcc/config/i386/i386.c	2010-12-22 14:19:21.091890600 +0100
@@ -5116,6 +5116,46 @@ ix86_function_ok_for_sibcall (tree decl,
   return true;
 }
 
+static bool
+ix86_attribute_affects_calling_convention (tree name)
+{
+  int ident_len;
+  const char *p;
+
+  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
+  p = IDENTIFIER_POINTER (name);
+  ident_len = IDENTIFIER_LENGTH (name);
+
+  if (ident_len > 4 && p[0] == '_' && p[1] == '_' && p[ident_len - 1] == '_'
+      && p[ident_len - 2] == '_')
+    {
+      ident_len -= 4;
+      p += 2;
+    }
+  switch (ident_len)
+    {
+    case 5:
+      if (!strncmp (p, "cdecl", 5))
+        return true;
+      break;
+    case 6:
+      if (!strncmp (p, "ms_abi", 6))
+        return true;
+      break;
+    case 7:
+      if (!strncmp (p, "regparm", 7) || !strncmp (p, "stdcall", 7))
+        return true;
+      break;
+    case 8:
+      if (!strncmp (p, "thiscall", 8) || !strncmp (p, "fastcall", 8)
+          || !strncmp (p, "sysv_abi", 8))
+        return true;
+      break;
+    }
+
+  return false;
+}
+
 /* Handle "cdecl", "stdcall", "fastcall", "regparm", "thiscall",
    and "sseregparm" calling convention attributes;
    arguments as in struct attribute_spec.handler.  */
@@ -34813,6 +34853,10 @@ ix86_autovectorize_vector_sizes (void)
 #undef TARGET_CONDITIONAL_REGISTER_USAGE
 #define TARGET_CONDITIONAL_REGISTER_USAGE ix86_conditional_register_usage
 
+#undef TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION
+#define TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION \
+  ix86_attribute_affects_calling_convention
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 #include "gt-i386.h"
Index: gcc/gcc/doc/tm.texi
===================================================================
--- gcc.orig/gcc/doc/tm.texi	2010-12-22 13:43:53.683439000 +0100
+++ gcc/gcc/doc/tm.texi	2010-12-22 14:35:44.981021900 +0100
@@ -9765,6 +9765,10 @@ to perform initial processing of the @sa
 @file{i386/i386.c}, for example.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION (const_tree @var{name})
+Returns @code{true} if given attribute by its @var{name} affects calling convention, otherwise @code{false}.
+@end deftypefn
+
 @deftypefn {Target Hook} bool TARGET_VALID_DLLIMPORT_ATTRIBUTE_P (const_tree @var{decl})
 @var{decl} is a variable or function with @code{__attribute__((dllimport))} specified.  Use this hook if the target needs to add extra validation checks to @code{handle_dll_attribute}.
 @end deftypefn
Index: gcc/gcc/doc/tm.texi.in
===================================================================
--- gcc.orig/gcc/doc/tm.texi.in	2010-12-22 13:43:53.000000000 +0100
+++ gcc/gcc/doc/tm.texi.in	2010-12-22 14:34:53.699443700 +0100
@@ -9729,6 +9729,8 @@ to perform initial processing of the @sa
 @file{i386/i386.c}, for example.
 @end deftypefn
 
+@hook TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION
+
 @hook TARGET_VALID_DLLIMPORT_ATTRIBUTE_P
 
 @defmac TARGET_DECLSPEC
Index: gcc/gcc/target.def
===================================================================
--- gcc.orig/gcc/target.def	2010-12-22 14:37:59.000000000 +0100
+++ gcc/gcc/target.def	2010-12-22 14:38:26.607056300 +0100
@@ -1113,6 +1113,14 @@ DEFHOOK
  tree, (tree olddecl, tree newdecl),
  merge_decl_attributes)
 
+/* Return true iff attribute NAME affects calling convention. */
+DEFHOOK
+(attribute_affects_calling_convention,
+ "Returns @code{true} if given attribute by its @var{name} affects calling\
+ convention, otherwise @code{false}.",
+ bool, (const_tree name),
+ hook_bool_const_tree_false)
+
 /* Given two types, merge their attributes and return the result.  */
 DEFHOOK
 (merge_type_attributes,

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Gabriel Dos Reis]

Any reason why is_first is of type `int' as opposed to `bool'?
rename pp_c_attributes_calling_convention to
pp_c_calling_convention_attributes.
You should probably modify cp/cxx-pretty-print.c too

I really prefer we don't add pretty printing code to cp/error.c.
Rather, we should be deferring to the pretty printers in
cp/cxx-pretty-print.c.  But, I suspect that is a battle for
another day.

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

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

2010/12/22 Gabriel Dos Reis <gdr@integrable-solutions.net>:
> Any reason why is_first is of type `int' as opposed to `bool'?
> rename pp_c_attributes_calling_convention to
> pp_c_calling_convention_attributes.
> You should probably modify cp/cxx-pretty-print.c too

Well, I was first think here to implement logic different, but you are
right, bool is better here. I adjusted patch. Also renamed function.

> I really prefer we don't add pretty printing code to cp/error.c.
> Rather, we should be deferring to the pretty printers in
> cp/cxx-pretty-print.c.  But, I suspect that is a battle for
> another day.

Well, I tried to do thing here first in cxx-pretty-print.c, but it
doesn't had any effect for function pointers, so I did it just in
error.c file.  This seems to be a different battle for a different
day.

Regards,
Kai

[-- Attachment #2: dispcallcvt.txt --]
[-- Type: text/plain, Size: 6961 bytes --]

Index: gcc/gcc/c-family/c-pretty-print.c
===================================================================
--- gcc.orig/gcc/c-family/c-pretty-print.c	2010-12-22 13:43:53.651439000 +0100
+++ gcc/gcc/c-family/c-pretty-print.c	2010-12-22 14:04:32.807689000 +0100
@@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.  
 #include "tree-pretty-print.h"
 #include "tree-iterator.h"
 #include "diagnostic.h"
+#include "target.h"
 
 /* Translate if being used for diagnostics, but not for dump files or
    __PRETTY_FUNCTION.  */
@@ -460,6 +461,7 @@ pp_c_specifier_qualifier_list (c_pretty_
 	  {
 	    pp_c_whitespace (pp);
 	    pp_c_left_paren (pp);
+	    pp_c_calling_convention_attributes (pp, TYPE_ATTRIBUTES (pointee));
 	  }
 	else if (!c_dialect_cxx ())
 	  pp_c_whitespace (pp);
@@ -790,6 +792,45 @@ pp_c_attributes (c_pretty_printer *pp, t
   pp_c_right_paren (pp);
 }
 
+/* Pretty-print ATTRIBUTES using GNU C extension syntax for calling
+   convention affecting attributes.  */
+
+void
+pp_c_calling_convention_attributes (c_pretty_printer *pp, tree a)
+{
+  bool is_first = true;
+
+  if (a == NULL_TREE)
+    return;
+
+  for (; a != NULL_TREE; a = TREE_CHAIN (a))
+    {
+      if (!targetm.attribute_affects_calling_convention (TREE_PURPOSE (a)))
+        continue;
+      if (is_first)
+	{
+	  pp_c_ws_string (pp, "__attribute__");
+	  pp_c_left_paren (pp);
+	  pp_c_left_paren (pp);
+	  is_first = false;
+	}
+      else
+	{
+	  pp_separate_with (pp, ',');
+	}
+      pp_tree_identifier (pp, TREE_PURPOSE (a));
+      if (TREE_VALUE (a))
+	pp_c_call_argument_list (pp, TREE_VALUE (a));
+    }
+
+  if (!is_first)
+    {
+      pp_c_right_paren (pp);
+      pp_c_right_paren (pp);
+      pp_c_whitespace (pp);
+    }
+}
+
 /* function-definition:
       declaration-specifiers declarator compound-statement  */
 
Index: gcc/gcc/cp/error.c
===================================================================
--- gcc.orig/gcc/cp/error.c	2010-12-22 13:43:53.681439000 +0100
+++ gcc/gcc/cp/error.c	2010-12-22 14:05:38.526439000 +0100
@@ -661,6 +661,8 @@ dump_type_prefix (tree t, int flags)
 	  {
 	    pp_cxx_whitespace (cxx_pp);
 	    pp_cxx_left_paren (cxx_pp);
+	    pp_c_calling_convention_attributes (pp_c_base (cxx_pp),
+						TYPE_ATTRIBUTES (sub));
 	  }
 	if (TREE_CODE (t) == POINTER_TYPE)
 	  pp_character(cxx_pp, '*');
Index: gcc/gcc/c-family/c-pretty-print.h
===================================================================
--- gcc.orig/gcc/c-family/c-pretty-print.h	2010-12-22 13:43:53.676439000 +0100
+++ gcc/gcc/c-family/c-pretty-print.h	2010-12-22 13:45:49.479564000 +0100
@@ -176,6 +176,7 @@ void pp_c_space_for_pointer_operator (c_
 void pp_c_tree_decl_identifier (c_pretty_printer *, tree);
 void pp_c_function_definition (c_pretty_printer *, tree);
 void pp_c_attributes (c_pretty_printer *, tree);
+void pp_c_calling_convention_attributes (c_pretty_printer *, tree);
 void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type);
 void pp_c_type_qualifier_list (c_pretty_printer *, tree);
 void pp_c_parameter_type_list (c_pretty_printer *, tree);
Index: gcc/gcc/config/i386/i386.c
===================================================================
--- gcc.orig/gcc/config/i386/i386.c	2010-12-22 13:43:53.679439000 +0100
+++ gcc/gcc/config/i386/i386.c	2010-12-22 14:19:21.091890600 +0100
@@ -5116,6 +5116,46 @@ ix86_function_ok_for_sibcall (tree decl,
   return true;
 }
 
+static bool
+ix86_attribute_affects_calling_convention (tree name)
+{
+  int ident_len;
+  const char *p;
+
+  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
+  p = IDENTIFIER_POINTER (name);
+  ident_len = IDENTIFIER_LENGTH (name);
+
+  if (ident_len > 4 && p[0] == '_' && p[1] == '_' && p[ident_len - 1] == '_'
+      && p[ident_len - 2] == '_')
+    {
+      ident_len -= 4;
+      p += 2;
+    }
+  switch (ident_len)
+    {
+    case 5:
+      if (!strncmp (p, "cdecl", 5))
+        return true;
+      break;
+    case 6:
+      if (!strncmp (p, "ms_abi", 6))
+        return true;
+      break;
+    case 7:
+      if (!strncmp (p, "regparm", 7) || !strncmp (p, "stdcall", 7))
+        return true;
+      break;
+    case 8:
+      if (!strncmp (p, "thiscall", 8) || !strncmp (p, "fastcall", 8)
+          || !strncmp (p, "sysv_abi", 8))
+        return true;
+      break;
+    }
+
+  return false;
+}
+
 /* Handle "cdecl", "stdcall", "fastcall", "regparm", "thiscall",
    and "sseregparm" calling convention attributes;
    arguments as in struct attribute_spec.handler.  */
@@ -34813,6 +34853,10 @@ ix86_autovectorize_vector_sizes (void)
 #undef TARGET_CONDITIONAL_REGISTER_USAGE
 #define TARGET_CONDITIONAL_REGISTER_USAGE ix86_conditional_register_usage
 
+#undef TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION
+#define TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION \
+  ix86_attribute_affects_calling_convention
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 #include "gt-i386.h"
Index: gcc/gcc/doc/tm.texi
===================================================================
--- gcc.orig/gcc/doc/tm.texi	2010-12-22 13:43:53.683439000 +0100
+++ gcc/gcc/doc/tm.texi	2010-12-22 14:35:44.981021900 +0100
@@ -9765,6 +9765,10 @@ to perform initial processing of the @sa
 @file{i386/i386.c}, for example.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION (const_tree @var{name})
+Returns @code{true} if given attribute by its @var{name} affects calling convention, otherwise @code{false}.
+@end deftypefn
+
 @deftypefn {Target Hook} bool TARGET_VALID_DLLIMPORT_ATTRIBUTE_P (const_tree @var{decl})
 @var{decl} is a variable or function with @code{__attribute__((dllimport))} specified.  Use this hook if the target needs to add extra validation checks to @code{handle_dll_attribute}.
 @end deftypefn
Index: gcc/gcc/doc/tm.texi.in
===================================================================
--- gcc.orig/gcc/doc/tm.texi.in	2010-12-22 13:43:53.000000000 +0100
+++ gcc/gcc/doc/tm.texi.in	2010-12-22 14:34:53.699443700 +0100
@@ -9729,6 +9729,8 @@ to perform initial processing of the @sa
 @file{i386/i386.c}, for example.
 @end deftypefn
 
+@hook TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION
+
 @hook TARGET_VALID_DLLIMPORT_ATTRIBUTE_P
 
 @defmac TARGET_DECLSPEC
Index: gcc/gcc/target.def
===================================================================
--- gcc.orig/gcc/target.def	2010-12-22 14:37:59.000000000 +0100
+++ gcc/gcc/target.def	2010-12-22 14:38:26.607056300 +0100
@@ -1113,6 +1113,14 @@ DEFHOOK
  tree, (tree olddecl, tree newdecl),
  merge_decl_attributes)
 
+/* Return true iff attribute NAME affects calling convention. */
+DEFHOOK
+(attribute_affects_calling_convention,
+ "Returns @code{true} if given attribute by its @var{name} affects calling\
+ convention, otherwise @code{false}.",
+ bool, (const_tree name),
+ hook_bool_const_tree_false)
+
 /* Given two types, merge their attributes and return the result.  */
 DEFHOOK
 (merge_type_attributes,

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Gabriel Dos Reis]

On Wed, Dec 22, 2010 at 9:34 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2010/12/22 Gabriel Dos Reis <gdr@integrable-solutions.net>:
>> Any reason why is_first is of type `int' as opposed to `bool'?
>> rename pp_c_attributes_calling_convention to
>> pp_c_calling_convention_attributes.
>> You should probably modify cp/cxx-pretty-print.c too
>
> Well, I was first think here to implement logic different, but you are
> right, bool is better here. I adjusted patch. Also renamed function.
>
>> I really prefer we don't add pretty printing code to cp/error.c.
>> Rather, we should be deferring to the pretty printers in
>> cp/cxx-pretty-print.c.  But, I suspect that is a battle for
>> another day.
>
> Well, I tried to do thing here first in cxx-pretty-print.c, but it
> doesn't had any effect for function pointers, so I did it just in
> error.c file.  This seems to be a different battle for a different
> day.

The problem is we are going to have the same regression when
the cp/error.c to what it should be (as originally plan).
So, my suggestion to is to add similar code to cp/cxx-pretty-print.c.
Patch approved with that modification.

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

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

2010/12/22 Gabriel Dos Reis <gdr@integrable-solutions.net>:
> On Wed, Dec 22, 2010 at 9:34 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> 2010/12/22 Gabriel Dos Reis <gdr@integrable-solutions.net>:
>>> Any reason why is_first is of type `int' as opposed to `bool'?
>>> rename pp_c_attributes_calling_convention to
>>> pp_c_calling_convention_attributes.
>>> You should probably modify cp/cxx-pretty-print.c too
>>
>> Well, I was first think here to implement logic different, but you are
>> right, bool is better here. I adjusted patch. Also renamed function.
>>
>>> I really prefer we don't add pretty printing code to cp/error.c.
>>> Rather, we should be deferring to the pretty printers in
>>> cp/cxx-pretty-print.c.  But, I suspect that is a battle for
>>> another day.
>>
>> Well, I tried to do thing here first in cxx-pretty-print.c, but it
>> doesn't had any effect for function pointers, so I did it just in
>> error.c file.  This seems to be a different battle for a different
>> day.
>
> The problem is we are going to have the same regression when
> the cp/error.c to what it should be (as originally plan).
> So, my suggestion to is to add similar code to cp/cxx-pretty-print.c.
> Patch approved with that modification.
>


Good, I added code to cp/cxx-pretty-print.c (pp_cxx_ptr_operator).

Regards,
Kai

[-- Attachment #2: dispcallcvt.txt --]
[-- Type: text/plain, Size: 7582 bytes --]

Index: gcc/gcc/c-family/c-pretty-print.c
===================================================================
--- gcc.orig/gcc/c-family/c-pretty-print.c	2010-12-22 16:29:55.231892000 +0100
+++ gcc/gcc/c-family/c-pretty-print.c	2010-12-22 16:39:57.923249700 +0100
@@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.  
 #include "tree-pretty-print.h"
 #include "tree-iterator.h"
 #include "diagnostic.h"
+#include "target.h"
 
 /* Translate if being used for diagnostics, but not for dump files or
    __PRETTY_FUNCTION.  */
@@ -460,6 +461,7 @@ pp_c_specifier_qualifier_list (c_pretty_
 	  {
 	    pp_c_whitespace (pp);
 	    pp_c_left_paren (pp);
+	    pp_c_calling_convention_attributes (pp, TYPE_ATTRIBUTES (pointee));
 	  }
 	else if (!c_dialect_cxx ())
 	  pp_c_whitespace (pp);
@@ -790,6 +792,45 @@ pp_c_attributes (c_pretty_printer *pp, t
   pp_c_right_paren (pp);
 }
 
+/* Pretty-print ATTRIBUTES using GNU C extension syntax for calling
+   convention affecting attributes.  */
+
+void
+pp_c_calling_convention_attributes (c_pretty_printer *pp, tree a)
+{
+  bool is_first = true;
+
+  if (a == NULL_TREE)
+    return;
+
+  for (; a != NULL_TREE; a = TREE_CHAIN (a))
+    {
+      if (!targetm.attribute_affects_calling_convention (TREE_PURPOSE (a)))
+        continue;
+      if (is_first)
+	{
+	  pp_c_ws_string (pp, "__attribute__");
+	  pp_c_left_paren (pp);
+	  pp_c_left_paren (pp);
+	  is_first = false;
+	}
+      else
+	{
+	  pp_separate_with (pp, ',');
+	}
+      pp_tree_identifier (pp, TREE_PURPOSE (a));
+      if (TREE_VALUE (a))
+	pp_c_call_argument_list (pp, TREE_VALUE (a));
+    }
+
+  if (!is_first)
+    {
+      pp_c_right_paren (pp);
+      pp_c_right_paren (pp);
+      pp_c_whitespace (pp);
+    }
+}
+
 /* function-definition:
       declaration-specifiers declarator compound-statement  */
 
Index: gcc/gcc/cp/error.c
===================================================================
--- gcc.orig/gcc/cp/error.c	2010-12-22 16:29:55.237892000 +0100
+++ gcc/gcc/cp/error.c	2010-12-22 16:39:58.001375200 +0100
@@ -661,6 +661,8 @@ dump_type_prefix (tree t, int flags)
 	  {
 	    pp_cxx_whitespace (cxx_pp);
 	    pp_cxx_left_paren (cxx_pp);
+	    pp_c_calling_convention_attributes (pp_c_base (cxx_pp),
+						TYPE_ATTRIBUTES (sub));
 	  }
 	if (TREE_CODE (t) == POINTER_TYPE)
 	  pp_character(cxx_pp, '*');
Index: gcc/gcc/c-family/c-pretty-print.h
===================================================================
--- gcc.orig/gcc/c-family/c-pretty-print.h	2010-12-22 16:29:55.232892000 +0100
+++ gcc/gcc/c-family/c-pretty-print.h	2010-12-22 16:39:58.032625400 +0100
@@ -176,6 +176,7 @@ void pp_c_space_for_pointer_operator (c_
 void pp_c_tree_decl_identifier (c_pretty_printer *, tree);
 void pp_c_function_definition (c_pretty_printer *, tree);
 void pp_c_attributes (c_pretty_printer *, tree);
+void pp_c_calling_convention_attributes (c_pretty_printer *, tree);
 void pp_c_cv_qualifiers (c_pretty_printer *pp, int qualifiers, bool func_type);
 void pp_c_type_qualifier_list (c_pretty_printer *, tree);
 void pp_c_parameter_type_list (c_pretty_printer *, tree);
Index: gcc/gcc/config/i386/i386.c
===================================================================
--- gcc.orig/gcc/config/i386/i386.c	2010-12-22 16:29:55.235892000 +0100
+++ gcc/gcc/config/i386/i386.c	2010-12-22 16:39:58.048250500 +0100
@@ -5116,6 +5116,46 @@ ix86_function_ok_for_sibcall (tree decl,
   return true;
 }
 
+static bool
+ix86_attribute_affects_calling_convention (tree name)
+{
+  int ident_len;
+  const char *p;
+
+  gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE);
+  p = IDENTIFIER_POINTER (name);
+  ident_len = IDENTIFIER_LENGTH (name);
+
+  if (ident_len > 4 && p[0] == '_' && p[1] == '_' && p[ident_len - 1] == '_'
+      && p[ident_len - 2] == '_')
+    {
+      ident_len -= 4;
+      p += 2;
+    }
+  switch (ident_len)
+    {
+    case 5:
+      if (!strncmp (p, "cdecl", 5))
+        return true;
+      break;
+    case 6:
+      if (!strncmp (p, "ms_abi", 6))
+        return true;
+      break;
+    case 7:
+      if (!strncmp (p, "regparm", 7) || !strncmp (p, "stdcall", 7))
+        return true;
+      break;
+    case 8:
+      if (!strncmp (p, "thiscall", 8) || !strncmp (p, "fastcall", 8)
+          || !strncmp (p, "sysv_abi", 8))
+        return true;
+      break;
+    }
+
+  return false;
+}
+
 /* Handle "cdecl", "stdcall", "fastcall", "regparm", "thiscall",
    and "sseregparm" calling convention attributes;
    arguments as in struct attribute_spec.handler.  */
@@ -34813,6 +34853,10 @@ ix86_autovectorize_vector_sizes (void)
 #undef TARGET_CONDITIONAL_REGISTER_USAGE
 #define TARGET_CONDITIONAL_REGISTER_USAGE ix86_conditional_register_usage
 
+#undef TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION
+#define TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION \
+  ix86_attribute_affects_calling_convention
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 \f
 #include "gt-i386.h"
Index: gcc/gcc/doc/tm.texi
===================================================================
--- gcc.orig/gcc/doc/tm.texi	2010-12-22 16:29:55.239892000 +0100
+++ gcc/gcc/doc/tm.texi	2010-12-22 16:39:58.157626200 +0100
@@ -9765,6 +9765,10 @@ to perform initial processing of the @sa
 @file{i386/i386.c}, for example.
 @end deftypefn
 
+@deftypefn {Target Hook} bool TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION (const_tree @var{name})
+Returns @code{true} if given attribute by its @var{name} affects calling convention, otherwise @code{false}.
+@end deftypefn
+
 @deftypefn {Target Hook} bool TARGET_VALID_DLLIMPORT_ATTRIBUTE_P (const_tree @var{decl})
 @var{decl} is a variable or function with @code{__attribute__((dllimport))} specified.  Use this hook if the target needs to add extra validation checks to @code{handle_dll_attribute}.
 @end deftypefn
Index: gcc/gcc/doc/tm.texi.in
===================================================================
--- gcc.orig/gcc/doc/tm.texi.in	2010-12-22 16:29:55.241892000 +0100
+++ gcc/gcc/doc/tm.texi.in	2010-12-22 16:39:58.282627000 +0100
@@ -9729,6 +9729,8 @@ to perform initial processing of the @sa
 @file{i386/i386.c}, for example.
 @end deftypefn
 
+@hook TARGET_ATTRIBUTE_AFFECTS_CALLING_CONVENTION
+
 @hook TARGET_VALID_DLLIMPORT_ATTRIBUTE_P
 
 @defmac TARGET_DECLSPEC
Index: gcc/gcc/target.def
===================================================================
--- gcc.orig/gcc/target.def	2010-12-22 16:29:55.243892000 +0100
+++ gcc/gcc/target.def	2010-12-22 16:39:58.282627000 +0100
@@ -1113,6 +1113,14 @@ DEFHOOK
  tree, (tree olddecl, tree newdecl),
  merge_decl_attributes)
 
+/* Return true iff attribute NAME affects calling convention. */
+DEFHOOK
+(attribute_affects_calling_convention,
+ "Returns @code{true} if given attribute by its @var{name} affects calling\
+ convention, otherwise @code{false}.",
+ bool, (const_tree name),
+ hook_bool_const_tree_false)
+
 /* Given two types, merge their attributes and return the result.  */
 DEFHOOK
 (merge_type_attributes,
Index: gcc/gcc/cp/cxx-pretty-print.c
===================================================================
--- gcc.orig/gcc/cp/cxx-pretty-print.c	2010-12-22 12:10:20.000000000 +0100
+++ gcc/gcc/cp/cxx-pretty-print.c	2010-12-22 16:48:14.020174700 +0100
@@ -1323,6 +1323,9 @@ pp_cxx_ptr_operator (cxx_pretty_printer 
       if (TREE_CODE (TREE_TYPE (t)) == POINTER_TYPE
 	  || TYPE_PTR_TO_MEMBER_P (TREE_TYPE (t)))
 	pp_cxx_ptr_operator (pp, TREE_TYPE (t));
+      pp_c_calling_convention_attributes (pp_c_base (pp),
+					  TYPE_ATTRIBUTES (TREE_TYPE (t)));
+
       if (TREE_CODE (t) == POINTER_TYPE)
 	{
 	  pp_star (pp);

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Joseph S. Myers]

On Wed, 22 Dec 2010, Kai Tietz wrote:

> Hi,  this patch fixes the bug described at
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15774 in bugzilla.

Could you please explain more about how and why it fixes the bug?  The bug 
described there is:

    The C++ frontend doesn't always call targetm.compare_type_attributes
    when comparing function decls in decl.c: decls_match.  As a result
    conflicting decls are not diagnosed.

But your patch doesn't change the compatibility testing at all; it just 
changes how things are printed.  Is there code somewhere that generates 
strings for type names and compares those strings to determine 
compatibility?  If so, where?  If not, how does this patch fix the bug?  
In any case, the patch does not include testcases, and failure to diagnose 
invalid code, or bad diagnostics for such code, are exactly the sort of 
things that should be easy to test in the testsuite, so it's not OK 
without testcases or proper explanation of why they are hard, and should 
not have been approved without such testcases.

For compatibility testing, targetm.comp_type_attributes is the right thing 
to use; no new hook should be needed.  For printing, I don't see the need 
for a new hook either.  Why is it appropriate to print only attributes 
that pass the new hook?  Shouldn't you rather be printing all attributes, 
regardless of whether they affect the calling convention, since they are 
all part of the type in GNU terms?

If it is appropriate to condition printing on that condition, it would 
seem that you should share code between the compatibility testing 
(ix86_comp_type_attributes) and your new hook rather than duplicating 
lists of attributes with type compatibility issues.  Why does this patch 
not share code like that?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Gabriel Dos Reis]

On Wed, Dec 22, 2010 at 10:31 AM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Wed, 22 Dec 2010, Kai Tietz wrote:
>
>> Hi,  this patch fixes the bug described at
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15774 in bugzilla.
>
> Could you please explain more about how and why it fixes the bug?  The bug
> described there is:
>
>    The C++ frontend doesn't always call targetm.compare_type_attributes
>    when comparing function decls in decl.c: decls_match.  As a result
>    conflicting decls are not diagnosed.
>
> But your patch doesn't change the compatibility testing at all; it just
> changes how things are printed.  Is there code somewhere that generates
> strings for type names and compares those strings to determine
> compatibility?

I don't think the patch actually implement compatibility checking.
What it does is to print the conflicting attributes during
diagnostic report.  And that is something that needed to be fixed.
Also, I am skeptical that we should print all attributes.

I think the actual code generation stuff is orthogonal.

-- Gaby

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

2010/12/22 Joseph S. Myers <joseph@codesourcery.com>:
> On Wed, 22 Dec 2010, Kai Tietz wrote:
>
>> Hi,  this patch fixes the bug described at
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15774 in bugzilla.
>
> Could you please explain more about how and why it fixes the bug?  The bug
> described there is:
>
>    The C++ frontend doesn't always call targetm.compare_type_attributes
>    when comparing function decls in decl.c: decls_match.  As a result
>    conflicting decls are not diagnosed.
>
> But your patch doesn't change the compatibility testing at all; it just
> changes how things are printed.  Is there code somewhere that generates
> strings for type names and compares those strings to determine
> compatibility?  If so, where?  If not, how does this patch fix the bug?
> In any case, the patch does not include testcases, and failure to diagnose
> invalid code, or bad diagnostics for such code, are exactly the sort of
> things that should be easy to test in the testsuite, so it's not OK
> without testcases or proper explanation of why they are hard, and should
> not have been approved without such testcases.
>
> For compatibility testing, targetm.comp_type_attributes is the right thing
> to use; no new hook should be needed.  For printing, I don't see the need
> for a new hook either.  Why is it appropriate to print only attributes
> that pass the new hook?  Shouldn't you rather be printing all attributes,
> regardless of whether they affect the calling convention, since they are
> all part of the type in GNU terms?
>
> If it is appropriate to condition printing on that condition, it would
> seem that you should share code between the compatibility testing
> (ix86_comp_type_attributes) and your new hook rather than duplicating
> lists of attributes with type compatibility issues.  Why does this patch
> not share code like that?
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
>

Well, a test is in PR, I can one explicit here, nevertheless we should
have for this already enough tests in testsuite.
For the rest of your reply, I can see a relation to the subject of the
patch (and PR).

Regards,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Joseph S. Myers]

On Wed, 22 Dec 2010, Kai Tietz wrote:

> Well, a test is in PR, I can one explicit here, nevertheless we should
> have for this already enough tests in testsuite.

What existing test in the testsuite fails before the patch and passes 
after it, then?

Among other things, the testsuite should verify that previous bugs do not 
reappear.  That means that if you don't fix an existing failing test, you 
should add a new one, that fails without the patch applied but passes once 
it is applied.  And there should be tests covering as much of the patch as 
possible; if you remove the new handling for any one of the attributes 
there, that should cause a test failure.

> For the rest of your reply, I can see a relation to the subject of the
> patch (and PR).

Then please answer my points.  How does your patch cause the results on a 
testcase to change from "no diagnostic" to "diagnostic"?  (If it doesn't 
do so, please state the exact testcase, what the diagnostics are before 
the patch, what they are afterwards, and why the new version is superior.)  
Why do you need a new hook at all?  Why are you duplicating code already 
present in the x86 back end that knows about what attributes are ABI 
attributes relevant to compatibility?

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

2010/12/22 Joseph S. Myers <joseph@codesourcery.com>:
> On Wed, 22 Dec 2010, Kai Tietz wrote:
>
>> Well, a test is in PR, I can one explicit here, nevertheless we should
>> have for this already enough tests in testsuite.
>
> What existing test in the testsuite fails before the patch and passes
> after it, then?
>
> Among other things, the testsuite should verify that previous bugs do not
> reappear.  That means that if you don't fix an existing failing test, you
> should add a new one, that fails without the patch applied but passes once
> it is applied.  And there should be tests covering as much of the patch as
> possible; if you remove the new handling for any one of the attributes
> there, that should cause a test failure.
>
>> For the rest of your reply, I can see a relation to the subject of the
>> patch (and PR).
>
> Then please answer my points.  How does your patch cause the results on a
> testcase to change from "no diagnostic" to "diagnostic"?  (If it doesn't
> do so, please state the exact testcase, what the diagnostics are before
> the patch, what they are afterwards, and why the new version is superior.)
> Why do you need a new hook at all?  Why are you duplicating code already
> present in the x86 back end that knows about what attributes are ABI
> attributes relevant to compatibility?
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
>

As this patch fixes a bug (as describe in the PR) in diagnostic output
It doesn't change anything on the diagnostic logic of C, or C++.  As
we usually (and this is a good habit) not checking the type output in
testsuite, I expect here no regression. Nevertheless I can add one
explicit checking complete error/warning message for C.
ABI attributes changing calling-convention. This makes asignments of
function pointers declaring different calling conventions (like for
i386 regparm, stdcall, fastcall, thiscall, cdecl, sysv_abi, ms_abi)
incompatible. This is (or should be) diagnosed by FE, as it is
already. But the display of such a warning/error lacks those attribute
information, why check failed.

See following example:
t.c:
int foo(void);
int bar(int (__attribute__ ((regparm(3))) *arg)(void));
int doo(void)
{
  return bar(foo);
}

and simply compile it by gcc -c t.c

You will see that the warning message produced looks like that
(without the patch):
t.c: In function 'doo':
t.c:5:3: warning: passing argument 1 of 'bar' from incompatible
pointer type [enabled by default]
t.c:2:5: note: expected 'int (*)(void)' but argument is of type 'int (*)(void)'

The patch I sent simply makes displayed message understandable why
'int (*)(void)' isn't 'int (*)(void)' by displaying the callabi
attributes, which are actual the cause for the warning.

Regards,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

2010/12/22 Kai Tietz <ktietz70@googlemail.com>:
> 2010/12/22 Joseph S. Myers <joseph@codesourcery.com>:
>> On Wed, 22 Dec 2010, Kai Tietz wrote:
>>
>>> Well, a test is in PR, I can one explicit here, nevertheless we should
>>> have for this already enough tests in testsuite.
>>
>> What existing test in the testsuite fails before the patch and passes
>> after it, then?
>>
>> Among other things, the testsuite should verify that previous bugs do not
>> reappear.  That means that if you don't fix an existing failing test, you
>> should add a new one, that fails without the patch applied but passes once
>> it is applied.  And there should be tests covering as much of the patch as
>> possible; if you remove the new handling for any one of the attributes
>> there, that should cause a test failure.
>>
>>> For the rest of your reply, I can see a relation to the subject of the
>>> patch (and PR).
>>
>> Then please answer my points.  How does your patch cause the results on a
>> testcase to change from "no diagnostic" to "diagnostic"?  (If it doesn't
>> do so, please state the exact testcase, what the diagnostics are before
>> the patch, what they are afterwards, and why the new version is superior.)
>> Why do you need a new hook at all?  Why are you duplicating code already
>> present in the x86 back end that knows about what attributes are ABI
>> attributes relevant to compatibility?
>>
>> --
>> Joseph S. Myers
>> joseph@codesourcery.com
>>
>
> As this patch fixes a bug (as describe in the PR) in diagnostic output
> It doesn't change anything on the diagnostic logic of C, or C++.  As
> we usually (and this is a good habit) not checking the type output in
> testsuite, I expect here no regression. Nevertheless I can add one
> explicit checking complete error/warning message for C.
> ABI attributes changing calling-convention. This makes asignments of
> function pointers declaring different calling conventions (like for
> i386 regparm, stdcall, fastcall, thiscall, cdecl, sysv_abi, ms_abi)
> incompatible. This is (or should be) diagnosed by FE, as it is
> already. But the display of such a warning/error lacks those attribute
> information, why check failed.
>
> See following example:
> t.c:
> int foo(void);
> int bar(int (__attribute__ ((regparm(3))) *arg)(void));
> int doo(void)
> {
>  return bar(foo);
> }
>
> and simply compile it by gcc -c t.c
>
> You will see that the warning message produced looks like that
> (without the patch):
> t.c: In function 'doo':
> t.c:5:3: warning: passing argument 1 of 'bar' from incompatible
> pointer type [enabled by default]
> t.c:2:5: note: expected 'int (*)(void)' but argument is of type 'int (*)(void)'
>
> The patch I sent simply makes displayed message understandable why
> 'int (*)(void)' isn't 'int (*)(void)' by displaying the callabi
> attributes, which are actual the cause for the warning.
>
> Regards,
> Kai
>
> --
> |  (\_/) This is Bunny. Copy and paste
> | (='.'=) Bunny into your signature to help
> | (")_(") him gain world domination
>

PS: Use here 32-bit to get warnings displayed.

Regards,
Kai

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Joseph S. Myers]

On Wed, 22 Dec 2010, Kai Tietz wrote:

> As this patch fixes a bug (as describe in the PR) in diagnostic output

I see nothing in PR 15774 that is relevant to this patch or the bug you 
now describe.  Have you given the wrong PR number?  That PR is, again, 
"Conflicting function decls not diagnosed".  It's not about bad or 
confusing diagnostics; it's about missing diagnostics.

> t.c:2:5: note: expected 'int (*)(void)' but argument is of type 'int (*)(void)'

That's a bug, but not 15774.  Please try again, resubmitting the patch 
under an accurate subject with more attention to writing it up accurately; 
every hour you spend on writing up patches carefully and accurately saves 
much more time for hundreds of readers.

You don't appear to have addressed my point that there should be one and 
only one point in the x86 back end that knows the list of ABI attributes; 
not duplicate lists in your new hook and in ix86_comp_type_attributes.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Jason Merrill]

On 12/22/2010 11:31 AM, Joseph S. Myers wrote:
> For compatibility testing, targetm.comp_type_attributes is the right thing
> to use; no new hook should be needed.  For printing, I don't see the need
> for a new hook either.  Why is it appropriate to print only attributes
> that pass the new hook?  Shouldn't you rather be printing all attributes,
> regardless of whether they affect the calling convention, since they are
> all part of the type in GNU terms?

I'm inclined to agree.

Jason

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

2010/12/22 Jason Merrill <jason@redhat.com>:
> On 12/22/2010 11:31 AM, Joseph S. Myers wrote:
>>
>> For compatibility testing, targetm.comp_type_attributes is the right thing
>> to use; no new hook should be needed.  For printing, I don't see the need
>> for a new hook either.  Why is it appropriate to print only attributes
>> that pass the new hook?  Shouldn't you rather be printing all attributes,
>> regardless of whether they affect the calling convention, since they are
>> all part of the type in GNU terms?
>
> I'm inclined to agree.
>
> Jason
>

Well, I talked yesterday to Ian about that, and I thought about adding
the attribute print even for tree-pretty-printer. Ian's said, that he
would accept here the printing of calling-convention attributes, but
he was strict against displaying all type attributes. So I added a
this hook for checking this, as obviously the comp_type_attributes
hook isn't usable for making this decission. Of course, if everybody
agrees, I can change the patch so that it displays all attributes.

Regards,
Kai

-- 
|  (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| (")_(") him gain world domination

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

2010/12/22 Kai Tietz <ktietz70@googlemail.com>:
> 2010/12/22 Jason Merrill <jason@redhat.com>:
>> On 12/22/2010 11:31 AM, Joseph S. Myers wrote:
>>>
>>> For compatibility testing, targetm.comp_type_attributes is the right thing
>>> to use; no new hook should be needed.  For printing, I don't see the need
>>> for a new hook either.  Why is it appropriate to print only attributes
>>> that pass the new hook?  Shouldn't you rather be printing all attributes,
>>> regardless of whether they affect the calling convention, since they are
>>> all part of the type in GNU terms?
>>
>> I'm inclined to agree.
>>
>> Jason
>>
>
> Well, I talked yesterday to Ian about that, and I thought about adding
> the attribute print even for tree-pretty-printer. Ian's said, that he
> would accept here the printing of calling-convention attributes, but
> he was strict against displaying all type attributes. So I added a
> this hook for checking this, as obviously the comp_type_attributes
> hook isn't usable for making this decission. Of course, if everybody
> agrees, I can change the patch so that it displays all attributes.
>
> Regards,
> Kai

Well, I notice that I did a pasto and used wrong PR number. Sorry for
that. It is http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12171 I meant.

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Kai Tietz]

2010/12/22 Kai Tietz <ktietz70@googlemail.com>:
> 2010/12/22 Kai Tietz <ktietz70@googlemail.com>:
>> 2010/12/22 Jason Merrill <jason@redhat.com>:
>>> On 12/22/2010 11:31 AM, Joseph S. Myers wrote:
>>>>
>>>> For compatibility testing, targetm.comp_type_attributes is the right thing
>>>> to use; no new hook should be needed.  For printing, I don't see the need
>>>> for a new hook either.  Why is it appropriate to print only attributes
>>>> that pass the new hook?  Shouldn't you rather be printing all attributes,
>>>> regardless of whether they affect the calling convention, since they are
>>>> all part of the type in GNU terms?
>>>
>>> I'm inclined to agree.
>>>
>>> Jason
>>>
>>
>> Well, I talked yesterday to Ian about that, and I thought about adding
>> the attribute print even for tree-pretty-printer. Ian's said, that he
>> would accept here the printing of calling-convention attributes, but
>> he was strict against displaying all type attributes. So I added a
>> this hook for checking this, as obviously the comp_type_attributes
>> hook isn't usable for making this decission. Of course, if everybody
>> agrees, I can change the patch so that it displays all attributes.
>>
>> Regards,
>> Kai
>
> Well, I notice that I did a pasto and used wrong PR number. Sorry for
> that. It is http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12171 I meant.
>

I am working on that, too.  But this (PR/15774) problem is nothing for
stage 3 phase AFAICS. It would need to handle the diagnostic of the
comp_type_attributes-hook more strictly and add the missing places for
diagnostics.

Sorry,
Kai

Re: [patch c,c++,i386]:PR/15774 - Conflicting function decls not diagnosed

[Gabriel Dos Reis]

On Wed, Dec 22, 2010 at 12:03 PM, Kai Tietz <ktietz70@googlemail.com> wrote:

> Well, I talked yesterday to Ian about that, and I thought about adding
> the attribute print even for tree-pretty-printer. Ian's said, that he
> would accept here the printing of calling-convention attributes, but
> he was strict against displaying all type attributes.

I strongly agree with that.

-- Gaby