diff mbox

c/66516 - missing diagnostic on taking the address of a builtin function

Message ID 5587432A.9000602@redhat.com
State New
Headers show

Commit Message

Martin Sebor June 21, 2015, 11:05 p.m. UTC 
Attached is a patch to reject C and C++ constructs that result
in obtaining a pointer (or a reference in C++) to a builtin
function.  These constructs are currently silently accepted by
GCC and, in most cases(*), result in a linker error.  The patch
brings GCC on par with Clang by rejecting all such constructs.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Okay to commit to trunk?

Martin

[*] Besides rejecting constructs that lead to linker errors
the patch leads to rejecting also some benign constructs (that
are also rejected by Clang).  For example, the program below
currently compiles and links successfully with GCC 5.1 is
rejected with the patch applied.  That's intended (but I'm
open to arguments against it).

$ cat /build/tmp/u.cpp && /build/gcc-66516/gcc/xg++ 
-B/build/gcc-66516/gcc -Wall -c -std=c++11 /build/tmp/u.cpp
static constexpr int (&r)(int) = __builtin_ffs;

int main () { return r (0); }
/build/tmp/u.cpp:1:34: error: invalid initialization of a reference with 
a builtin function
  static constexpr int (&r)(int) = __builtin_ffs;
                                   ^

Comments

Joseph Myers June 22, 2015, 2:10 p.m. UTC | #1 
On Sun, 21 Jun 2015, Martin Sebor wrote:

> diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> index 636e0bb..637a292 100644
> --- a/gcc/c/c-typeck.c
> +++ b/gcc/c/c-typeck.c
> @@ -58,6 +58,8 @@ along with GCC; see the file COPYING3.  If not see
>  #include "cilk.h"
>  #include "gomp-constants.h"
> 
> +#include <stdlib.h>

Included from system.h, don't include it explicitly in source files.

