[c/c++ (rfa)] fix 32455

[c/c++ (rfa)] fix 32455

[Richard Henderson]

In fixing 32455, I agree with Richard Guenther that probably 
the best way to fix the bug is simply to prevent users from
re-declaring __builtin_ functions with different signatures.
Indeed, the easiest way appears to be to prevent them from
doing pretty much anything with the __builtin_ namespace.

This is an RFA for the C++ side, because while the testcase
I have appears to work, I've no idea what other kind of side
effects may be involved on that side.


r~


	* c-common.c (builtin_name_p): New.
	(disable_builtin_function, def_builtin_1): Use it.
	* c-common.h (builtin_name_p): Declare.
	* c-decl.c (grokdeclarator): Deny declarations w/ builtin_name_p.
	(grokfield): Create an error field when grokdeclarator fails.

cp/
	* decl.c (grokdeclarator): Deny declarations w/ builtin_name_p.

testsuite/
	* g++.old-deja/g++.law/operators24.C: Remove.
	* gcc.dg/torture/builtin-convert-3.c: Don't declare __builtin's.
	* gcc.dg/torture/builtin-nonneg-1.c: Likewise.
	* gcc.dg/pr32455-1.c, gcc.dg/pr32455-2.c: New.

--- gcc/c-common.c	(revision 132159)
+++ gcc/c-common.c	(local)
@@ -3966,13 +3966,19 @@ static disabled_builtin *disabled_builti
 
 static bool builtin_function_disabled_p (const char *);
 
