[RFA] PR c++/41020

[RFA] PR c++/41020

[Dodji Seketeli]

Hello,

If a builtin function is declared as extern "C", G++ forbids it's
re-declaration, unless the re-declaration is extern "C" as well.

The problem is G++ applies this constraint to friend function re-declarations
as well.

The [class.friend] section of the c++ standards reads:

~=~
4. "A function first declared in a friend declaration has external linkage
Otherwise, the function retains its previous linkage"
~=~

So I am proposing the patch below that relaxes the builtin function
constaint for friend function declarations.

Tested against trunk on x86-64-unknown-linux-gnu.

Comments ?

commit e0e60dd248a92c9743a33e56255d4e5b4017be2d
Author: Dodji Seketeli <dodji@redhat.com>
Date:   Fri Oct 23 21:15:12 2009 +0200

    Fix PR c++/41020
    
    gcc/cp/ChangeLog:
    
    	PR c++/41020
    	* decl.c (decls_match_1): Break this out from decl_match.
    	Make it aware of function friend-ness.
    	(decls_match): Use the new decls_match_1.
    	(duplicate_decls): Likewise.
    
    gcc/testsuite/ChangeLog:
    	PR c++/41020
    	* g++.dg/lookup/friend16.C: New test.

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 5eb389f..7c01ee2 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -104,6 +104,7 @@ static void store_parm_decls (tree);
 static void initialize_local_var (tree, tree);
 static void expand_static_init (tree, tree);
 static tree next_initializable_field (tree);
+static int decls_match_1 (tree, tree, bool);
 
 /* The following symbols are subsumed in the cp_global_trees array, and
    listed here individually for documentation purposes.
@@ -899,6 +900,14 @@ push_local_name (tree decl)
 int
 decls_match (tree newdecl, tree olddecl)
 {
+  return decls_match_1 (newdecl, olddecl, /* newdecl_is_friend  */ false);
+}
+
+/* Subroutine of decls_match.  */
+
+static int
+decls_match_1 (tree newdecl, tree olddecl, bool newdecl_is_friend)
+{
   int types_match;
 
   if (newdecl == olddecl)
@@ -934,9 +943,11 @@ decls_match (tree newdecl, tree olddecl)
 
 #ifdef NO_IMPLICIT_EXTERN_C
       /* A new declaration doesn't match a built-in one unless it
-	 is also extern "C".  */
+	 is also extern "C". Friend function re-declarations retain the
+	 the linkage of the original declaration though.  */
       if (DECL_BUILT_IN (olddecl)
-	  && DECL_EXTERN_C_P (olddecl) && !DECL_EXTERN_C_P (newdecl))
+	  && DECL_EXTERN_C_P (olddecl) && !DECL_EXTERN_C_P (newdecl)
+	  && !newdecl_is_friend)
 	return 0;
 #endif
 
@@ -1122,7 +1133,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend)
   if (newdecl == olddecl)
     return olddecl;
 