> +      if (DECL_IS_BUILTIN (exp.value))
> +	{
> +	  error_at (loc, "converting builtin function to a pointer");

Say "built-in" (see codingconventions.html).
Marek Polacek June 22, 2015, 2:40 p.m. UTC | #2 
On Sun, Jun 21, 2015 at 05:05:14PM -0600, Martin Sebor wrote:
> Attached is a patch to reject C and C++ constructs that result
> in obtaining a pointer (or a reference in C++) to a builtin
> function.  These constructs are currently silently accepted by
> GCC and, in most cases(*), result in a linker error.  The patch
> brings GCC on par with Clang by rejecting all such constructs.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
 
It seems like this patch regresess pr59630.c testcase; I don't see
the testcase being addressed in this patch.

> 2015-06-21  Martin Sebor  <msebor@redhat.com>
> 
> 	PR c/66516
> 	* c/c-typeck.c (default_function_array_conversion): Reject
> 	converting a builtin function to a pointer.
> 	(parser_build_unary_op): Reject taking the address of a builtin
> 	function.
> 	* cp/call.c (convert_like_real): Reject converting a builtin function
> 	to a pointer.
> 	(initialize_reference): Reject initializing a reference with a builtin
> 	function.
> 	* cp/typeck.c (cp_build_addr_expr_strict): Reject taking the address
> 	of a builtin function.
> 	(build_reinterpret_cast_1): Reject casting a builtin function to
> 	a pointer.
> 	(convert_for_initialization): Reject initializing a pointer with
> 	the a builtin function.
 
Please no c/ and cp/ prefixes.

> +#include <stdlib.h>

As Joseph already pointed out, this is redundant.

> @@ -3384,7 +3392,14 @@ parser_build_unary_op (location_t loc, enum tree_code code, struct c_expr arg)
>    result.original_code = code;
>    result.original_type = NULL;
> 
> -  if (TREE_OVERFLOW_P (result.value) && !TREE_OVERFLOW_P (arg.value))
> +  if (code == ADDR_EXPR
> +      && TREE_CODE (TREE_TYPE (arg.value)) == FUNCTION_TYPE
> +      && DECL_IS_BUILTIN (arg.value))
> +    {
> +      error_at (loc, "taking address of a builtin function");
> +      result.value = error_mark_node;
> +    }
> +  else if (TREE_OVERFLOW_P (result.value) && !TREE_OVERFLOW_P (arg.value))
>      overflow_warning (loc, result.value);

It seems like you can move the new hunk a bit above so that we don't call
build_unary_op in a case when taking the address of a built-in function.

Unfortunately, it doesn't seem possible to do this error in build_unary_op
or in function_to_pointer_conversion :(.

	Marek
Marek Polacek June 22, 2015, 7:10 p.m. UTC | #3 
On Sun, Jun 21, 2015 at 05:05:14PM -0600, Martin Sebor wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/addr_builtin-pr66516.c
> @@ -0,0 +1,59 @@
> +/* { dg-do compile } */

One more nit: I think I'd prefer naming the test addr-builtin-1.c
and then putting /* PR c/66516 */ on the first line of the test.

	Marek
Martin Sebor June 23, 2015, 1:59 a.m. UTC | #4 
> It seems like this patch regresess pr59630.c testcase; I don't see
> the testcase being addressed in this patch.

Thanks for the review and for pointing out this regression!
I missed it among all the C test suite failures (I see 157
of them in 24 distinct tests on x86_64.)

pr59630 is marked ice-on-valid-code even though the call via
the converted pointer is clearly invalid (UB). What's more
relevant, though, is that the test case is one of those that
(while they both compile and link with the unpatched GCC) are
not intended to compile with the patch (and don't compile with
Clang).

In this simple case, the call to __builtin_abs(0) is folded
into the constant 0, but in more involved cases GCC emits
a call to abs. It's not clear to me from the manual or from
the builtin tests I've seen whether this is by design or
an accident of the implementation

Is it intended that programs be able to take the address of
the builtins that correspond to libc functions and make calls
to the underlying libc functions via such pointers? (If so,
the patch will need some tweaking.)

>
> Please no c/ and cp/ prefixes.

Sure, let me fix that in the next patch once the question
above has been settled.

>
>> +#include <stdlib.h>
>
> As Joseph already pointed out, this is redundant.

Yes, that was an accidental vestige of some debugging code I had
added. I'll take it out.

>
>> @@ -3384,7 +3392,14 @@ parser_build_unary_op (location_t loc, enum tree_code code, struct c_expr arg)
>>     result.original_code = code;
>>     result.original_type = NULL;
>>
>> -  if (TREE_OVERFLOW_P (result.value) && !TREE_OVERFLOW_P (arg.value))
>> +  if (code == ADDR_EXPR
>> +      && TREE_CODE (TREE_TYPE (arg.value)) == FUNCTION_TYPE
>> +      && DECL_IS_BUILTIN (arg.value))
>> +    {
>> +      error_at (loc, "taking address of a builtin function");
>> +      result.value = error_mark_node;
>> +    }
>> +  else if (TREE_OVERFLOW_P (result.value) && !TREE_OVERFLOW_P (arg.value))
>>       overflow_warning (loc, result.value);
>
> It seems like you can move the new hunk a bit above so that we don't call
> build_unary_op in a case when taking the address of a built-in function.

Yes, that should work.

>
> Unfortunately, it doesn't seem possible to do this error in build_unary_op
> or in function_to_pointer_conversion :(.

Right. I couldn't find a way to do it because it gets called
for function calls too.

> One more nit: I think I'd prefer naming the test addr-builtin-1.c
> and then putting /* PR c/66516 */ on the first line of the test.

Will do.

Martin
Marek Polacek June 23, 2015, 10:18 a.m. UTC | #5 
On Mon, Jun 22, 2015 at 07:59:20PM -0600, Martin Sebor wrote:
> >It seems like this patch regresess pr59630.c testcase; I don't see
> >the testcase being addressed in this patch.
> 
> Thanks for the review and for pointing out this regression!
> I missed it among all the C test suite failures (I see 157
> of them in 24 distinct tests on x86_64.)

You might want to use contrib/test_summary and then compare its
outputs.

> pr59630 is marked ice-on-valid-code even though the call via
> the converted pointer is clearly invalid (UB). What's more
> relevant, though, is that the test case is one of those that
> (while they both compile and link with the unpatched GCC) are
> not intended to compile with the patch (and don't compile with
> Clang).

Right, just turn dg-warning into dg-error.

> In this simple case, the call to __builtin_abs(0) is folded
> into the constant 0, but in more involved cases GCC emits
> a call to abs. It's not clear to me from the manual or from
> the builtin tests I've seen whether this is by design or
> an accident of the implementation
> 
> Is it intended that programs be able to take the address of
> the builtins that correspond to libc functions and make calls
> to the underlying libc functions via such pointers? (If so,
> the patch will need some tweaking.)

I don't think so, at least clang doesn't allow e.g.
size_t (*fp) (const char *) = __builtin_strlen;

	Marek
Jakub Jelinek June 23, 2015, 10:29 a.m. UTC | #6 
On Tue, Jun 23, 2015 at 12:18:30PM +0200, Marek Polacek wrote:
> > Is it intended that programs be able to take the address of
> > the builtins that correspond to libc functions and make calls
> > to the underlying libc functions via such pointers? (If so,
> > the patch will need some tweaking.)
> 
> I don't think so, at least clang doesn't allow e.g.
> size_t (*fp) (const char *) = __builtin_strlen;

Well, clang is irrelevant here, __builtin_strlen etc. is a GNU
extension, so it matters what we decide about it.  As this used to work
for decades (if the builtin function has a libc fallback), suddenly
rejecting it could break various programs that e.g. just
#define strlen __builtin_strlen
or similar.  Can't we really reject it just for the functions
that don't have a unique fallback?

	Jakub
Martin Sebor June 23, 2015, 3:11 p.m. UTC | #7 
On 06/23/2015 04:29 AM, Jakub Jelinek wrote:
> On Tue, Jun 23, 2015 at 12:18:30PM +0200, Marek Polacek wrote:
>>> Is it intended that programs be able to take the address of
>>> the builtins that correspond to libc functions and make calls
>>> to the underlying libc functions via such pointers? (If so,
>>> the patch will need some tweaking.)
>>
>> I don't think so, at least clang doesn't allow e.g.
>> size_t (*fp) (const char *) = __builtin_strlen;
>
> Well, clang is irrelevant here, __builtin_strlen etc. is a GNU
> extension, so it matters what we decide about it.  As this used to work
> for decades (if the builtin function has a libc fallback), suddenly
> rejecting it could break various programs that e.g. just
> #define strlen __builtin_strlen
> or similar.  Can't we really reject it just for the functions
> that don't have a unique fallback?

Let me look into it.

Martin
diff mbox

Patch

2015-06-21  Martin Sebor  <msebor@redhat.com>

	PR c/66516
	* c/c-typeck.c (default_function_array_conversion): Reject
	converting a builtin function to a pointer.
	(parser_build_unary_op): Reject taking the address of a builtin
	function.
	* cp/call.c (convert_like_real): Reject converting a builtin function
	to a pointer.
	(initialize_reference): Reject initializing a reference with a builtin
	function.
	* cp/typeck.c (cp_build_addr_expr_strict): Reject taking the address
	of a builtin function.
	(build_reinterpret_cast_1): Reject casting a builtin function to
	a pointer.
	(convert_for_initialization): Reject initializing a pointer with
	the a builtin function.

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 636e0bb..637a292 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -58,6 +58,8 @@  along with GCC; see the file COPYING3.  If not see
 #include "cilk.h"
 #include "gomp-constants.h"

+#include <stdlib.h>
+
 /* Possible cases of implicit bad conversions.  Used to select
    diagnostic messages in convert_for_assignment.  */
 enum impl_conv {
@@ -1940,6 +1942,12 @@  default_function_array_conversion (location_t loc, struct c_expr exp)
       }
       break;
     case FUNCTION_TYPE:
+      if (DECL_IS_BUILTIN (exp.value))
+	{
+	  error_at (loc, "converting builtin function to a pointer");
+	  exp.value = error_mark_node;
+	}
+      else
 	exp.value = function_to_pointer_conversion (loc, exp.value);
       break;
     default:
@@ -3384,7 +3392,14 @@  parser_build_unary_op (location_t loc, enum tree_code code, struct c_expr arg)
   result.original_code = code;
   result.original_type = NULL;

-  if (TREE_OVERFLOW_P (result.value) && !TREE_OVERFLOW_P (arg.value))
+  if (code == ADDR_EXPR
+      && TREE_CODE (TREE_TYPE (arg.value)) == FUNCTION_TYPE
+      && DECL_IS_BUILTIN (arg.value))
+    {
+      error_at (loc, "taking address of a builtin function");
+      result.value = error_mark_node;
+    }
+  else if (TREE_OVERFLOW_P (result.value) && !TREE_OVERFLOW_P (arg.value))
     overflow_warning (loc, result.value);

   return result;
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 2d90ed9..df4cc77 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -6570,6 +6570,13 @@  convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
 	    expr = build_target_expr_with_type (expr, type, complain);
 	  }

+	if (TREE_CODE (TREE_TYPE (expr)) == FUNCTION_TYPE
+	    && DECL_P (expr) && DECL_IS_BUILTIN (expr))
+	  {
+	    error_at (input_location, "taking address of a builtin function");
+	    return error_mark_node;
+	  }
+
 	/* Take the address of the thing to which we will bind the
 	   reference.  */
 	expr = cp_build_addr_expr (expr, complain);
@@ -9768,8 +9775,18 @@  initialize_reference (tree type, tree expr,
     }

   if (conv->kind == ck_ref_bind)
+    {
+      if (TREE_CODE (TREE_TYPE (expr)) == FUNCTION_TYPE
+	  && DECL_P (expr) && DECL_IS_BUILTIN (expr))
+	{
+	  error_at (input_location, "invalid initialization of a reference "
+		    "with a builtin function");
+	  return error_mark_node;
+	}
+
       /* Perform the conversion.  */
       expr = convert_like (conv, expr, complain);
+    }
   else if (conv->kind == ck_ambig)
     /* We gave an error in build_user_type_conversion_1.  */
     expr = error_mark_node;
diff --git a/gcc/cp/typeck.c b/gcc/cp/typeck.c
index 5b09b73..fbea052 100644
--- a/gcc/cp/typeck.c
+++ b/gcc/cp/typeck.c
@@ -5621,6 +5621,13 @@  cp_build_addr_expr (tree arg, tsubst_flags_t complain)
 static tree
 cp_build_addr_expr_strict (tree arg, tsubst_flags_t complain)
 {
+  if (TREE_CODE (TREE_TYPE (arg)) == FUNCTION_TYPE
+      && DECL_P (arg) && DECL_IS_BUILTIN (arg))
+    {
+      error_at (input_location, "taking address of a builtin function");
+      return error_mark_node;
+    }
+
   return cp_build_addr_expr_1 (arg, 1, complain);
 }

@@ -6860,6 +6867,14 @@  build_reinterpret_cast_1 (tree type, tree expr, bool c_cast_p,
 	  || VOID_TYPE_P (TREE_TYPE (type))))
     return convert_member_func_to_ptr (type, expr, complain);

+  if (TREE_CODE (intype) == FUNCTION_TYPE
+      && DECL_P (expr) && DECL_IS_BUILTIN (expr))
+    {
+      error_at (EXPR_LOC_OR_LOC (expr, input_location),
+		"taking address of a builtin function");
+      return error_mark_node;
+    }
+
   /* If the cast is not to a reference type, the lvalue-to-rvalue,
      array-to-pointer, and function-to-pointer conversions are
      performed.  */
@@ -8307,6 +8322,13 @@  convert_for_initialization (tree exp, tree type, tree rhs, int flags,
       || (TREE_CODE (rhs) == TREE_LIST && TREE_VALUE (rhs) == error_mark_node))
     return error_mark_node;

+  if (TREE_CODE (TREE_TYPE (rhs)) == FUNCTION_TYPE
+      && DECL_P (rhs) && DECL_IS_BUILTIN (rhs))
+    {
+      error_at (input_location, "taking address of a builtin function");
+      return error_mark_node;
+    }
+
   if ((TREE_CODE (TREE_TYPE (rhs)) == ARRAY_TYPE
        && TREE_CODE (type) != ARRAY_TYPE
        && (TREE_CODE (type) != REFERENCE_TYPE

2015-06-21  Martin Sebor  <msebor@redhat.com>

	PR c/66516
	* gcc.dg/addr_builtin-pr66516.c: New test.
	* g++.dg/addr_builtin-pr66516.C: New test.

diff --git a/gcc/testsuite/g++.dg/addr_builtin-pr66516.C b/gcc/testsuite/g++.dg/addr_builtin-pr66516.C
new file mode 100644
index 0000000..38f1a51
--- /dev/null
+++ b/gcc/testsuite/g++.dg/addr_builtin-pr66516.C
@@ -0,0 +1,70 @@ 
+/* { dg-do compile } */
+
+/* Verify that taking the address of a builtin function generates
+   a compilation error.  */
+
+#if 201103L <= __cplusplus
+
+bool func (void)
+{
+  /* Taking the address of builtin constants is valid.  */
+  return &__func__ == &__func__;             /* { dg-bogus "builtin" } */
+}
+
+#endif
+
+/* Calling the builtins is valid.  */
+int foo (int i)
+{
+  const void* p = 0;
+
+  switch (i & 0xf) {
+  case 0: i = __builtin_expect (i, 0);       /* { dg-bogus "builtin" } */
+  case 1: p = __builtin_return_address (0);  /* { dg-bogus "builtin" } */
+  case 2: i = __builtin_strlen ("");         /* { dg-bogus "builtin" } */
+  case 3: p = __builtin_FILE ();             /* { dg-bogus "builtin" } */
+  case 4: p = __builtin_FUNCTION ();         /* { dg-bogus "builtin" } */
+  case 5: i = __builtin_LINE ();             /* { dg-bogus "builtin" } */
+  }
+  return p ? *(int*)p : i;
+}
+
+void* const addr[] = {
+  (void*)__builtin_expect,          /* { dg-error "builtin" } */
+  (void*)__builtin_return_address,  /* { dg-error "builtin" } */
+  (void*)__atomic_load,             /* { dg-error "builtin" } */
+  (void*)__builtin_add_overflow,    /* { dg-error "builtin" } */
+  (void*)__builtin_constant_p,      /* { dg-error "builtin" } */
+  (void*)__builtin_nanf,            /* { dg-error "builtin" } */
+  (void*)__builtin_strlen,          /* { dg-error "builtin" } */
+  (void*)__builtin_unreachable,     /* { dg-error "builtin" } */
+  (void*)__builtin_FILE,            /* { dg-error "builtin" } */
+  (void*)__builtin_FUNCTION,        /* { dg-error "builtin" } */
+  (void*)__builtin_LINE,            /* { dg-error "builtin" } */
+
+  (void*)&__builtin_expect,         /* { dg-error "builtin" } */
+  (void*)&__builtin_return_address, /* { dg-error "builtin" } */
+  (void*)&__atomic_load,            /* { dg-error "builtin" } */
+  (void*)&__builtin_add_overflow,   /* { dg-error "builtin" } */
+  (void*)&__builtin_constant_p,     /* { dg-error "builtin" } */
+  (void*)&__builtin_nanf,           /* { dg-error "builtin" } */
+  (void*)&__builtin_strlen,         /* { dg-error "builtin" } */
+  (void*)&__builtin_unreachable,    /* { dg-error "builtin" } */
+  (void*)&__builtin_FILE,           /* { dg-error "builtin" } */
+  (void*)&__builtin_FUNCTION,       /* { dg-error "builtin" } */
+  (void*)&__builtin_LINE,           /* { dg-error "builtin" } */
+
+  reinterpret_cast<void*>(__builtin_expect),       /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__builtin_return_address), /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__atomic_load),          /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__builtin_add_overflow), /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__builtin_constant_p),   /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__builtin_nanf),         /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__builtin_strlen),       /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__builtin_unreachable),  /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__builtin_FILE),         /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__builtin_FUNCTION),     /* { dg-error "builtin" } */
+  reinterpret_cast<void*>(__builtin_LINE),         /* { dg-error "builtin" } */
+
+  0
+};
diff --git a/gcc/testsuite/gcc.dg/addr_builtin-pr66516.c b/gcc/testsuite/gcc.dg/addr_builtin-pr66516.c
new file mode 100644
index 0000000..b0b480e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/addr_builtin-pr66516.c
@@ -0,0 +1,59 @@ 
+/* { dg-do compile } */
+
+/* Verify that taking the address of a builtin function generates
+   a compilation error.  */
+
+#if 199901L <= __STDC_VERSION__
+
+int func (void)
+{
+  /* Taking the address of builtin constants is valid.  */
+  return &__func__ == &__func__;                /* { dg-bogus "builtin" } */
+}
+
+#endif
+
+
+/* Calling the builtins is valid.  */
+int foo (int i)
+{
+  const void* p = 0;
+
+  switch (i & 0xf) {
+  case 0: i = __builtin_expect (i, 0);       /* { dg-bogus "builtin" } */
+  case 1: p = __builtin_return_address (0);  /* { dg-bogus "builtin" } */
+  case 2: i = __builtin_strlen ("");         /* { dg-bogus "builtin" } */
+  case 3: p = __builtin_FILE ();             /* { dg-bogus "builtin" } */
+  case 4: p = __builtin_FUNCTION ();         /* { dg-bogus "builtin" } */
+  case 5: i = __builtin_LINE ();             /* { dg-bogus "builtin" } */
+  }
+  return p ? *(int*)p : i;
+}
+
+void* const addr[] = {
+  (void*)__builtin_expect,          /* { dg-error "builtin" } */
+  (void*)__builtin_return_address,  /* { dg-error "builtin" } */
+  (void*)__atomic_load,             /* { dg-error "builtin" } */
+  (void*)__builtin_add_overflow,    /* { dg-error "builtin" } */
+  (void*)__builtin_constant_p,      /* { dg-error "builtin" } */
+  (void*)__builtin_nanf,            /* { dg-error "builtin" } */
+  (void*)__builtin_strlen,          /* { dg-error "builtin" } */
+  (void*)__builtin_unreachable,     /* { dg-error "builtin" } */
+  (void*)__builtin_FILE,            /* { dg-error "builtin" } */
+  (void*)__builtin_FUNCTION,        /* { dg-error "builtin" } */
+  (void*)__builtin_LINE,            /* { dg-error "builtin" } */
+
+  (void*)&__builtin_expect,         /* { dg-error "builtin" } */
+  (void*)&__builtin_return_address, /* { dg-error "builtin" } */
+  (void*)&__atomic_load,            /* { dg-error "builtin" } */
+  (void*)&__builtin_add_overflow,   /* { dg-error "builtin" } */
+  (void*)&__builtin_constant_p,     /* { dg-error "builtin" } */
+  (void*)&__builtin_nanf,           /* { dg-error "builtin" } */
+  (void*)&__builtin_strlen,         /* { dg-error "builtin" } */
+  (void*)&__builtin_unreachable,    /* { dg-error "builtin" } */
+  (void*)&__builtin_FILE,           /* { dg-error "builtin" } */
+  (void*)&__builtin_FUNCTION,       /* { dg-error "builtin" } */
+  (void*)&__builtin_LINE,           /* { dg-error "builtin" } */
+
+  0
+};