+bool
+builtin_name_p (const char *name)
+{
+  return strncmp (name, "__builtin_", strlen ("__builtin_")) == 0;
+}
+
 /* Disable a built-in function specified by -fno-builtin-NAME.  If NAME
    begins with "__builtin_", give an error.  */
 
 void
 disable_builtin_function (const char *name)
 {
-  if (strncmp (name, "__builtin_", strlen ("__builtin_")) == 0)
+  if (builtin_name_p (name))
     error ("cannot disable built-in function %qs", name);
   else
     {
@@ -4019,9 +4025,7 @@ def_builtin_1 (enum built_in_function fn
   if (fntype == error_mark_node)
     return;
 
-  gcc_assert ((!both_p && !fallback_p)
-	      || !strncmp (name, "__builtin_",
-			   strlen ("__builtin_")));
+  gcc_assert ((!both_p && !fallback_p) || builtin_name_p (name));
 
   libname = name + strlen ("__builtin_");
   decl = add_builtin_function (name, fntype, fncode, fnclass,
--- gcc/c-common.h	(revision 132159)
+++ gcc/c-common.h	(local)
@@ -649,6 +649,7 @@ extern const struct attribute_spec c_com
 
 extern tree (*make_fname_decl) (tree, int);
 
+extern bool builtin_name_p (const char *);
 extern tree identifier_global_value (tree);
 extern void record_builtin_type (enum rid, const char *, tree);
 extern tree build_void_list_node (void);
--- gcc/c-decl.c	(revision 132159)
+++ gcc/c-decl.c	(local)
@@ -4028,9 +4028,14 @@ grokdeclarator (const struct c_declarato
       name = "type name";
   }
 
+  if (name && builtin_name_p (name))
+    {
+      error ("%qs is a reserved name", name);
+      return 0;
+    }
+
   /* A function definition's declarator must have the form of
      a function declarator.  */
-
   if (funcdef_flag && !funcdef_syntax)
     return 0;
 
@@ -5449,8 +5454,13 @@ grokfield (struct c_declarator *declarat
 			  width ? &width : NULL, decl_attrs,
 			  DEPRECATED_NORMAL);
 
-  finish_decl (value, NULL_TREE, NULL_TREE);
-  DECL_INITIAL (value) = width;
+  if (value == 0)
+    value = build_decl (FIELD_DECL, NULL_TREE, error_mark_node);
+  else
+    {
+      finish_decl (value, NULL_TREE, NULL_TREE);
+      DECL_INITIAL (value) = width;
+    }
 
   return value;
 }
--- gcc/cp/decl.c	(revision 132159)
+++ gcc/cp/decl.c	(local)
@@ -7562,6 +7562,12 @@ grokdeclarator (const cp_declarator *dec
 	break;
     }
 
+  if (name && builtin_name_p (name))
+    {
+      error ("%qs is a reserved name", name);
+      return error_mark_node;
+    }
+
   /* [dcl.fct.edf]
 
      The declarator in a function-definition shall have the form
--- gcc/testsuite/g++.old-deja/g++.law/operators24.C	(revision 132159)
+++ gcc/testsuite/g++.old-deja/g++.law/operators24.C	(local)
@@ -1,14 +0,0 @@
-// { dg-do assemble  }
-// GROUPS passed operators
-// opr-new file
-// From: rwave!myersn%rwave.roguewave@cs.orst.edu (Nathan Myers)
-// Date:     Wed, 16 Dec 92 11:55 PST
-// Subject:  2.3.2: friend decl breaks member op new
-// Message-ID: <m0n24qP-0000GmC@rwave.roguewave.com>
-
-#include <stddef.h>
-struct Link {
-  void* operator new(size_t, int);
-  friend void* __builtin_new(size_t);  // This declaration triggers the bug
-};
-void f() { new(2) Link; }
--- gcc/testsuite/gcc.dg/torture/builtin-convert-3.c	(revision 132159)
+++ gcc/testsuite/gcc.dg/torture/builtin-convert-3.c	(local)
@@ -20,7 +20,6 @@
    argument is a narrower type (perhaps implicitly) cast to a wider
    one.  */
 #define INNER_CAST1(MATHFN, RET) \
- PROTOTYPE1_RET (MATHFN, RET); \
  extern void link_failure_inner_##MATHFN##l_##MATHFN(void); \
  extern void link_failure_inner_##MATHFN##l_##MATHFN##f(void); \
  extern void link_failure_inner_##MATHFN##_##MATHFN##f(void); \
@@ -37,6 +36,10 @@
      && MATHFN(f1) != MATHFN##f(f1)) \
     link_failure_inner_##MATHFN##_##MATHFN##f()
 
+#define INNER_CAST2(MATHFN, RET) \
+ PROTOTYPE1_RET (MATHFN, RET) \
+ INNER_CAST1 (MATHFN, RET)
+
 void __attribute__ ((__noinline__)) test (double d1, float f1)
 {
 #ifdef __OPTIMIZE__
@@ -46,10 +49,10 @@ void __attribute__ ((__noinline__)) test
   INNER_CAST1 (__builtin_llceil, long long);
   INNER_CAST1 (__builtin_lfloor, long);
   INNER_CAST1 (__builtin_llfloor, long long);
-  INNER_CAST1 (lround, long);
-  INNER_CAST1 (llround, long long);
-  INNER_CAST1 (lrint, long);
-  INNER_CAST1 (llrint, long long);
+  INNER_CAST2 (lround, long);
+  INNER_CAST2 (llround, long long);
+  INNER_CAST2 (lrint, long);
+  INNER_CAST2 (llrint, long long);
 #endif /* HAVE_C99_RUNTIME */
 #endif /* __OPTIMIZE__ */
 }
--- gcc/testsuite/gcc.dg/torture/builtin-nonneg-1.c	(revision 132159)
+++ gcc/testsuite/gcc.dg/torture/builtin-nonneg-1.c	(local)
@@ -55,6 +55,11 @@ void test(double d1, double d2, float f1
  if (signbit(FN(d1)) || signbitf(FN##f(f1)) || signbitl(FN##l(ld1))) \
    link_failure_##FN()
 
+#define _ITEST1(FN) \
+ extern void link_failure_##FN (void); \
+ if (signbit(FN(d1)) || signbitf(FN##l(f1)) || signbitl(FN##ll(ld1))) \
+   link_failure_##FN()
+
 #define ITEST1(FN) \
  extern void link_failure_##FN (void); IPROTOTYPE(FN) \
  if (signbit(FN(d1)) || signbitf(FN##l(f1)) || signbitl(FN##ll(ld1))) \
@@ -74,8 +79,8 @@ void test(double d1, double d2, float f1
   TEST1 (pow10);
   TEST1 (sqrt);
   ITEST1 (ffs);
-  ITEST1 (__builtin_parity);
-  ITEST1 (__builtin_popcount);
+  _ITEST1 (__builtin_parity);
+  _ITEST1 (__builtin_popcount);
 
   /* These are nonnegative if the first argument is.  */
 #define ARG1TEST1(FN) \
@@ -85,6 +90,12 @@ void test(double d1, double d2, float f1
    link_failure_##FN()
 
   /* Same, but allow specifying the return type.  */
+#define _ARG1TEST1_RTYPE(FN,RTYPE) \
+ extern void link_failure_##FN (void); \
+ if (signbit(FN(fabs(d1))) || signbitf(FN##f(fabsf(f1))) \
+     || signbitl(FN##l(fabsl(ld1)))) \
+   link_failure_##FN()
+
 #define ARG1TEST1_RTYPE(FN,RTYPE) \
  extern void link_failure_##FN (void); PROTOTYPE_RTYPE(FN,RTYPE) \
  if (signbit(FN(fabs(d1))) || signbitf(FN##f(fabsf(f1))) \
@@ -144,12 +155,12 @@ void test(double d1, double d2, float f1
   ARG1TEST1 (floor);
   ARG1TEST2 (fmod);
   ARG1TEST2_A2INT (ldexp, int);
-  ARG1TEST1_RTYPE (__builtin_llceil, long long);
-  ARG1TEST1_RTYPE (__builtin_llfloor, long long);
+  _ARG1TEST1_RTYPE (__builtin_llceil, long long);
+  _ARG1TEST1_RTYPE (__builtin_llfloor, long long);
   ARG1TEST1_RTYPE (llrint, long long);
   ARG1TEST1_RTYPE (llround, long long);
-  ARG1TEST1_RTYPE (__builtin_lceil, long);
-  ARG1TEST1_RTYPE (__builtin_lfloor, long);
+  _ARG1TEST1_RTYPE (__builtin_lceil, long);
+  _ARG1TEST1_RTYPE (__builtin_lfloor, long);
   ARG1TEST1_RTYPE (lrint, long);
   ARG1TEST1_RTYPE (lround, long);
   /* The modf* functions aren't ever "const" or "pure" even with
--- gcc/testsuite/gcc.dg/pr32455-1.c	(revision 132159)
+++ gcc/testsuite/gcc.dg/pr32455-1.c	(local)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+
+typedef double* va_list;
+
+void __builtin_va_start(va_list, ...);		/* { dg-error "reserved" } */
+
+void foo(const char* s, ...)
+{
+  va_list v;
+  __builtin_va_start(v, s);			/* { dg-error "" } */
+}
--- gcc/testsuite/gcc.dg/pr32455-2.c	(revision 132159)
+++ gcc/testsuite/gcc.dg/pr32455-2.c	(local)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+
+struct __builtin_struct
+{
+  int x;
+  int __builtin_field;				/* { dg-error "reserved" } */
+};
+
+int __builtin_var = 1;				/* { dg-error "reserved" } */
+
+extern void __builtin_func_decl (void);		/* { dg-error "reserved" } */
+
+/* ??? We get multiple errors here, cause we forget it's supposed
+   to be a function type we're declaring...  */
+void __builtin_func_defn (void)			/* { dg-error "" } */
+{
+}

Re: [c/c++ (rfa)] fix 32455

[Richard Guenther]

On Feb 6, 2008 7:03 PM, Richard Henderson <rth@redhat.com> wrote:
> In fixing 32455, I agree with Richard Guenther that probably
> the best way to fix the bug is simply to prevent users from
> re-declaring __builtin_ functions with different signatures.
> Indeed, the easiest way appears to be to prevent them from
> doing pretty much anything with the __builtin_ namespace.
>
> This is an RFA for the C++ side, because while the testcase
> I have appears to work, I've no idea what other kind of side
> effects may be involved on that side.

I think this is a sensible solution.  But we should probably add
documentation and say these identifiers are reserved.

Also,

> --- gcc/testsuite/gcc.dg/pr32455-2.c    (revision 132159)
> +++ gcc/testsuite/gcc.dg/pr32455-2.c    (local)
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +
> +struct __builtin_struct
> +{
> +  int x;
> +  int __builtin_field;                         /* { dg-error "reserved" } */
> +};

We reject __builtin_field, but not __builtin_struct?  What happens
if you re-declare the __builtin_va_list type?

Thanks,
Richard.

Re: [c/c++ (rfa)] fix 32455

[Richard Henderson]

On Wed, Feb 06, 2008 at 09:24:14PM +0100, Richard Guenther wrote:
> We reject __builtin_field, but not __builtin_struct?  What happens
> if you re-declare the __builtin_va_list type?

Err, probably bad things.  I'll see about fixing that too.


r~

Re: [c/c++ (rfa)] fix 32455

[Mark Mitchell]

Richard Henderson wrote:
> On Wed, Feb 06, 2008 at 09:24:14PM +0100, Richard Guenther wrote:
>> We reject __builtin_field, but not __builtin_struct?  What happens
>> if you re-declare the __builtin_va_list type?
> 
> Err, probably bad things.  I'll see about fixing that too.

I think it's a good idea to completely forbid declarations of __builtin 
things and I think the C++ parts of the patch are fine.  However, I do 
expect we'll see some fallout from people who have header files that do 
declare __builtins.  Perhaps for 4.3 we should make this an 
unconditional warning, and indicate that in 4.4 it will go away completely?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [c/c++ (rfa)] fix 32455

[Jakub Jelinek]

On Sun, Feb 10, 2008 at 11:31:01AM -0800, Mark Mitchell wrote:
> Richard Henderson wrote:
> >On Wed, Feb 06, 2008 at 09:24:14PM +0100, Richard Guenther wrote:
> >>We reject __builtin_field, but not __builtin_struct?  What happens
> >>if you re-declare the __builtin_va_list type?
> >
> >Err, probably bad things.  I'll see about fixing that too.
> 
> I think it's a good idea to completely forbid declarations of __builtin 
> things and I think the C++ parts of the patch are fine.  However, I do 
> expect we'll see some fallout from people who have header files that do 
> declare __builtins.  Perhaps for 4.3 we should make this an 
> unconditional warning, and indicate that in 4.4 it will go away completely?

glibc internal headers declare some __builtin_* functions in order to
add some __asm ("name") to them to override what symbol is used for them
if not optimized out.  I think we should only error out if declaring
__builtin_* types or if explicit __builtin_* function declaration has
non-matching types.

	Jakub

Re: [c/c++ (rfa)] fix 32455

[Mark Mitchell]

Jakub Jelinek wrote:
> On Sun, Feb 10, 2008 at 11:31:01AM -0800, Mark Mitchell wrote:
>> Richard Henderson wrote:
>>> On Wed, Feb 06, 2008 at 09:24:14PM +0100, Richard Guenther wrote:
>>>> We reject __builtin_field, but not __builtin_struct?  What happens
>>>> if you re-declare the __builtin_va_list type?
>>> Err, probably bad things.  I'll see about fixing that too.
>> I think it's a good idea to completely forbid declarations of __builtin 
>> things and I think the C++ parts of the patch are fine.  However, I do 
>> expect we'll see some fallout from people who have header files that do 
>> declare __builtins.  Perhaps for 4.3 we should make this an 
>> unconditional warning, and indicate that in 4.4 it will go away completely?
> 
> glibc internal headers declare some __builtin_* functions in order to
> add some __asm ("name") to them to override what symbol is used for them
> if not optimized out.  I think we should only error out if declaring
> __builtin_* types or if explicit __builtin_* function declaration has
> non-matching types.

Do we think that ought to be an acceptable thing for GLIBC to do?

I guess I don't think it is.  GLIBC is just a library, albeit a very 
important one.  These are the compiler's magic functions and it's free 
to do what it likes with them.  Your suggestion also won't future-proof 
people against us adding new builtins which is one of the risks.  For 
example, I've seen people write __builtin_xyz functions as ordinary 
functions to try to mimic what we do for assembly intrinsics.  Then, if 
we add that as a real intrinsic in the future, there will be a conflict.

I'd prefer that we just declare the __builtin namespace off-limits for 
user declarations.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713