[patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Kai Tietz]

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

Hi,

This time with the correct version of patch and changelog for the bug
described at
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15774 in bugzilla.
It takes care about the following problematic. If there was an old
function prototype, and the new one has same or none attributes as the
old one, C++ accepts.
This behavior is unlikely to the C case, where this condition is
warned or in some cases even
errored out. If the new declaration specifies attributes, which aren't
identical to the old
declaration, an error has to be emitted.

ChangeLog gcc/cp

2010-12-22  Kai Tietz

      PR c++/15774
      * cp-tree.h (COMPARE_ALLOW_FIRST_NO_ATTRIBUTES): New define.
      * decl.c (decls_match): Compare parameter and function types
      by COMPARE_ALLOW_FIRST_NO_ATTRIBUTES.
      * typeck.c (structural_comptypes): Handle new flag.


ChangeLog gcc/testsuite

       PR c++/15774
       * g++.dg/pr15774-1.C: New test.
       * g++.dg/pr15774-2.C: New test.

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

Ok for apply?

Regards,
Kai

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

Index: gcc/gcc/cp/cp-tree.h
===================================================================
--- gcc.orig/gcc/cp/cp-tree.h	2010-12-17 21:52:25.000000000 +0100
+++ gcc/gcc/cp/cp-tree.h	2010-12-22 20:31:54.963786500 +0100
@@ -4276,6 +4276,10 @@ enum overload_flags { NO_SPECIAL = 0, DT
 				   structural. The actual comparison
 				   will be identical to
 				   COMPARE_STRICT.  */
+#define COMPARE_ALLOW_FIRST_NO_ATTRIBUTES 16
+				/* Allow identity for two types, if difference
+				   is in attributes only and first (the new)
+				   has no attributes specified.  */
 
 /* Used with push_overloaded_decl.  */
 #define PUSH_GLOBAL	     0  /* Push the DECL into namespace scope,
Index: gcc/gcc/cp/decl.c
===================================================================
--- gcc.orig/gcc/cp/decl.c	2010-12-17 21:52:25.000000000 +0100
+++ gcc/gcc/cp/decl.c	2010-12-22 20:31:29.549332800 +0100
@@ -1009,7 +1009,12 @@ decls_match (tree newdecl, tree olddecl)
 	    }
 #endif
 	  else
-	    types_match = compparms (p1, p2);
+	    types_match = compparms (p1, p2)
+			  && comptypes (TREE_TYPE (newdecl),
+					TREE_TYPE (olddecl),
+					COMPARE_REDECLARATION
+					| COMPARE_ALLOW_FIRST_NO_ATTRIBUTES
+				       );
 	}
       else
 	types_match = 0;