-  types_match = decls_match (newdecl, olddecl);
+  types_match = decls_match_1 (newdecl, olddecl, newdecl_is_friend);
 
   /* If either the type of the new decl or the type of the old decl is an
      error_mark_node, then that implies that we have already issued an
diff --git a/gcc/testsuite/g++.dg/lookup/friend16.C b/gcc/testsuite/g++.dg/lookup/friend16.C
new file mode 100644
index 0000000..e2c7169
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lookup/friend16.C
@@ -0,0 +1,21 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin PR c++/41020
+// { dg-do compile }
+
+extern "C"
+{
+  int fork (void);
+};
+
+class frok
+{
+  int this_errno;
+  friend int fork (void);
+};
+
+extern "C" int
+fork (void)
+{
+  frok grouped;
+  return grouped.this_errno;
+}

-- 
Dodji Seketeli
Red Hat

Re: [RFA] PR c++/41020

[Jason Merrill]

What happens with block-scope extern declarations?

Jason

Re: [RFA] PR c++/41020

[Dodji Seketeli]

On Fri, Oct 23, 2009 at 01:07:07PM -0700, Jason Merrill wrote:
> What happens with block-scope extern declarations?

This small example compiles fine with the patch:

void
foo ()
{
  extern int fork (void);
}

class frok
{
  int this_errno;
  friend int fork (void);
};

int
fork (void)
{
  frok grouped;
  return grouped.this_errno;
}


-- 
Dodji Seketeli
Red Hat

Re: [RFA] PR c++/41020

[Jason Merrill]

On 10/23/2009 02:27 PM, Dodji Seketeli wrote:
> On Fri, Oct 23, 2009 at 01:07:07PM -0700, Jason Merrill wrote:
>> What happens with block-scope extern declarations?
>
> This small example compiles fine with the patch:
>
> void
> foo ()
> {
>    extern int fork (void);
> }
>
> class frok
> {
>    int this_errno;
>    friend int fork (void);
> };
>
> int
> fork (void)
> {
>    frok grouped;
>    return grouped.this_errno;
> }

Does it also compile without the patch?  What I was trying to get at 
was, are we doing something else to make block-scope externs work that 
we should also do for friend functions?  I don't know the answer.

In any case, a friend declaration should not match a builtin 
declaration.  It shouldn't give an error, either, it should declare a 
different function.

Jason

Re: [RFA] PR c++/41020

[Dave Korn]

Jason Merrill wrote:

> In any case, a friend declaration should not match a builtin
> declaration.  It shouldn't give an error, either, it should declare a
> different function.

  How do I make a non-friend declaration for that "different" function, so
that I can specify an extern C "previous linkage" for the friend declaration
to "retain"?

    cheers,
      DaveK

Re: [RFA] PR c++/41020

[Jason Merrill]

On 10/23/2009 09:14 PM, Dave Korn wrote:
> How do I make a non-friend declaration for that "different" function, so
> that I can specify an extern C "previous linkage" for the friend declaration
> to "retain"?

The usual ways:

   #include <unistd.h>

or

   extern "C" int fork();

or whatever the signature should be.

Jason

Re: [RFA] PR c++/41020

[Jason Merrill]

The problem in the PR is that G++ is failing to distinguish between the 
implicit built-in declaration (which should be overwritten by a friend 
declaration) and an explicit declaration of a built-in (which should be 
referenced by a friend declaration).

Jason

Re: [RFA] PR c++/41020

[Dave Korn]

Jason Merrill wrote:
> The problem in the PR is that G++ is failing to distinguish between the
> implicit built-in declaration (which should be overwritten by a friend
> declaration) and an explicit declaration of a built-in (which should be
> referenced by a friend declaration).

  This is similar to the problem with printf-format attributes on builtins and
explicit prototypes that Ralf C. just posted about on the main list, isn't it?
 Ah, that maybe why Ralf Cc'd you on the thread.

    cheers,
      DaveK

Re: [RFA] PR c++/41020

[Dodji Seketeli]

On Fri, Oct 23, 2009 at 04:51:18PM -0700, Jason Merrill wrote:
> Does it also compile without the patch?  What I was trying to get at  
> was, are we doing something else to make block-scope externs work that  
> we should also do for friend functions?  I don't know the answer.

Thanks for clarifying your question. I didn't actually got what you wanted
at first.

So I looked into what happens when compiling the following program:

~=~
extern "C" int fork (void);

void
foo ()
{
  extern int fork (void);
  fork ();
}

extern "C"
int
fork (void)
{
  return 0;
}
~=~

It compiles fine, but looking at the generated assembly, I can see this in
the body of the foo function:

 call    _Z4forkv

So we are calling a version of fork that has C++ linkage, where I think we
should be calling the version of fork with C linkage.

In pushdecl_maybe_friend, we detect the extern function re-declaration
we but we fail to link the re-declaration to the previous extern "C"
declaration because decls_match fails at:

 if (t && t != error_mark_node)
        {
          if (different_binding_level)
            {
              if (decls_match (x, t))
                /* The standard only says that the local extern
                   inherits linkage from the previous decl; in
                   particular, default args are not shared.  Add
                   the decl into a hash table to make sure only
                   the previous decl in this case is seen by the
                   middle end.  */
                {

That linking should have taken place in the body of the if (decls_match (x, t))).
And of course decls_match fails for the same reason it is failing in
duplicate_decls for the friend case.

So what happens instead is that the re-declared decl is pushed locally in
foo as if it was a new declaration with c++ linkage.

> In any case, a friend declaration should not match a builtin  
> declaration.  It shouldn't give an error, either, it should declare a  
> different function.

From what you said in http://gcc.gnu.org/ml/gcc-patches/2009-10/msg01501.html
could we say that in this case (block-scope extern or friend declarations
coming after explicit builtin declarations) we should match the
explicit extern "C" declaration of the builtin and thus reference it ?

If we can say that then I think the current handling of block-scope extern
declarations should be changed so that we can match explicitely declared
builtin functions.

So I have updated the patch to make decls_match use DECL_IS_BUILTIN instead of
DECL_BUILT_IN. From what I understand, I think the former doesn't catch
explicit declarations of builtin functions.

