restore bootstrap with a C++ compiler

restore bootstrap with a C++ compiler

[Gabriel Dos Reis]

Richard,

Your recent patch broke --enable-build-with-cxx because internal-fn.h
was using good 'ol C-style name lookup trick that behaves badly with
C++ linkage rules.  Fixed thusly.  Applied as obvious.

-- Gaby

2011-04-27  Gabriel Dos Reis  <gdr@integrable-solutions.net>

	* internal-fn.h (internal_fn_name_array): Declare.
	(internal_fn_flags_array): Likewise.

*** internal-fn.h	(revision 173070)
--- internal-fn.h	(local)
*************** enum internal_fn {
*** 30,48 ****
  /* Return the name of internal function FN.  The name is only meaningful
     for dumps; it has no linkage.  */
  
  static inline const char *
  internal_fn_name (enum internal_fn fn)
  {
-   extern const char *const internal_fn_name_array[];
    return internal_fn_name_array[(int) fn];
  }
  
  /* Return the ECF_* flags for function FN.  */
  
  static inline int
  internal_fn_flags (enum internal_fn fn)
  {
-   extern const int internal_fn_flags_array[];
    return internal_fn_flags_array[(int) fn];
  }
  
--- 30,50 ----
  /* Return the name of internal function FN.  The name is only meaningful
     for dumps; it has no linkage.  */
  
+ extern const char *const internal_fn_name_array[];
+ 
  static inline const char *
  internal_fn_name (enum internal_fn fn)
  {
    return internal_fn_name_array[(int) fn];
  }
  
  /* Return the ECF_* flags for function FN.  */
  
+ extern const int internal_fn_flags_array[];
+ 
  static inline int
  internal_fn_flags (enum internal_fn fn)
  {
    return internal_fn_flags_array[(int) fn];
  }
  

Re: restore bootstrap with a C++ compiler

[Andrew Pinski]

On Wed, Apr 27, 2011 at 6:30 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
>
> Richard,
>
> Your recent patch broke --enable-build-with-cxx because internal-fn.h
> was using good 'ol C-style name lookup trick that behaves badly with
> C++ linkage rules.  Fixed thusly.  Applied as obvious.

Can you explain some more?  Does it have do with the fact those
functions are static?  If so I think we should move away from static
functions for compiling with C++ and move to using anonymous
namespaces.

Thanks,
Andrew Pinski

Re: restore bootstrap with a C++ compiler

[Gabriel Dos Reis]

Andrew Pinski <pinskia@gmail.com> writes:

| On Wed, Apr 27, 2011 at 6:30 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
| >
| > Richard,
| >
| > Your recent patch broke --enable-build-with-cxx because internal-fn.h
| > was using good 'ol C-style name lookup trick that behaves badly with
| > C++ linkage rules.  Fixed thusly.  Applied as obvious.
| 
| Can you explain some more?  Does it have do with the fact those
| functions are static?

The issue has nothing to do with the fact that the functions are static.

A local `extern' declaration does not give the entity an external linkage
-- irrespective of the linkage of the function enclosing the declaration.
It just makes the name locally available for name lookup
purpose.   A variable declared const has
 -- external linkage by default in C
 -- internal linkage by default in C++

So before the patch: the variables had external linkage in C, but
internal linkage in C++.  That meant that a link will fail in C++, but
succeeds in C.  All my patch did was to give the external linkage
explicitly (both in C and in C++.)

|  If so I think we should move away from static
| functions for compiling with C++ and move to using anonymous
| namespaces.

I suspect that is a separate issue.

BTW, in my own codes I still use "static" for non-member functions
because the compiler will tell me when a function becomes unused.
GCC at the moment is unable to tell when a function in an unnamed
namespace is unused (that is surely a bug.)  For that reason alone, I
would recommend that we keep "static" for functions.

-- Gaby

Re: restore bootstrap with a C++ compiler

[Andrew Pinski]

On Wed, Apr 27, 2011 at 6:48 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
> A local `extern' declaration does not give the entity an external linkage
> -- irrespective of the linkage of the function enclosing the declaration.
> It just makes the name locally available for name lookup
> purpose.   A variable declared const has
>  -- external linkage by default in C
>  -- internal linkage by default in C++
>
> So before the patch: the variables had external linkage in C, but
> internal linkage in C++.  That meant that a link will fail in C++, but
> succeeds in C.  All my patch did was to give the external linkage
> explicitly (both in C and in C++.)

Then I think a better fix is to do:
Index: internal-fn.c
===================================================================
--- internal-fn.c	(revision 172940)
+++ internal-fn.c	(working copy)
@@ -27,6 +27,7 @@ along with GCC; see the file COPYING3.
 #include "gimple.h"

 /* The names of each internal function, indexed by function number.  */
+extern const char *const internal_fn_name_array[];
 const char *const internal_fn_name_array[] = {
 #define DEF_INTERNAL_FN(CODE, FLAGS) #CODE,
 #include "internal-fn.def"
@@ -35,6 +36,7 @@ const char *const internal_fn_name_array
 };

 /* The ECF_* flags of each internal function, indexed by function number.  */
+extern const int internal_fn_flags_array[];
 const int internal_fn_flags_array[] = {
 #define DEF_INTERNAL_FN(CODE, FLAGS) FLAGS,
 #include "internal-fn.def"
--- CUT ---
So nobody is tempted to use those arrays directly from the code but
rather only use the static inline functions.

Thanks,
Andrew Pinski

Re: restore bootstrap with a C++ compiler

[Gabriel Dos Reis]

Andrew Pinski <pinskia@gmail.com> writes:

| On Wed, Apr 27, 2011 at 6:48 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
| > A local `extern' declaration does not give the entity an external linkage
| > -- irrespective of the linkage of the function enclosing the declaration.
| > It just makes the name locally available for name lookup
| > purpose.   A variable declared const has
| >  -- external linkage by default in C
| >  -- internal linkage by default in C++
| >
| > So before the patch: the variables had external linkage in C, but
| > internal linkage in C++.  That meant that a link will fail in C++, but
| > succeeds in C.  All my patch did was to give the external linkage
| > explicitly (both in C and in C++.)
| 
| Then I think a better fix is to do:
| Index: internal-fn.c
| ===================================================================
| --- internal-fn.c	(revision 172940)
| +++ internal-fn.c	(working copy)
| @@ -27,6 +27,7 @@ along with GCC; see the file COPYING3.
|  #include "gimple.h"
| 
|  /* The names of each internal function, indexed by function number.  */
| +extern const char *const internal_fn_name_array[];
|  const char *const internal_fn_name_array[] = {
|  #define DEF_INTERNAL_FN(CODE, FLAGS) #CODE,
|  #include "internal-fn.def"
| @@ -35,6 +36,7 @@ const char *const internal_fn_name_array
|  };
| 
|  /* The ECF_* flags of each internal function, indexed by function number.  */
| +extern const int internal_fn_flags_array[];
|  const int internal_fn_flags_array[] = {
|  #define DEF_INTERNAL_FN(CODE, FLAGS) FLAGS,
|  #include "internal-fn.def"
| --- CUT ---
| So nobody is tempted to use those arrays directly from the code but
| rather only use the static inline functions.

Well, anybody who can put those extern declarations anywhere and use
them directly.  The real issue is that the array variables have external
linkage.

So, I think the patch is largely stylistic but equivalent; I'll defer to
you which one should "prevail".

-- Gaby

Re: restore bootstrap with a C++ compiler

[Richard Sandiford]

Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
> Andrew Pinski <pinskia@gmail.com> writes:
>
> | On Wed, Apr 27, 2011 at 6:48 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
> | > A local `extern' declaration does not give the entity an external linkage
> | > -- irrespective of the linkage of the function enclosing the declaration.
> | > It just makes the name locally available for name lookup
> | > purpose.   A variable declared const has
> | >  -- external linkage by default in C
> | >  -- internal linkage by default in C++
> | >
> | > So before the patch: the variables had external linkage in C, but
> | > internal linkage in C++.  That meant that a link will fail in C++, but
> | > succeeds in C.  All my patch did was to give the external linkage
> | > explicitly (both in C and in C++.)
> | 
> | Then I think a better fix is to do:
> | Index: internal-fn.c
> | ===================================================================
> | --- internal-fn.c	(revision 172940)
> | +++ internal-fn.c	(working copy)
> | @@ -27,6 +27,7 @@ along with GCC; see the file COPYING3.
> |  #include "gimple.h"
> | 
> |  /* The names of each internal function, indexed by function number.  */
> | +extern const char *const internal_fn_name_array[];
> |  const char *const internal_fn_name_array[] = {
> |  #define DEF_INTERNAL_FN(CODE, FLAGS) #CODE,
> |  #include "internal-fn.def"
> | @@ -35,6 +36,7 @@ const char *const internal_fn_name_array
> |  };
> | 
> |  /* The ECF_* flags of each internal function, indexed by function number.  */
> | +extern const int internal_fn_flags_array[];
> |  const int internal_fn_flags_array[] = {
> |  #define DEF_INTERNAL_FN(CODE, FLAGS) FLAGS,
> |  #include "internal-fn.def"
> | --- CUT ---
> | So nobody is tempted to use those arrays directly from the code but
> | rather only use the static inline functions.
>
> Well, anybody who can put those extern declarations anywhere and use
> them directly.  The real issue is that the array variables have external
> linkage.
>
> So, I think the patch is largely stylistic but equivalent; I'll defer to
> you which one should "prevail".

FWIW, I prefer Andrew's patch, but since yours has been applied,
I suppose there's no point changing it.

Richard

Re: restore bootstrap with a C++ compiler

[Gabriel Dos Reis]

Richard Sandiford <richard.sandiford@linaro.org> writes:

| Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
| > Andrew Pinski <pinskia@gmail.com> writes:
| >
| > | On Wed, Apr 27, 2011 at 6:48 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
| > | > A local `extern' declaration does not give the entity an external linkage
| > | > -- irrespective of the linkage of the function enclosing the declaration.
| > | > It just makes the name locally available for name lookup
| > | > purpose.   A variable declared const has
| > | >  -- external linkage by default in C
| > | >  -- internal linkage by default in C++
| > | >
| > | > So before the patch: the variables had external linkage in C, but
| > | > internal linkage in C++.  That meant that a link will fail in C++, but
| > | > succeeds in C.  All my patch did was to give the external linkage
| > | > explicitly (both in C and in C++.)
| > | 
| > | Then I think a better fix is to do:
| > | Index: internal-fn.c
| > | ===================================================================
| > | --- internal-fn.c	(revision 172940)
| > | +++ internal-fn.c	(working copy)
| > | @@ -27,6 +27,7 @@ along with GCC; see the file COPYING3.
| > |  #include "gimple.h"
| > | 
| > |  /* The names of each internal function, indexed by function number.  */
| > | +extern const char *const internal_fn_name_array[];
| > |  const char *const internal_fn_name_array[] = {
| > |  #define DEF_INTERNAL_FN(CODE, FLAGS) #CODE,
| > |  #include "internal-fn.def"
| > | @@ -35,6 +36,7 @@ const char *const internal_fn_name_array
| > |  };
| > | 
| > |  /* The ECF_* flags of each internal function, indexed by function number.  */
| > | +extern const int internal_fn_flags_array[];
| > |  const int internal_fn_flags_array[] = {
| > |  #define DEF_INTERNAL_FN(CODE, FLAGS) FLAGS,
| > |  #include "internal-fn.def"
| > | --- CUT ---
| > | So nobody is tempted to use those arrays directly from the code but
| > | rather only use the static inline functions.
| >
| > Well, anybody who can put those extern declarations anywhere and use
| > them directly.  The real issue is that the array variables have external
| > linkage.
| >
| > So, I think the patch is largely stylistic but equivalent; I'll defer to
| > you which one should "prevail".
| 
| FWIW, I prefer Andrew's patch, but since yours has been applied,
| I suppose there's no point changing it.

when we come to agree on coding style guidelines for GCC in C++, I hope we
recommend against local extern declarations.

-- Gaby

Re: restore bootstrap with a C++ compiler

[Richard Sandiford]

Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
> | FWIW, I prefer Andrew's patch, but since yours has been applied,
> | I suppose there's no point changing it.
>
> when we come to agree on coding style guidelines for GCC in C++, I hope we
> recommend against local extern declarations.

Oh, for pure C++, I definitely agree.  This sort of thing ought to
be marked as private or some such in a C++ context.  But while we're
confining ourselves to a subset of C, I think local externs are a useful
way of hiding implementation details.

Richard

Re: restore bootstrap with a C++ compiler

[Gabriel Dos Reis]

Richard Sandiford <richard.sandiford@linaro.org> writes:

| Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
| > | FWIW, I prefer Andrew's patch, but since yours has been applied,
| > | I suppose there's no point changing it.
| >
| > when we come to agree on coding style guidelines for GCC in C++, I hope we
| > recommend against local extern declarations.
| 
| Oh, for pure C++, I definitely agree.  This sort of thing ought to
| be marked as private or some such in a C++ context.  But while we're
| confining ourselves to a subset of C, I think local externs are a useful
| way of hiding implementation details.


well, please apply Andrew's and revert mine then.

-- Gaby