[C++ PATCH] Don't clear DECL_BUILT_IN_CLASS/DECL_FUNCTION_CODE when redeclaring a builtin if types match

[C++ PATCH] Don't clear DECL_BUILT_IN_CLASS/DECL_FUNCTION_CODE when redeclaring a builtin if types match

[Jakub Jelinek]

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

Hi!

The C FE always keeps DECL_BUILT_IN_CLASS when types match, even when
newdecl defines a function.
      if (DECL_BUILT_IN (olddecl))
        {
          /* If redeclaring a builtin function, it stays built in.
             But it gets tagged as having been declared.  */
          DECL_BUILT_IN_CLASS (newdecl) = DECL_BUILT_IN_CLASS (olddecl);
          DECL_FUNCTION_CODE (newdecl) = DECL_FUNCTION_CODE (olddecl);
          C_DECL_DECLARED_BUILTIN (newdecl) = 1;
          if (new_is_prototype)
            C_DECL_BUILTIN_PROTOTYPE (newdecl) = 0;
          else
            C_DECL_BUILTIN_PROTOTYPE (newdecl)
              = C_DECL_BUILTIN_PROTOTYPE (olddecl);
        }
But the C++ FE doesn't do this:
      if (new_defines_function)
	...
      else if (types_match)
	{
          /* If redeclaring a builtin function, and not a definition,
             it stays built in.  */
          if (DECL_BUILT_IN (olddecl))
            {
              DECL_BUILT_IN_CLASS (newdecl) = DECL_BUILT_IN_CLASS (olddecl);
              DECL_FUNCTION_CODE (newdecl) = DECL_FUNCTION_CODE (olddecl);
              /* If we're keeping the built-in definition, keep the rtl,
                 regardless of declaration matches.  */
              COPY_DECL_RTL (olddecl, newdecl);
            }
	...

This causes quite undesirable behavior e.g. on glibc headers on ppc with
-mlong-double-64 (as shown on the testcase).  Attached are two different
patches, one does what the C FE does and copies DECL_BUILT_IN_CLASS
etc. whenever types_match (bootstrapped/regtested on x86_64-linux).
IMHO when you define memcpy in current CU (or some other builtins), there
is no reason why any calls to that builtin from within other functions in
the CU shouldn't be suddenly optimized as builtins.

The second alternative patch only does that for gnu_inline wrappers
- especially if the function is just extern inline
__attribute__((gnu_inline)) without always_inline attribute, it might
not be inlined in some places.  Still they should be treated as builtin
calls.  I very much prefer the first patch though.

Ok for trunk?

	Jakub

[-- Attachment #2: E2 --]
[-- Type: text/plain, Size: 3658 bytes --]

2007-10-02  Jakub Jelinek  <jakub@redhat.com>

	* decl.c (duplicate_decls): When redeclaring a builtin function,
	keep the merged decl builtin whenever types match, even if new
	decl defines a function.

	* gcc.dg/builtins-65.c: New test.
	* g++.dg/ext/builtin10.C: New test.

--- gcc/cp/decl.c.jj	2007-10-01 22:11:09.000000000 +0200
+++ gcc/cp/decl.c	2007-10-02 11:39:46.000000000 +0200
@@ -1988,23 +1988,21 @@ duplicate_decls (tree newdecl, tree oldd
 	  DECL_ARGUMENTS (olddecl) = DECL_ARGUMENTS (newdecl);
 	  DECL_RESULT (olddecl) = DECL_RESULT (newdecl);
 	}
+      /* If redeclaring a builtin function, it stays built in.  */
+      if (types_match && DECL_BUILT_IN (olddecl))
+	{
+	  DECL_BUILT_IN_CLASS (newdecl) = DECL_BUILT_IN_CLASS (olddecl);
+	  DECL_FUNCTION_CODE (newdecl) = DECL_FUNCTION_CODE (olddecl);
+	  /* If we're keeping the built-in definition, keep the rtl,
+	     regardless of declaration matches.  */
+	  COPY_DECL_RTL (olddecl, newdecl);
+	}
       if (new_defines_function)
 	/* If defining a function declared with other language
 	   linkage, use the previously declared language linkage.  */
 	SET_DECL_LANGUAGE (newdecl, DECL_LANGUAGE (olddecl));
       else if (types_match)
 	{
-	  /* If redeclaring a builtin function, and not a definition,
-	     it stays built in.  */
-	  if (DECL_BUILT_IN (olddecl))
-	    {
-	      DECL_BUILT_IN_CLASS (newdecl) = DECL_BUILT_IN_CLASS (olddecl);
-	      DECL_FUNCTION_CODE (newdecl) = DECL_FUNCTION_CODE (olddecl);
-	      /* If we're keeping the built-in definition, keep the rtl,
-		 regardless of declaration matches.  */
-	      COPY_DECL_RTL (olddecl, newdecl);
-	    }
-
 	  DECL_RESULT (newdecl) = DECL_RESULT (olddecl);
 	  /* Don't clear out the arguments if we're redefining a function.  */
 	  if (DECL_ARGUMENTS (olddecl))
--- gcc/testsuite/gcc.dg/builtins-65.c.jj	2007-10-02 11:23:51.000000000 +0200
+++ gcc/testsuite/gcc.dg/builtins-65.c	2007-10-02 11:24:12.000000000 +0200
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+typedef __SIZE_TYPE__ size_t;
+extern void __chk_fail (void);
+extern int snprintf (char *, size_t, const char *, ...);
+extern inline __attribute__((gnu_inline, always_inline)) int snprintf (char *a, size_t b, const char *fmt, ...)
+{
+  if (__builtin_object_size (a, 0) != -1UL && __builtin_object_size (a, 0) < b)
+    __chk_fail ();
+  return __builtin_snprintf (a, b, fmt, __builtin_va_arg_pack ());
+}
+extern int snprintf (char *, size_t, const char *, ...) __asm ("mysnprintf");
+
+char buf[10];
+
+int
+main (void)
+{
+  snprintf (buf, 10, "%d%d\n", 10, 10);
+  return 0;
+}
+
+/* { dg-final { scan-assembler "mysnprintf" } } */
+/* { dg-final { scan-assembler-not "__chk_fail" } } */
--- gcc/testsuite/g++.dg/ext/builtin10.C.jj	2007-10-02 11:19:45.000000000 +0200
+++ gcc/testsuite/g++.dg/ext/builtin10.C	2007-10-02 11:23:26.000000000 +0200
@@ -0,0 +1,27 @@
+// { dg-do compile }
+// { dg-options "-O2" }
+
+typedef __SIZE_TYPE__ size_t;
+extern "C" {
+extern void __chk_fail (void);
+extern int snprintf (char *, size_t, const char *, ...);
+extern inline __attribute__((gnu_inline, always_inline)) int snprintf (char *a, size_t b, const char *fmt, ...)
+{
+  if (__builtin_object_size (a, 0) != -1UL && __builtin_object_size (a, 0) < b)
+    __chk_fail ();
+  return __builtin_snprintf (a, b, fmt, __builtin_va_arg_pack ());
+}
+extern int snprintf (char *, size_t, const char *, ...) __asm ("mysnprintf");
+}
+
+char buf[10];
+
+int
+main (void)
+{
+  snprintf (buf, 10, "%d%d\n", 10, 10);
+  return 0;
+}
+
+// { dg-final { scan-assembler "mysnprintf" } }
+// { dg-final { scan-assembler-not "__chk_fail" } }

[-- Attachment #3: E --]
[-- Type: text/plain, Size: 3337 bytes --]

2007-10-02  Jakub Jelinek  <jakub@redhat.com>

	* decl.c (duplicate_decls): When redeclaring a builtin function,
	keep the merged decl builtin also when new decl defines a gnu_inline
	function and types match.

	* gcc.dg/builtins-65.c: New test.
	* g++.dg/ext/builtin10.C: New test.

--- gcc/cp/decl.c.jj	2007-10-01 22:11:09.000000000 +0200
+++ gcc/cp/decl.c	2007-10-02 11:16:40.000000000 +0200
@@ -1989,9 +1989,21 @@ duplicate_decls (tree newdecl, tree oldd
 	  DECL_RESULT (olddecl) = DECL_RESULT (newdecl);
 	}
       if (new_defines_function)
-	/* If defining a function declared with other language
-	   linkage, use the previously declared language linkage.  */
-	SET_DECL_LANGUAGE (newdecl, DECL_LANGUAGE (olddecl));
+	{
+	  /* If defining a function declared with other language
+	     linkage, use the previously declared language linkage.  */
+	  SET_DECL_LANGUAGE (newdecl, DECL_LANGUAGE (olddecl));
+	  /* If redeclaring a builtin function as gnu_inline wrapper,
+	     it stays built in.  */
+	  if (DECL_BUILT_IN (olddecl) && types_match && GNU_INLINE_P (newdecl))
+	    {
+	      DECL_BUILT_IN_CLASS (newdecl) = DECL_BUILT_IN_CLASS (olddecl);
+	      DECL_FUNCTION_CODE (newdecl) = DECL_FUNCTION_CODE (olddecl);
+	      /* If we're keeping the built-in definition, keep the rtl,
+		 regardless of declaration matches.  */
+	      COPY_DECL_RTL (olddecl, newdecl);
+	    }
+	}
       else if (types_match)
 	{
 	  /* If redeclaring a builtin function, and not a definition,
--- gcc/testsuite/gcc.dg/builtins-65.c.jj	2007-10-02 11:23:51.000000000 +0200
+++ gcc/testsuite/gcc.dg/builtins-65.c	2007-10-02 11:24:12.000000000 +0200
@@ -0,0 +1,25 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+typedef __SIZE_TYPE__ size_t;
+extern void __chk_fail (void);
+extern int snprintf (char *, size_t, const char *, ...);
+extern inline __attribute__((gnu_inline, always_inline)) int snprintf (char *a, size_t b, const char *fmt, ...)
+{
+  if (__builtin_object_size (a, 0) != -1UL && __builtin_object_size (a, 0) < b)
+    __chk_fail ();
+  return __builtin_snprintf (a, b, fmt, __builtin_va_arg_pack ());
+}
+extern int snprintf (char *, size_t, const char *, ...) __asm ("mysnprintf");
+
+char buf[10];
+
+int
+main (void)
+{
+  snprintf (buf, 10, "%d%d\n", 10, 10);
+  return 0;
+}
+
+/* { dg-final { scan-assembler "mysnprintf" } } */
+/* { dg-final { scan-assembler-not "__chk_fail" } } */
--- gcc/testsuite/g++.dg/ext/builtin10.C.jj	2007-10-02 11:19:45.000000000 +0200
+++ gcc/testsuite/g++.dg/ext/builtin10.C	2007-10-02 11:23:26.000000000 +0200
@@ -0,0 +1,27 @@
+// { dg-do compile }
+// { dg-options "-O2" }
+
+typedef __SIZE_TYPE__ size_t;
+extern "C" {
+extern void __chk_fail (void);
+extern int snprintf (char *, size_t, const char *, ...);
+extern inline __attribute__((gnu_inline, always_inline)) int snprintf (char *a, size_t b, const char *fmt, ...)
+{
+  if (__builtin_object_size (a, 0) != -1UL && __builtin_object_size (a, 0) < b)
+    __chk_fail ();
+  return __builtin_snprintf (a, b, fmt, __builtin_va_arg_pack ());
+}
+extern int snprintf (char *, size_t, const char *, ...) __asm ("mysnprintf");
+}
+
+char buf[10];
+
+int
+main (void)
+{
+  snprintf (buf, 10, "%d%d\n", 10, 10);
+  return 0;
+}
+
+// { dg-final { scan-assembler "mysnprintf" } }
+// { dg-final { scan-assembler-not "__chk_fail" } }

Re: [C++ PATCH] Don't clear DECL_BUILT_IN_CLASS/DECL_FUNCTION_CODE when redeclaring a builtin if types match

[Mark Mitchell]

Jakub Jelinek wrote:

> The C FE always keeps DECL_BUILT_IN_CLASS when types match, even when
> newdecl defines a function.

> But the C++ FE doesn't do this:
>       if (new_defines_function)

The standard does prohibit redefining these functions.  But, in
practice, people would be surprised if they did this:

/* Nobody should use memcpy, I think it's evil!  */
extern "C" void memcpy (void *, void *, size_t) { abort (); }

void f() {
  memcpy (...);
}

and the program did not abort because the call to memcpy was inlined.
Or, for a more practical example, if the implementation of memcpy had
additional auditing code -- as the GLIBC headers are trying to do.  So,
I think the C++ front-end behavior is reasonable: if you define memcpy,
we forget what we think we know about memcpy, and just treat it as we
would any other function.

At least in the example you posted, it looks like the GLIBC code is
checking for something that is known at compile-time: whether the
arguments to snprintf are inherently wrong.  Why can't we handle that
with a compiler warning/error so that this check benefits all users on
all operating systems, regardless of C library?  You can't use that
approach for functions that GCC doesn't know about -- but, then again,
you won't have the performance problem that you're concerned about in
that case either.

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

Re: [C++ PATCH] Don't clear DECL_BUILT_IN_CLASS/DECL_FUNCTION_CODE when redeclaring a builtin if types match

[Jakub Jelinek]

On Sun, Oct 07, 2007 at 10:43:06AM -0700, Mark Mitchell wrote:
> Jakub Jelinek wrote:
> 
> > The C FE always keeps DECL_BUILT_IN_CLASS when types match, even when
> > newdecl defines a function.
> 
> > But the C++ FE doesn't do this:
> >       if (new_defines_function)
> 
> The standard does prohibit redefining these functions.  But, in
> practice, people would be surprised if they did this:
> 
> /* Nobody should use memcpy, I think it's evil!  */
> extern "C" void memcpy (void *, void *, size_t) { abort (); }
> 
> void f() {
>   memcpy (...);
> }

If you do this, then you really want to use -fno-builtin-memcpy.
Clearing DECL_BUILT_IN_CLASS in duplicate_decls will just mean
that the behavior will be different in CUs where you define this
non-conforming memcpy and other CUs linked into your program.

> and the program did not abort because the call to memcpy was inlined.
> Or, for a more practical example, if the implementation of memcpy had
> additional auditing code -- as the GLIBC headers are trying to do.  So,
> I think the C++ front-end behavior is reasonable: if you define memcpy,
> we forget what we think we know about memcpy, and just treat it as we
> would any other function.

To me the C front-end behavior does make much more sense.  That helps
you to optimize conforming programs, even when you define the standard
in the same CU.  If you define memcpy and don't compile with
-fno-builtin-memcpy, the right assumption is that your definition is
standard conforming.  If you have auditing code you want to do always
whenever that fn is called, just make the function always_inline.

> At least in the example you posted, it looks like the GLIBC code is
> checking for something that is known at compile-time: whether the
> arguments to snprintf are inherently wrong.  Why can't we handle that
> with a compiler warning/error so that this check benefits all users on
> all operating systems, regardless of C library?

But we don't want that checking code turned on always, only when the user
asks for it using a special macro.  Furthermore, we need to know that the
particular version of the C library supports the checking function, how
exactly it needs to be checked (which is changing over time) and how other
details are handled (e.g. DFmode vs. TFmode long double related functions
on ppc* etc.).  All this is terribly specific to the exact C library used
(or to libssp if you are using that instead).  The current behavior is
that by calling a __*_chk builtin you are telling GCC that 1) you want to
do the checking 2) the C library supports it 3) among arguments you tell
the user's choice of whether %n should be recognized in writable strings
or not and in the future perhaps other details as well.  Without the
inline wrappers we are talking about here all this would get hardcoded
into GCC, which would terribly tie glibc's hands and would mean users would
need to duplicate the options to request the checking (say
-D_FORTIFY_SOURCE=2 would need to be accompanied by -ffortify-source=2).

>  You can't use that
> approach for functions that GCC doesn't know about -- but, then again,
> you won't have the performance problem that you're concerned about in
> that case either.

As I said in the mail, I'm ATM primarily not concerned about performance
- glibc always uses always_inline gnu_inline functions for this stuff - but
that adding an __asm ("...") redirect to the function will no longer result
in the needed behavior - it will not have any effect on the __builtin_*
functions.

	Jakub

Re: [C++ PATCH] Don't clear DECL_BUILT_IN_CLASS/DECL_FUNCTION_CODE when redeclaring a builtin if types match

[Mark Mitchell]

Jakub Jelinek wrote:

>>> The C FE always keeps DECL_BUILT_IN_CLASS when types match, even when
>>> newdecl defines a function.
>>> But the C++ FE doesn't do this:
>>>       if (new_defines_function)
>> The standard does prohibit redefining these functions.  But, in
>> practice, people would be surprised if they did this:
>>
>> /* Nobody should use memcpy, I think it's evil!  */
>> extern "C" void memcpy (void *, void *, size_t) { abort (); }
>>
>> void f() {
>>   memcpy (...);
>> }
> 
> If you do this, then you really want to use -fno-builtin-memcpy.
> Clearing DECL_BUILT_IN_CLASS in duplicate_decls will just mean
> that the behavior will be different in CUs where you define this
> non-conforming memcpy and other CUs linked into your program.

Yes, you may want to use -fno-builtin-memcpy in other translation units.
 But, it certainly seems likely to lead to user confusion if overriding
a standard library function (let's not call these "builtins" --
__builtin_memcpy is a "builtin", but "memcpy" is a standard library
function) is ignored by the compiler.

One of the things I'm concerned about is ability for users to bring code
to GCC from other compilers.  I'm certain that code exists that
overrides definitions of standard library functions.  Your suggestion
would make the compiler ignore the plain meaning of the user's code and
I think that's a bad thing.

> As I said in the mail, I'm ATM primarily not concerned about performance
> - glibc always uses always_inline gnu_inline functions for this stuff - but
> that adding an __asm ("...") redirect to the function will no longer result
> in the needed behavior - it will not have any effect on the __builtin_*
> functions.

We could argue about how GLIBC could best implement its checking code,
but that's GLIBC's business. :-)

I do agree that the call to __builtin_snprintf  in your builtin10.C
example should end up calling mysnprintf, if not expanded inline by the
compiler because the semantics of __builtin_x is that if it cannot
expand inline, then it calls the function whose *source* name is "x" --
and whose assembler name is whatever assembler name we would normally
associate with "x".  But, that doesn't mean that a user declaration of
"x" should be marked as builtin.  Instead, I think it means that the
code that expands builtins needs to be able to find the function with
the matching source name, and use that to find the corresponding
assembly name.  Perhaps when the user declares a replacement for a
standard library routine we need to update a table mapping builtins to
FUNCTION_DECLs.

I also agree that the C and C++ front-ends should be consistent in their
handling of this situation.  That's more important than whatever
semantic decision we end up making.  So, I think the C and C++
maintainers need to get together and see if we can get consensus on what
semantics we want.

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

Re: [C++ PATCH] Don't clear DECL_BUILT_IN_CLASS/DECL_FUNCTION_CODE when redeclaring a builtin if types match

[Jason Merrill]

I can see both sides to this, but I tend to agree with Jakub.  Code that 
really wants to override the builtin will need to use -fno-builtin in 
other translation units, so it might as well use it in the translation 
unit with the definition for consistency.  Yes, this is likely to 
confuse people who want to override builtins, but that's going to be 
tricky to get right in any case.

The other option would be to add an attribute so glibc can mark the 
inlines as "still builtin".

Jason