I have added more regression tests to the patch as well.

Tested against trunk on x86-64-unknown-linux-gnu.

Thanks.

commit ebae280b67752cf5d2a537e5157cb3c9a618a6ed
Author: Dodji Seketeli <dodji@redhat.com>
Date:   Fri Oct 23 21:15:12 2009 +0200

    Fix PR c++/41020
    
    gcc/cp/ChangeLog:
    
    	PR c++/41020
    	* decl.c (decls_match): Use DECL_IS_BUILTIN instead of
    	DECL_BUILT_IN.
    
    gcc/testsuite/ChangeLog:
    	PR c++/41020
    	* g++.dg/lookup/extern-c-redecl2.C: New test.
    	* g++.dg/lookup/extern-c-redecl3.C: Likewise.
    	* g++.dg/lookup/extern-c-redecl4.C: Likewise.
    	* g++.dg/lookup/extern-c-redecl5.C: Likewise.

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 5eb389f..c772ca5 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -935,7 +935,7 @@ decls_match (tree newdecl, tree olddecl)
 #ifdef NO_IMPLICIT_EXTERN_C
       /* A new declaration doesn't match a built-in one unless it
 	 is also extern "C".  */
-      if (DECL_BUILT_IN (olddecl)
+      if (DECL_IS_BUILTIN (olddecl)
 	  && DECL_EXTERN_C_P (olddecl) && !DECL_EXTERN_C_P (newdecl))
 	return 0;
 #endif
diff --git a/gcc/testsuite/g++.dg/lookup/extern-c-redecl2.C b/gcc/testsuite/g++.dg/lookup/extern-c-redecl2.C
new file mode 100644
index 0000000..055148f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lookup/extern-c-redecl2.C
@@ -0,0 +1,21 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin PR c++/41020
+// { dg-do compile }
+
+extern "C"
+{
+  int fork (void);
+}
+
+class frok
+{
+  int this_errno;
+  friend int fork (void);
+};
+
+extern "C" int
+fork (void)
+{
+  frok grouped;
+  return grouped.this_errno;
+}
diff --git a/gcc/testsuite/g++.dg/lookup/extern-c-redecl3.C b/gcc/testsuite/g++.dg/lookup/extern-c-redecl3.C
new file mode 100644
index 0000000..00ff4a9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lookup/extern-c-redecl3.C
@@ -0,0 +1,22 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin: PR c++/41020
+// { dg-do compile }
+// { dg-final { scan-assembler-not "call\[\t \]+_Z4forkv" } }
+// { dg-final { scan-assembler "call\[\t \]+fork" } }
+
+extern "C" int fork (void);
+
+void
+foo ()
+{
+  extern int fork (void);
+  fork ();
+}
+
+extern "C"
+int
+fork (void)
+{
+  return 0;
+}
+
diff --git a/gcc/testsuite/g++.dg/lookup/extern-c-redecl4.C b/gcc/testsuite/g++.dg/lookup/extern-c-redecl4.C
new file mode 100644
index 0000000..9dfa54d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lookup/extern-c-redecl4.C
@@ -0,0 +1,19 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin: PR c++/41020
+
+// Avoid the "-ansi -pedantic" option
+// { dg-options "" }
+// { dg-do compile }
+// { dg-final { scan-assembler "call\[\t \]+_Z4forkv" } }
+
+class frok
+{
+  int this_errno;
+  friend int fork (void);
+};
+
+void
+foo ()
+{
+  fork ();
+}
diff --git a/gcc/testsuite/g++.dg/lookup/extern-c-redecl5.C b/gcc/testsuite/g++.dg/lookup/extern-c-redecl5.C
new file mode 100644
index 0000000..031059f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lookup/extern-c-redecl5.C
@@ -0,0 +1,18 @@
+// Contributed by Dodji Seketeli <dodji@redhat.com>
+// Origin: PR c++/41020
+// { dg-do compile }
+
+
+class frok
+{
+  int this_errno;
+  friend int fork (void); // { dg-error "previous declaration .*?C++. linkage" }
+};
+
+extern "C" int
+fork (void) // { dg-error "conflicts with new declaration .*?C. linkage" }}
+{
+  frok grouped;
+  return grouped.this_errno;
+}
+

-- 
Dodji Seketeli
Red Hat

Re: [RFA] PR c++/41020

[Jason Merrill]

On 10/25/2009 07:49 AM, Dodji Seketeli wrote:
>      	PR c++/41020
>      	* decl.c (decls_match): Use DECL_IS_BUILTIN instead of
>      	DECL_BUILT_IN.

OK.

Jason