Index: gcc/gcc/cp/typeck.c
===================================================================
--- gcc.orig/gcc/cp/typeck.c	2010-12-17 21:52:25.000000000 +0100
+++ gcc/gcc/cp/typeck.c	2010-12-22 20:31:46.535304400 +0100
@@ -1146,6 +1146,8 @@ comp_template_parms_position (tree t1, t
 static bool
 structural_comptypes (tree t1, tree t2, int strict)
 {
+  int attr_comp = 1;
+
   if (t1 == t2)
     return true;
 
@@ -1250,7 +1252,8 @@ structural_comptypes (tree t1, tree t2,
 
     case OFFSET_TYPE:
       if (!comptypes (TYPE_OFFSET_BASETYPE (t1), TYPE_OFFSET_BASETYPE (t2),
-		      strict & ~COMPARE_REDECLARATION))
+		      strict & ~(COMPARE_REDECLARATION
+				 | COMPARE_ALLOW_FIRST_NO_ATTRIBUTES)))
 	return false;
       if (!same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
 	return false;
@@ -1338,7 +1341,16 @@ structural_comptypes (tree t1, tree t2,
   /* If we get here, we know that from a target independent POV the
      types are the same.  Make sure the target attributes are also
      the same.  */
-  return targetm.comp_type_attributes (t1, t2);
+  attr_comp = targetm.comp_type_attributes (t1, t2);
+  if (attr_comp || (TREE_CODE (t1) != FUNCTION_TYPE
+  		    && TREE_CODE (t1) != METHOD_TYPE))
+    return attr_comp;
+  /* If new type T1 has no attributes, and therefore match fails, allow
+     check if COMPARE_ALLOW_FIRST_NO_ATTRIBUTES is defined.  */
+  if ((strict & COMPARE_ALLOW_FIRST_NO_ATTRIBUTES) !=0
+      && TYPE_ATTRIBUTES (t1) == NULL)
+    attr_comp = 1;
+  return attr_comp;
 }
 
 /* Return true if T1 and T2 are related as allowed by STRICT.  STRICT
Index: gcc/gcc/testsuite/g++.dg/pr15774-1.C
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/gcc/testsuite/g++.dg/pr15774-1.C	2010-12-22 21:09:50.303928500 +0100
@@ -0,0 +1,15 @@
+// { dg-do compile { target i?86-*-* } }
+// Test that an new declartion with different attributes then old one fail.
+extern void foo (int); // { dg-error "ambiguates old declaration" }
+
+void
+bar (void)
+{
+  foo (1);
+}
+
+void __attribute__((stdcall)) foo (int i) // { dg-error "new declaration" }
+{
+}
+
+
Index: gcc/gcc/testsuite/g++.dg/pr15774-2.C
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/gcc/testsuite/g++.dg/pr15774-2.C	2010-12-22 21:08:46.691290000 +0100
@@ -0,0 +1,15 @@
+// { dg-do compile { target i?86-*-* } }
+// Test that old declaration is used, if new one has no attributes.
+extern void __attribute__((stdcall)) foo (int);
+
+void
+bar (void)
+{
+  foo (1);
+}
+
+void foo (int i)
+{
+}
+
+

Re: [patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Kai Tietz]

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

Hello,

as we discussed yesterday on IRC, I simplified patch by doing checks
for compatible attributes directly within decl, but take care the
newdecl possibly can omit attributes.

ChangeLog gcc/cp

2010-12-23  Kai Tietz

	* decl.c (decls_match): Check for FUNCTION_DECL
	also for identity of compatible attributes.


ChangeLog gcc/testsuite

2010-12-23  Kai Tietz


	* g++.dg/pr15774-1.C: New test.
	* g++.dg/pr15774-2.C: New test.

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

Regards,
Kai

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

Index: gcc/gcc/cp/decl.c
===================================================================
--- gcc.orig/gcc/cp/decl.c	2010-12-23 11:16:30.063178000 +0100
+++ gcc/gcc/cp/decl.c	2010-12-23 11:18:59.781928200 +0100
@@ -1009,7 +1009,11 @@ decls_match (tree newdecl, tree olddecl)
 	    }
 #endif
 	  else
-	    types_match = compparms (p1, p2);
+	    types_match =
+	      compparms (p1, p2)
+	      && (TYPE_ATTRIBUTES (TREE_TYPE (newdecl)) == NULL_TREE
+	          || targetm.comp_type_attributes (TREE_TYPE (newdecl),
+						   TREE_TYPE (olddecl)) != 0);
 	}
       else
 	types_match = 0;
Index: gcc/gcc/testsuite/g++.dg/pr15774-1.C
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/gcc/testsuite/g++.dg/pr15774-1.C	2010-12-23 11:18:59.813178200 +0100
@@ -0,0 +1,15 @@
+// { dg-do compile { target i?86-*-* } }
+// Test that an new declartion with different attributes then old one fail.
+extern void foo (int); // { dg-error "ambiguates old declaration" }
+
+void
+bar (void)
+{
+  foo (1);
+}
+
+void __attribute__((stdcall)) foo (int i) // { dg-error "new declaration" }
+{
+}
+
+
Index: gcc/gcc/testsuite/g++.dg/pr15774-2.C
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/gcc/testsuite/g++.dg/pr15774-2.C	2010-12-23 11:18:59.813178200 +0100
@@ -0,0 +1,15 @@
+// { dg-do compile { target i?86-*-* } }
+// Test that old declaration is used, if new one has no attributes.
+extern void __attribute__((stdcall)) foo (int);
+
+void
+bar (void)
+{
+  foo (1);
+}
+
+void foo (int i)
+{
+}
+
+

Re: [patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Jason Merrill]

OK.  Does the error message mention the attributes?

Jason

Re: [patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Kai Tietz]

2010/12/24 Jason Merrill <jason@redhat.com>:
> OK.  Does the error message mention the attributes?
>
> Jason
>

Ok, I'll apply it tomorrow. No the error message doesn't display the
attribute as it displays a decl.  This would be possibly another task
to display attributes (at least a wished limited set) also for
declarations.

Regards,
Kai

PS: Merry Christmas

Re: [patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Kai Tietz]

2010/12/24 Kai Tietz <ktietz70@googlemail.com>:
> 2010/12/24 Jason Merrill <jason@redhat.com>:
>> OK.  Does the error message mention the attributes?
>>
>> Jason
>>
>
> Ok, I'll apply it tomorrow. No the error message doesn't display the
> attribute as it displays a decl.  This would be possibly another task
> to display attributes (at least a wished limited set) also for
> declarations.
>
> Regards,
> Kai
>
> PS: Merry Christmas
>

Applied at revision 168241.

Kai

Re: [patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Jason Merrill]

On 12/24/2010 05:24 PM, Kai Tietz wrote:
> Ok, I'll apply it tomorrow. No the error message doesn't display the
> attribute as it displays a decl.  This would be possibly another task
> to display attributes (at least a wished limited set) also for
> declarations.

I think we want to print whatever attributes cause the attributed entity 
to be considered distinct from the non-attributed version, rather than a 
redeclaration.

Thinking about it that way, I think your patch actually isn't the right 
way to handle this: in pr15774-1.C, the two foos are the same foo, just 
the change in attributes is ill-formed.  So decls_match should return 1, 
but we should complain about attribute mismatch in duplicate_decls, 
perhaps just before the call to merge_decl_attributes.

Jason

Re: [patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Kai Tietz]

2010/12/25 Jason Merrill <jason@redhat.com>:
> On 12/24/2010 05:24 PM, Kai Tietz wrote:
>>
>> Ok, I'll apply it tomorrow. No the error message doesn't display the
>> attribute as it displays a decl.  This would be possibly another task
>> to display attributes (at least a wished limited set) also for
>> declarations.
>
> I think we want to print whatever attributes cause the attributed entity to
> be considered distinct from the non-attributed version, rather than a
> redeclaration.
>
> Thinking about it that way, I think your patch actually isn't the right way
> to handle this: in pr15774-1.C, the two foos are the same foo, just the
> change in attributes is ill-formed.  So decls_match should return 1, but we
> should complain about attribute mismatch in duplicate_decls, perhaps just
> before the call to merge_decl_attributes.
>
> Jason
>

Hmm, not sure. Case 2 is simply ill-formed, but doesn't cause wrong
code. But case 1 in fact can produce wrong code. See as expample

extern void foo (int);
void doo(int a) { foo(a); }
void __attribute__((stdcall)) foo (int i) { }

As here in function "doo" the "foo" gets called with cdecl
calling-convention, but its implementation gets "stdcall" one, which
are incompatible and therefore lead to wrong-code.

The -2 case doesn't have this issue

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

Re: [patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Jason Merrill]

On 12/25/2010 11:00 AM, Kai Tietz wrote:
> Hmm, not sure. Case 2 is simply ill-formed, but doesn't cause wrong
> code. But case 1 in fact can produce wrong code. See as expample

I agree that we should reject it, I'm just saying we should reject it in 
a different function.  :)

Jason

Re: [patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Dave Korn]

On 25/12/2010 10:41, Kai Tietz wrote:
> 2010/12/24 Kai Tietz <ktietz70@googlemail.com>:
>> 2010/12/24 Jason Merrill <jason@redhat.com>:
>>> OK.  Does the error message mention the attributes?
>>>
>>> Jason
>>>
>> Ok, I'll apply it tomorrow. No the error message doesn't display the
>> attribute as it displays a decl.  This would be possibly another task
>> to display attributes (at least a wished limited set) also for
>> declarations.
>>
>> Regards,
>> Kai
>>
>> PS: Merry Christmas
>>
> 
> Applied at revision 168241.

  Unfortunately this broke the use of typedefs with dll attributes on them,
breaking target-libjava build in the process.  Testcase, reduced from libjava
sources where the first build problem arises:

-----------------------snip-----------------------
#define JNICALL          __stdcall

typedef void *jobject;

class _Jv_JNIEnv
{
public:
  int x;
};

jobject _Jv_JNI_ToReflectedField (_Jv_JNIEnv*);

typedef struct _Jv_JNIEnv JNIEnv;

jobject JNICALL
_Jv_JNI_ToReflectedField (JNIEnv *env)
{
  return 0;
}
-----------------------snip-----------------------

  With your patch reverted:

> admin@ubik /gnu/gcc/obj-mingw-pr43601/gcc/pr17136
> $ ../g++.exe  -B..  -c  tc.cc  -Wall -W 2>&1 | tee pass.log
> tc.cc:17:1: warning: unused parameter 'env' [-Wunused-parameter]
> 
> admin@ubik /gnu/gcc/obj-mingw-pr43601/gcc/pr17136
> $ 

  With your patch applied:

> admin@ubik /gnu/gcc/obj-mingw-pr43601/gcc/pr17136
> $ ../g++.exe  -B..  -c  tc.cc  -Wall -W
> tc.cc: In function 'void* _Jv_JNI_ToReflectedField(JNIEnv*)':
> tc.cc:17:38: error: new declaration 'void* _Jv_JNI_ToReflectedField(JNIEnv*)'
> tc.cc:12:9: error: ambiguates old declaration 'void* _Jv_JNI_ToReflectedField(_Jv_JNIEnv*)'
> tc.cc: At global scope:
> tc.cc:17:1: warning: unused parameter 'env' [-Wunused-parameter]
> 
> admin@ubik /gnu/gcc/obj-mingw-pr43601/gcc/pr17136
> $ 

  Can you investigate please Kai?

  (And the moral of the story is: it's always a good idea to build and test
java if you're doing something that affects C++.)

    cheers,
      DaveK

Re: [patch c++]:PR/15774 - Conflicting function decls not diagnosed (this time really for 15774)

[Dave Korn]

On 04/01/2011 03:46, Dave Korn wrote:
> On 25/12/2010 10:41, Kai Tietz wrote:
>> 2010/12/24 Kai Tietz <ktietz70@googlemail.com>:
>>> 2010/12/24 Jason Merrill <jason@redhat.com>:
>>>> OK.  Does the error message mention the attributes?
>>>>
>>>> Jason
>>>>
>>> Ok, I'll apply it tomorrow. No the error message doesn't display the
>>> attribute as it displays a decl.  This would be possibly another task
>>> to display attributes (at least a wished limited set) also for
>>> declarations.
>>>
>>> Regards,
>>> Kai
>>>
>>> PS: Merry Christmas
>>>
>> Applied at revision 168241.
> 
>   Unfortunately this broke the use of typedefs with dll attributes on them,

  Um, I'm not reading and thinking clearly.  This is actually PR17136(*) come
back to haunt us.  Nothing to do with the typedef in the formal parameters,
nor dll attributes; just the missing stdcall on the function prototype that is
causing the problem.  Reduced testcase:

-------------------------------------
#define JNICALL          __stdcall

int _Jv_JNI_ToReflectedField (void);

int JNICALL
_Jv_JNI_ToReflectedField (void)
{
  return 0;
}
-------------------------------------

  As the explanation in the PR makes clear, this is a real bug in libjava,
that we have not previously diagnosed.  Danny suggested a fix in that PR; I'll
take a look at updating it for current trunk.

    cheers,
      DaveK
-- 
(*) - http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17136