Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-03 Thread Joseph Myers 
On Thu, 3 Sep 2015, Jason Merrill wrote:

> The C++ parts are OK.

The diagnostic should say "built-in" not "builtin" (see 
codingconventions.html).  The C parts are OK with that change (which will 
require testcases to be updated).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-03 Thread Jason Merrill 

The C++ parts are OK.

Jason

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-03 Thread Martin Sebor 

You've committed empty gcc/builtins.c.orig file, I've removed it, but
please be more careful next time.


And c/ or cp/ prefixes don't belong to c/ChangeLog or cp/ChangeLog
(also fixed).

Jakub



Thank you for fixing that up.

Martin

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-03 Thread Jakub Jelinek 
On Wed, Sep 02, 2015 at 03:53:07PM -0600, Martin Sebor wrote:
> gcc/ChangeLog
> 2015-09-02  Martin Sebor  
> 
>   PR c/66516
>   * doc/extend.texi (Other Builtins): Document when the address
>   of a builtin function can be taken.
> 
> gcc/c-family/ChangeLog
> 2015-09-02  Martin Sebor  
> 
>   PR c/66516
>   * c-common.h (c_decl_implicit, reject_gcc_builtin): Declare new
>   functions.
>   * c-common.c (reject_gcc_builtin): Define.

You've committed empty gcc/builtins.c.orig file, I've removed it, but
please be more careful next time.

> gcc/c/ChangeLog
> 2015-09-02  Martin Sebor  
> 
>   PR c/66516
>   * c/c-typeck.c (convert_arguments, parser_build_unary_op)

And c/ or cp/ prefixes don't belong to c/ChangeLog or cp/ChangeLog
(also fixed).

Jakub

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-02 Thread Jason Merrill 

On 09/01/2015 06:25 PM, Martin Sebor wrote:

Having now made this change, I don't think the added complexity
of three declarations and two trivial definitions of the new
c_decl_implicit function across five files is an improvement,


Three declarations?  Isn't declaring it in c-common.h enough?


especially since we're still checking for c_dialect_cxx().
(The change simply replaces:

   && (c_dialect_cxx () || !DECL_LANG_FLAG_2 (expr))

with

   && (c_dialect_cxx () || c_decl_implicit (expr))

as suggested.)


Seems like you can do without the check for C++ if you're defining this 
function for C++ (to just return false).


I agree with Joseph that the function is better.


+bool diag /* = true */)


Let's call these parameters "reject_builtin" rather than the generic "diag".


+function = decay_conversion (function, complain, false);


Please add a comment to "false", e.g. /*reject_builtin*/false

Jason

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-02 Thread Martin Sebor 

On 09/02/2015 09:29 AM, Jason Merrill wrote:

On 09/01/2015 06:25 PM, Martin Sebor wrote:

Having now made this change, I don't think the added complexity
of three declarations and two trivial definitions of the new
c_decl_implicit function across five files is an improvement,


Three declarations?  Isn't declaring it in c-common.h enough?

...


Seems like you can do without the check for C++ if you're defining this
function for C++ (to just return false).


Yes on both counts. Thanks.



I agree with Joseph that the function is better.


+ bool diag /* = true */)


Let's call these parameters "reject_builtin" rather than the generic
"diag".


+function = decay_conversion (function, complain, false);


Please add a comment to "false", e.g. /*reject_builtin*/false


Done.

The latest patch (attached) implements all of requested changes
plus a few tweaks to the tests to make them more strict.

I also noticed and fixed a gotcha in the dg-error directives
that might be interesting to mention: in prior patches tests
were all: /* dg-error "builtin" */
Unfortunately, since both the name of the test file and the
test function have the string "builtin" in them, the directives
would pass even if the error message wasn't the expected:

  builtin function ‘__builtin_trap’ must be directly called

This problem was hiding a few invalid C++ tests. I've fixed
the directives to avoid the problem.

I've tested it on x86_64 this time with no regressions.

Martin

gcc/ChangeLog
2015-09-02  Martin Sebor  

	PR c/66516
	* doc/extend.texi (Other Builtins): Document when the address
	of a builtin function can be taken.

gcc/c-family/ChangeLog
2015-09-02  Martin Sebor  

	PR c/66516
	* c-common.h (c_decl_implicit, reject_gcc_builtin): Declare new
	functions.
	* c-common.c (reject_gcc_builtin): Define.

gcc/c/ChangeLog
2015-09-02  Martin Sebor  

	PR c/66516
	* c/c-typeck.c (convert_arguments, parser_build_unary_op)
	(build_conditional_expr, c_cast_expr, convert_for_assignment)
	(build_binary_op, _objc_common_truthvalue_conversion): Call
	reject_gcc_builtin.
	(c_decl_implicit): Define.

gcc/cp/ChangeLog
2015-09-02  Martin Sebor  

	PR c/66516
	* cp/cp-tree.h (mark_rvalue_use, decay_conversion): Add new
	argument(s).
	* cp/expr.c (mark_rvalue_use): Use new argument.
	* cp/call.c (build_addr_func): Call decay_conversion with new
	argument.
	* cp/pt.c (convert_template_argument): Call reject_gcc_builtin.
	* cp/typeck.c (decay_conversion): Use new argument.
	(c_decl_implicit): Define.

gcc/testsuite/ChangeLog
2015-09-02  Martin Sebor  

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

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 7691035..4cc6c3e 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -12882,4 +12882,41 @@ pointer_to_zero_sized_aggr_p (tree t)
   return (TYPE_SIZE (t) && integer_zerop (TYPE_SIZE (t)));
 }

+/* For an EXPR of a FUNCTION_TYPE that references a GCC built-in function
+   with no library fallback or for an ADDR_EXPR whose operand is such type
+   issues an error pointing to the location LOC.
+   Returns true when the expression has been diagnosed and false
+   otherwise.  */
+bool
+reject_gcc_builtin (const_tree expr, location_t loc /* = UNKNOWN_LOCATION */)
+{
+  if (TREE_CODE (expr) == ADDR_EXPR)
+expr = TREE_OPERAND (expr, 0);
+
+  if (TREE_TYPE (expr)
+  && TREE_CODE (TREE_TYPE (expr)) == FUNCTION_TYPE
+  && DECL_P (expr)
+  /* The intersection of DECL_BUILT_IN and DECL_IS_BUILTIN avoids
+	 false positives for user-declared built-ins such as abs or
+	 strlen, and for C++ operators new and delete.
+	 The c_decl_implicit() test avoids false positives for implicitly
+	 declared builtins with library fallbacks (such as abs).  */
+  && DECL_BUILT_IN (expr)
+  && DECL_IS_BUILTIN (expr)
+  && !c_decl_implicit (expr)
+  && !DECL_ASSEMBLER_NAME_SET_P (expr))
+{
+  if (loc == UNKNOWN_LOCATION)
+	loc = EXPR_LOC_OR_LOC (expr, input_location);
+
+  /* Reject arguments that are builtin functions with
+	 no library fallback.  */
+  error_at (loc, "builtin function %qE must be directly called", expr);
+
+  return true;
+}
+
+  return false;
+}
+
 #include "gt-c-family-c-common.h"
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index be63cd2..0d589b5 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -572,6 +572,7 @@ extern int field_decl_cmp (const void *, const void *);
 extern void resort_sorted_fields (void *, void *, gt_pointer_operator,
   void *);
 extern bool has_c_linkage (const_tree decl);
+extern bool c_decl_implicit (const_tree);
 
 /* Switches common to the C front ends.  */

@@ -1437,5 +1438,6 @@ extern bool contains_cilk_spawn_stmt (tree);
 extern tree cilk_for_number_of_iterations (tree);
 extern bool check_no_cilk (tree, const

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-01 Thread Joseph Myers 
On Tue, 1 Sep 2015, Martin Sebor wrote:

> Attached is an updated patch that avoids diagnosing taking the address
> of implicitly declared library builtins like abs, bootstrapped and
> tested on ppc64le with no regressions.
> 
> The tweak below was added to reject_gcc_builtin make it possible.
> Since the expression is in c-family/c-common.c, the more descriptive
> C_DECL_IMPLICIT that exists for this purpose is not available (it's
> defined in c/c-tree.h).
> 
> +  && (c_dialect_cxx () || !DECL_LANG_FLAG_2 (expr))

I don't think hardcoding DECL_LANG_FLAG_2 here is a good idea; rather, 
some sort of c_decl_implicit hook should be defined (that would just 
return false for C++) - existing practice is various functions that have 
different definitions for C and C++.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-01 Thread Joseph Myers 
On Tue, 1 Sep 2015, Martin Sebor wrote:

> I also noticed uses of DECL_LANG_FLAG_4 in the definitions of
> what appear to be C-specific macros in c-family/c-common.h,
> and then uses of the same macro in definitions of a C++-specific
> macro in cp/cp-tree.h.

That seems like a bug waiting to happen, given that there is nothing 
obviously C-specific about the users in common code of those macros.

> In this light it seems to me that leaving the test for the flag
> as it was would be both in keeping with existing practice and
> preferable to introducing the hook.

The existing practice you found was of use of DECL_LANG_FLAG_* in defining 
macros.  Not direct use in C files, which is clearly much worse.  I 
suppose there's the option of defining the macro in c-common.h, but 
defining it in a way that includes an assertion that it's never evaluated 
for C++.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-01 Thread Martin Sebor 

On 09/01/2015 11:29 AM, Joseph Myers wrote:

On Tue, 1 Sep 2015, Martin Sebor wrote:


Attached is an updated patch that avoids diagnosing taking the address
of implicitly declared library builtins like abs, bootstrapped and
tested on ppc64le with no regressions.

The tweak below was added to reject_gcc_builtin make it possible.
Since the expression is in c-family/c-common.c, the more descriptive
C_DECL_IMPLICIT that exists for this purpose is not available (it's
defined in c/c-tree.h).

+  && (c_dialect_cxx () || !DECL_LANG_FLAG_2 (expr))


I don't think hardcoding DECL_LANG_FLAG_2 here is a good idea; rather,
some sort of c_decl_implicit hook should be defined (that would just
return false for C++) - existing practice is various functions that have
different definitions for C and C++.


I've made the suggested change and tested it by bootstrapping
and running the two tests.

Before I post another revision of the patch for review let me
make sure there are no other change requests, and that introducing
a new c_decl_implicit hook here is what everyone wants.

The original patch had a distinct function for each of the C and
C++ front ends to issue the diagnostics because each had slightly
different tests. I've since merged the two functions into one on
Jason's suggestion and removed most of the  differences. Adding
a hook seems to be step in the opposite direction.

Having now made this change, I don't think the added complexity
of three declarations and two trivial definitions of the new
c_decl_implicit function across five files is an improvement,
especially since we're still checking for c_dialect_cxx().
(The change simply replaces:

  && (c_dialect_cxx () || !DECL_LANG_FLAG_2 (expr))

with

  && (c_dialect_cxx () || c_decl_implicit (expr))

as suggested.)

Having also looked at the existing code in c-family/common.c
that does things differently between C and C++ I see examples
of both hooks for more complex computations, and conditionals
similar to the one I just replaced for simpler things.

I also noticed uses of DECL_LANG_FLAG_4 in the definitions of
what appear to be C-specific macros in c-family/c-common.h,
and then uses of the same macro in definitions of a C++-specific
macro in cp/cp-tree.h.

In this light it seems to me that leaving the test for the flag
as it was would be both in keeping with existing practice and
preferable to introducing the hook.

Let me know.

Thanks
Martin

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-09-01 Thread Martin Sebor 

Attached is an updated patch that avoids diagnosing taking the address
of implicitly declared library builtins like abs, bootstrapped and
tested on ppc64le with no regressions.

The tweak below was added to reject_gcc_builtin make it possible.
Since the expression is in c-family/c-common.c, the more descriptive
C_DECL_IMPLICIT that exists for this purpose is not available (it's
defined in c/c-tree.h).

+  && (c_dialect_cxx () || !DECL_LANG_FLAG_2 (expr))

Martin
gcc/ChangeLog
2015-08-31  Martin Sebor  

	PR c/66516
	* doc/extend.texi (Other Builtins): Document when the address
	of a builtin function can be taken.

gcc/c-family/ChangeLog
2015-08-31  Martin Sebor  

	PR c/66516
	* c-common.h (reject_gcc_builtin): Declare new function.
	* c-common.c (reject_gcc_builtin): Define it.

gcc/c/ChangeLog
2015-08-31  Martin Sebor  

	PR c/66516
	* c/c-typeck.c (convert_arguments, parser_build_unary_op)
	(build_conditional_expr, c_cast_expr, convert_for_assignment)
	(build_binary_op, _objc_common_truthvalue_conversion): Call
	reject_gcc_builtin.

gcc/cp/ChangeLog
2015-08-31  Martin Sebor  

	PR c/66516
	* cp/cp-tree.h (mark_rvalue_use, decay_conversion): Add new
	argument(s).
	* cp/expr.c (mark_rvalue_use): Use new argument.
	* cp/call.c (build_addr_func): Call decay_conversion with new
	argument.
	* cp/pt.c (convert_template_argument): Call reject_gcc_builtin.
	* cp/typeck.c (decay_conversion): Use new argument.

gcc/testsuite/ChangeLog
2015-08-31  Martin Sebor  

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

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 7691035..8cca668 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -12882,4 +12882,41 @@ pointer_to_zero_sized_aggr_p (tree t)
   return (TYPE_SIZE (t) && integer_zerop (TYPE_SIZE (t)));
 }

+/* For an EXPR of a FUNCTION_TYPE that references a GCC built-in function
+   with no library fallback or for an ADDR_EXPR whose operand is such type
+   issues an error pointing to the location LOC.
+   Returns true when the expression has been diagnosed and false
+   otherwise.  */
+bool
+reject_gcc_builtin (const_tree expr, location_t loc /* = UNKNOWN_LOCATION */)
+{
+  if (TREE_CODE (expr) == ADDR_EXPR)
+expr = TREE_OPERAND (expr, 0);
+
+  if (TREE_TYPE (expr)
+  && TREE_CODE (TREE_TYPE (expr)) == FUNCTION_TYPE
+  && DECL_P (expr)
+  /* The intersection of DECL_BUILT_IN and DECL_IS_BUILTIN avoids
+	 false positives for user-declared built-ins such as abs or
+	 strlen, and for C++ operators new and delete.
+	 In C mode only, DECL_LANG_FLAG_2 avoids false positives for
+	 implicitly declared builtins with library fallbacks.  */
+  && DECL_BUILT_IN (expr)
+  && DECL_IS_BUILTIN (expr)
+  && (c_dialect_cxx () || !DECL_LANG_FLAG_2 (expr))
+  && !DECL_ASSEMBLER_NAME_SET_P (expr))
+{
+  if (loc == UNKNOWN_LOCATION)
+	loc = EXPR_LOC_OR_LOC (expr, input_location);
+
+  /* Reject arguments that are builtin functions with
+	 no library fallback.  */
+  error_at (loc, "builtin function %qE must be directly called", expr);
+
+  return true;
+}
+
+  return false;
+}
+
 #include "gt-c-family-c-common.h"
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index be63cd2..559a561 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1437,5 +1437,6 @@ extern bool contains_cilk_spawn_stmt (tree);
 extern tree cilk_for_number_of_iterations (tree);
 extern bool check_no_cilk (tree, const char *, const char *,
 		   location_t loc = UNKNOWN_LOCATION);
+extern bool reject_gcc_builtin (const_tree, location_t = UNKNOWN_LOCATION);

 #endif /* ! GCC_C_COMMON_H */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index e8c8189..ca4e3e2 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -3339,6 +3339,10 @@ convert_arguments (location_t loc, vec arg_loc, tree typelist,
 	  error (invalid_func_diag);
 	  return -1;
 	}
+  else if (TREE_CODE (val) == ADDR_EXPR && reject_gcc_builtin (val))
+	{
+	  return -1;
+	}
   else
 	/* Convert `short' and `char' to full-size `int'.  */
 	parmval = default_conversion (val);
@@ -3376,12 +3380,20 @@ parser_build_unary_op (location_t loc, enum tree_code code, struct c_expr arg)
 {
   struct c_expr result;

-  result.value = build_unary_op (loc, code, arg.value, 0);
   result.original_code = code;
   result.original_type = NULL;

+  if (reject_gcc_builtin (arg.value))
+{
+  result.value = error_mark_node;
+}
+  else
+{
+  result.value = build_unary_op (loc, code, arg.value, 0);
+
   if (TREE_OVERFLOW_P (result.value) && !TREE_OVERFLOW_P (arg.value))
 overflow_warning (loc, result.value);
+}

   return result;
 }
@@ -4484,6 +4496,12 @@ build_conditional_expr (location_t colon_loc, tree ifexp, bool ifexp_bcp,
   type2 = TREE_TYPE (op2);
   code2 = TREE_CODE

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-08-28 Thread Martin Sebor 

On 08/28/2015 02:09 PM, Joseph Myers wrote:

On Fri, 28 Aug 2015, Martin Sebor wrote:


I ran into one regression in the gcc.dg/lto/pr54702_1.c test.
The file takes the address of malloc without declaring it, and
after calling it first. The code is invalid but GCC compiles it
due to a bug. I raised it in c/67386 -- missing diagnostic on
a use of an undeclared function, and suppressed the error by


But that PR isn't a bug - the code is working exactly as it's meant to (an
implicit declaration acts exactly like an explicit declaration int func
(); in the nearest containing scope).  The declaration has an
incompatible type, it's true, but GCC deliberately allows that with a
warning.
What if (a) you use a built-in function that returns int, instead of
malloc, and (b) use -std=gnu89, so the implicit declaration isn't even an
extension?  Then you have something that's completely valid, including
taking the address of the implicitly declared function.


In that case the patched GCC issues an error for taking the address
of the undeclared function as the test case below shows.

I was aware of the C90 implicit declaration rule but I interpreted
it as saying that the injected declaration is only in effect for
the call expression. Since no other tests broke, I assumed the one
that did was buggy. Anyway, after testing a few other compilers it
looks like they all also extend the implicit declaration through
the rest of the scope, so the patch will need further tweaking to
allow this corner case.

The problem is that DECL_IS_BUILTIN(expr) returns true for
an implicitly declared builtin function with a library fallback
but false for one that's been declared explicitly. I'll either
have to find some other test to determine that the implicitly
declared function has a fallback or fix whatever is causing
the macro to return the wrong value.

Martin

$ cat t.c  /build/gcc-66516/gcc/xgcc -B /build/gcc-66516/gcc 
-std=gnu89 t.cint (*p)(int);


void foo (void) {
p = abs;
}

int bar (void) {
int n = abs (0);
p = abs;
return n;
}

t.c: In function ‘foo’:
t.c:4:9: error: ‘abs’ undeclared (first use in this function)
 p = abs;
 ^
t.c:4:9: note: each undeclared identifier is reported only once for each 
function it appears in

t.c: In function ‘bar’:
t.c:9:5: error: builtin function ‘abs’ must be directly called
 p = abs;
 ^

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-08-28 Thread Martin Sebor 

The second question is about your suggestion to consolidate the code
into mark_rvalue_use. The problem I'm running into there is that
mark_rvalue_use is called for calls to builtins as well as for other
uses and doesn't have enough context to tell one from the other.


Ah, true.  But special-casing call uses is still fewer places than
special-casing all non-call uses.


Sorry it's taken me so long to get back to this.

Changing the patch to issue the diagnostic in mark_rvalue_use instead
of anywhere in expr.c or call.c caused a false positive for calls to
builtin functions, and a number of false negatives (e.g., for the
address-of expression and for reinterpret_castF(builtin)).

To avoid the false positive I added a new argument to both
mark_rvalue_use and decay_conversion (which calls mark_rvalue_use
when called from build_addr_func) to avoid diagnosing calls to
builtins.

To avoid the false negatives, we still need to retain some calls to
cp_reject_gcc_builtin from functions other than mark_rvalue_use.

I also removed the DECL_IS_GCC_BUILTIN macro introduced in the first
patch and replaced its uses with a somewhat simplified expression
that's the same between the C and C++ front ends.

Finally, I merged the c_ and cp_reject_gcc_builtin functions into
a single reject_gcc_builtin function and simplified the logic to
avoid testing for operators new and delete by name. I removed from
the C++ version the checks for the type substitution flags since
(IIUC) they don't come into play here (all uses of the builtins
can be diagnosed, regardless of the type substitution context).

I ran into one regression in the gcc.dg/lto/pr54702_1.c test.
The file takes the address of malloc without declaring it, and
after calling it first. The code is invalid but GCC compiles it
due to a bug. I raised it in c/67386 -- missing diagnostic on
a use of an undeclared function, and suppressed the error by
adding a declaration to the test. I mention it here because
the error that GCC issues otherwise (without the declaration)
is one about __builtin_malloc not being directly called. I'm
sure the error will change to 'malloc' undeclared once PR67386
is fixed but until then, in this case, the error is rather cryptic.
I haven't spent too much time trying to fix it (it seems to have
to do with the undeclared malloc being treated as a builtin without
a library fallback).

Let me know if this is closer to what you were suggesting or if
you would like to see some other changes (or prefer the original
approach).

I tested the patch by bootstrapping on 86_64 and on powerpc64le
and running tests on the latter. I see the following difference
in the test results

Thanks
Martin



gcc/ChangeLog
2015-08-27  Martin Sebor  mse...@redhat.com

	PR c/66516
	* doc/extend.texi (Other Builtins): Document when the address
	of a builtin function can be taken.

gcc/c-family/ChangeLog
2015-08-27  Martin Sebor  mse...@redhat.com

	PR c/66516
	* c-common.h (reject_gcc_builtin): Declare new function.
	* c-common.c (reject_gcc_builtin): Define it.

gcc/c/ChangeLog
2015-08-27  Martin Sebor  mse...@redhat.com

	PR c/66516
	* c/c-typeck.c (convert_arguments, parser_build_unary_op)
	(build_conditional_expr, c_cast_expr, convert_for_assignment)
	(build_binary_op, _objc_common_truthvalue_conversion): Call
	reject_gcc_builtin.

gcc/cp/ChangeLog
2015-08-27  Martin Sebor  mse...@redhat.com

	PR c/66516
	* cp/cp-tree.h (mark_rvalue_use, decay_conversion): Add new
	argument(s).
	* cp/expr.c (mark_rvalue_use): Use new argument.
	* cp/call.c (build_addr_func): Call decay_conversion with new
	argument.
	* cp/pt.c (convert_template_argument): Call reject_gcc_builtin.
	* cp/typeck.c (decay_conversion): Use new argument.

gcc/testsuite/ChangeLog
2015-08-27  Martin Sebor  mse...@redhat.com

	PR c/66516
	* g++.dg/addr_builtin-1.C: New test.
	* gcc.dg/addr_builtin-1.c: New test.
	* gcc.dg/lto/pr54702_1.c: Declare malloc before taking its address.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 7691035..8fda350 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -12882,4 +12882,38 @@ pointer_to_zero_sized_aggr_p (tree t)
   return (TYPE_SIZE (t)  integer_zerop (TYPE_SIZE (t)));
 }

+/* For an EXPR of a FUNCTION_TYPE that references a GCC built-in function
+   with no library fallback or for an ADDR_EXPR whose operand is such type
+   issues an error pointing to the location LOC.
+   Returns true when the expression has been diagnosed and false
+   otherwise.  */
+bool
+reject_gcc_builtin (const_tree expr, location_t loc /* = UNKNOWN_LOCATION */)
+{
+  if (TREE_CODE (expr) == ADDR_EXPR)
+expr = TREE_OPERAND (expr, 0);
+
+  if (TREE_TYPE (expr)
+   TREE_CODE (TREE_TYPE (expr)) == FUNCTION_TYPE
+   DECL_P (expr)
+  /* The intersection of DECL_BUILT_IN and DECL_IS_BUILTIN avoids
+	 false positives for user-declared built-ins such as abs or
+	 strlen, and for C++ operators new and delete.  */
+   DECL_BUILT_IN (expr)
+

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-08-28 Thread Joseph Myers 
On Fri, 28 Aug 2015, Martin Sebor wrote:

 I ran into one regression in the gcc.dg/lto/pr54702_1.c test.
 The file takes the address of malloc without declaring it, and
 after calling it first. The code is invalid but GCC compiles it
 due to a bug. I raised it in c/67386 -- missing diagnostic on
 a use of an undeclared function, and suppressed the error by

But that PR isn't a bug - the code is working exactly as it's meant to (an 
implicit declaration acts exactly like an explicit declaration int func 
(); in the nearest containing scope).  The declaration has an 
incompatible type, it's true, but GCC deliberately allows that with a 
warning.

What if (a) you use a built-in function that returns int, instead of 
malloc, and (b) use -std=gnu89, so the implicit declaration isn't even an 
extension?  Then you have something that's completely valid, including 
taking the address of the implicitly declared function.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-08-04 Thread Jason Merrill 

On 08/03/2015 07:02 PM, Martin Sebor wrote:

I've prototyped this approach in a couple places in the middle
end. Both implementations are very simple and work great when
the code isn't optimized away. The problem is that the
optimizations done at various points between the front end and
the final gimplification make the diagnostics inconsistent.

For instance, with F being the type of the builtin, this is
diagnosed in the first prototype:

   F* foo () { if (0) return __builtin_trap; return 0; }

but this isn't:

   F* bar () { return 0 ? __builtin_trap : 0; }

because the ternary ?: expression is folded into a constant by
the front end even before it reaches the gimplifier, while the
if-else statement isn't folded until the control flow graph is
built. (As an aside: I'm wondering why that is. Why have the
front end do any optimization at all if the middle can and
does them too, and better?)


This is largely historical baggage, I think, from days where computers 
had less memory and we were trying to do as much processing as possible 
immediately.


The c++-delayed-folding branch delays folding the ?: expression until 
the end of the function, at which point we can see better what context 
the function is being used in, which could simplify your patch.


But your question leads me to wonder if we need to do front end folding 
there, either...


Jason

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-08-04 Thread Joseph Myers 
On Mon, 3 Aug 2015, Martin Sebor wrote:

 because the ternary ?: expression is folded into a constant by
 the front end even before it reaches the gimplifier, while the
 if-else statement isn't folded until the control flow graph is
 built. (As an aside: I'm wondering why that is. Why have the
 front end do any optimization at all if the middle can and
 does them too, and better?)

Well, in C, if an expression is an integer constant expression, then in 
certain contexts its folded value needs to be known for checking lots of 
other constraints that should be diagnosed in the front end, such as on 
bit-field widths and array sizes - and then it needs to be known for 
layout so that sizeof and offsetof (which can be used in other integer 
constant expressions) can be computed.  And in other contexts (pointers 
conditional expressions), the value of an integer constant expression 
affects the type of the containing expression, so types cannot be 
determined without folding (with consequent effects on other diagnostics).  
And then certain expressions that aren't integer constant expressions but 
can be folded to them in fact get used (e.g. in the Linux kernel) in 
contexts requiring integer constant expressions, so also need folding.  
And then a wider range of expressions need folding in static initializers 
so the front end can diagnose whether an initializer is valid or not, 
while allowing extensions that again are used in practice (though there's 
a possibility such diagnostics for initializers could be left until after 
some middle-end optimization).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-08-04 Thread Jeff Law 

On 08/04/2015 09:04 AM, Jason Merrill wrote:


This is largely historical baggage, I think, from days where computers
had less memory and we were trying to do as much processing as possible
immediately.
Right.  Also note the early folding was from a time before we had the 
gimple optimization pipeline -- the only significant optimizations we 
did on trees was the (excessive) fold-const.c stuff.





The c++-delayed-folding branch delays folding the ?: expression until
the end of the function, at which point we can see better what context
the function is being used in, which could simplify your patch.
Right.   And that's a design direction we want to take with folding in 
general -- delay it until the transition to gimple/ssa.  That way what 
we get out of the front-ends looks much more like the original source.


jeff

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-08-03 Thread Martin Sebor 

I suspect the back end or even the middle end route isn't going to
work even if there was enough context to diagnose the problem
expressions because some of them will have been optimized away by then
(e.g., 'if ( __builtin_foo != 0)' is optimized into if (1) by gimple).


I was thinking that if they're optimized away, they aren't problematic
anymore; that was part of the attraction for me of handling this lower
down.


Yes, they're not problematic in that they don't cause linker unsats.
I have a few concerns with the approach of accepting or rejecting
constructs based on optimization (e.g., that it might lead to
similar problems as with -Wmaybe-uninitialized; or that it could
be perceived as inconsistent). But it may not be as bad as it seems
-- let me look into it if only as a learning exercise and see how
it goes.


I've prototyped this approach in a couple places in the middle
end. Both implementations are very simple and work great when
the code isn't optimized away. The problem is that the
optimizations done at various points between the front end and
the final gimplification make the diagnostics inconsistent.

For instance, with F being the type of the builtin, this is
diagnosed in the first prototype:

  F* foo () { if (0) return __builtin_trap; return 0; }

but this isn't:

  F* bar () { return 0 ? __builtin_trap : 0; }

because the ternary ?: expression is folded into a constant by
the front end even before it reaches the gimplifier, while the
if-else statement isn't folded until the control flow graph is
built. (As an aside: I'm wondering why that is. Why have the
front end do any optimization at all if the middle can and
does them too, and better?)

Diagnosing neither of these cases might perhaps seem like the
way to go since they are both safe (don't result in a linker
error). I implemented this approach in the second prototype in
a later pass but that exposed even more inconsistencies as even
more code is optimized (for instance, exception handling).

I think this sort of inconsistency in diagnosing semantically
equivalent language constructs would be viewed as arbitrary
and be unhelpful to users. It's too bad because otherwise this
solution would have been quite elegant and simple (touching
just one function).

Unless you have some other ideas I'm going to abandon this
approach see if the original patch can be simplified by
consoldating some of the handling into mark_rvalue_use.

Martin

PS Another problem, one that in my view sinks the middle end
approach, is the fact that references in unused static inline
functions aren't diagnosed either, even if calling the inline
function would result in taking the address of the builtin
function. E.g., this isn't diagnosed:

  static inline F* baz (void) { return __builtin_trap; }

unless baz() is called. This would make the feature largely
ineffective in C++ template libraries.

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-07-29 Thread Martin Sebor 

On 07/28/2015 09:38 PM, Jason Merrill wrote:

Sorry about the slow response on IRC today, I got distracted onto
another issue and forgot to check back.  What I started to write:


No problem.




I'm exploring your suggestion to see if the back end could emit the
diagnostics. But I'm not sure it has sufficient context (location
information) to point to the line of code that uses the function.


Hmm, that's a good point.  I think it would make sense for the ADDR_EXPR
to carry location information as long as we're dealing with trees, but I
suspect we don't currently set the location of an ADDR_EXPR.  So that
would need to be fixed as part of this approach.


Okay, let me look into this.




I suspect the back end or even the middle end route isn't going to
work even if there was enough context to diagnose the problem
expressions because some of them will have been optimized away by then
(e.g., 'if ( __builtin_foo != 0)' is optimized into if (1) by gimple).


I was thinking that if they're optimized away, they aren't problematic
anymore; that was part of the attraction for me of handling this lower
down.


Yes, they're not problematic in that they don't cause linker unsats.
I have a few concerns with the approach of accepting or rejecting
constructs based on optimization (e.g., that it might lead to
similar problems as with -Wmaybe-uninitialized; or that it could
be perceived as inconsistent). But it may not be as bad as it seems
-- let me look into it if only as a learning exercise and see how
it goes.




The second question is about your suggestion to consolidate the code
into mark_rvalue_use. The problem I'm running into there is that
mark_rvalue_use is called for calls to builtins as well as for other
uses and doesn't have enough context to tell one from the other.


Ah, true.  But special-casing call uses is still fewer places than
special-casing all non-call uses.


This will be moot if I can implement it the middle-end. If not,
I'll give it a try to see if this alternative ends up reducing
the footprint of the patch.

Thanks
Martin

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-07-28 Thread Jason Merrill 
Sorry about the slow response on IRC today, I got distracted onto 
another issue and forgot to check back.  What I started to write:



I'm exploring your suggestion to see if the back end could emit the 
diagnostics. But I'm not sure it has sufficient context (location information) 
to point to the line of code that uses the function.


Hmm, that's a good point.  I think it would make sense for the ADDR_EXPR 
to carry location information as long as we're dealing with trees, but I 
suspect we don't currently set the location of an ADDR_EXPR.  So that 
would need to be fixed as part of this approach.



I suspect the back end or even the middle end route isn't going to work even if 
there was enough context to diagnose the problem expressions because some of them 
will have been optimized away by then (e.g., 'if ( __builtin_foo != 0)' is 
optimized into if (1) by gimple).


I was thinking that if they're optimized away, they aren't problematic 
anymore; that was part of the attraction for me of handling this lower down.



The second question is about your suggestion to consolidate the code into 
mark_rvalue_use. The problem I'm running into there is that mark_rvalue_use is 
called for calls to builtins as well as for other uses and doesn't have enough 
context to tell one from the other.


Ah, true.  But special-casing call uses is still fewer places than 
special-casing all non-call uses.


Jason

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-07-14 Thread Jason Merrill 
I wonder if it would make sense to handle this when we actually try to 
emit a reference to one of these functions in the back end, rather than 
in many places through the front end.


If it's going to stay in the front end, the C and C++ front-ends should 
share an implementation of this function, in c-common.c.


Most of the calls in the C++ front end can be replaced by a single call 
in mark_rvalue_use.



-#ifdef ENABLE_CHECKING
+#if 0 // def ENABLE_CHECKING


This change is unrelated.


+#define DECL_IS_GCC_BUILTIN(DECL) \


This macro name could be clearer.  DECL_IS_ONLY_BUILTIN? 
DECL_IS_NOFALLBACK_BUILTIN?


Jason

Re: [PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-07-14 Thread Martin Sebor 

On 07/14/2015 09:01 AM, Jason Merrill wrote:

I wonder if it would make sense to handle this when we actually try to
emit a reference to one of these functions in the back end, rather than
in many places through the front end.

If it's going to stay in the front end, the C and C++ front-ends should
share an implementation of this function, in c-common.c.


Thanks for the comments and the suggestion. It would certainly
be much cleaner if it could be detected in one place. Let me
investigate the back end option.

FWIW, my initial attempt at a patch did have a single function
with common logic, until it became clear that the C++ conditions
the function tested were slightly different (and more involved
due to the checks for operators new and delete) than those in
C. But since they don't seem to be incompatible with one another,
as long as no one minds that when called from the C front end
the function ends up doing some unnecessary work I can merge
them back (if the back end idea doesn't pan out).



Most of the calls in the C++ front end can be replaced by a single call
in mark_rvalue_use.


-#ifdef ENABLE_CHECKING
+#if 0 // def ENABLE_CHECKING


This change is unrelated.


Yes, sorry about that.




+#define DECL_IS_GCC_BUILTIN(DECL) \


This macro name could be clearer.  DECL_IS_ONLY_BUILTIN?
DECL_IS_NOFALLBACK_BUILTIN?


I derived DECL_IS_GCC_BUILTIN from the DEF_GCC_BUILTIN macro
in builtins.def used to define such builtins. There are no
DECL_IS_XXX_BUILTINs for the other DEF_XXX_BUILTIN macros yet
but I imagine there could be and it seems that keeping the
names consistent would make the two sets of macros easier to
understand and work with. But if you don't think this is
important I'm fine renaming the macro.

Martin

[PING] Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-07-13 Thread Martin Sebor 

[CC Jason since the patch also touches the C++ front end]

The updated patch is here:
  https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00258.html

Thanks
Martin

On 07/04/2015 04:32 PM, Martin Sebor wrote:

I don't think c_validate_addressable is a good name - given that it's
called for lots of things that aren't addressable, in contexts in which
there is no need for them to be addressable, and doesn't do checks of
addressability in contexts where they are actually needed and done
elsewhere (e.g. checks for not taking the address of a register
variable).
The question seems to be something more like: is the expression used
as an
operand something it's OK to use as an operand at all?


Thank you for the review.

I've changed the name to c_reject_gcc_builtin. If you would prefer
a different name still please suggest one. I'm not partial to any
one in particular.


What is the logic for the list of functions above being a complete
list of
the places that need changes?


The logic by which I arrived at the changes was by constructing
test cases exercising expressions where a function is a valid
operand and where its address might need to be obtained when
it's not available, and stepping through the code and modifying
it until I found a suitable place to change to reject it.


How can ifexp be of pointer type?  It's undergone truthvalue conversion
and should always be of type int at this point (but in any case, you
can't
refer to TREE_OPERAND (ifexp, 0) without knowing what sort of expression
it is).


I've removed the redundant test from this function.




+/* For EXPR that is an ADDR_EXPR or whose type is a FUNCTION_TYPE,
+   determines whether its operand can have its address taken issues
+   an error pointing to the location LOC.
+   Operands that cannot have their address taken are builtin functions
+   that have no library fallback (no other kinds of expressions are
+   considered).
+   Returns true when the expression can have its address taken and
+   false otherwise.  */


Apart from the naming issue, the comment says nothing about the semantics
of the function for an argument that's not of that form.


I've reworded the comment to hopefully make the semantics of
the function more clear.

Attached is an updated patch with these changes.

Martin

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-07-04 Thread Martin Sebor 

I don't think c_validate_addressable is a good name - given that it's
called for lots of things that aren't addressable, in contexts in which
there is no need for them to be addressable, and doesn't do checks of
addressability in contexts where they are actually needed and done
elsewhere (e.g. checks for not taking the address of a register variable).
The question seems to be something more like: is the expression used as an
operand something it's OK to use as an operand at all?


Thank you for the review.

I've changed the name to c_reject_gcc_builtin. If you would prefer
a different name still please suggest one. I'm not partial to any
one in particular.


What is the logic for the list of functions above being a complete list of
the places that need changes?


The logic by which I arrived at the changes was by constructing
test cases exercising expressions where a function is a valid
operand and where its address might need to be obtained when
it's not available, and stepping through the code and modifying
it until I found a suitable place to change to reject it.


How can ifexp be of pointer type?  It's undergone truthvalue conversion
and should always be of type int at this point (but in any case, you can't
refer to TREE_OPERAND (ifexp, 0) without knowing what sort of expression
it is).


I've removed the redundant test from this function.




+/* For EXPR that is an ADDR_EXPR or whose type is a FUNCTION_TYPE,
+   determines whether its operand can have its address taken issues
+   an error pointing to the location LOC.
+   Operands that cannot have their address taken are builtin functions
+   that have no library fallback (no other kinds of expressions are
+   considered).
+   Returns true when the expression can have its address taken and
+   false otherwise.  */


Apart from the naming issue, the comment says nothing about the semantics
of the function for an argument that's not of that form.


I've reworded the comment to hopefully make the semantics of
the function more clear.

Attached is an updated patch with these changes.

Martin
gcc/ChangeLog

2015-07-04  Martin Sebor  mse...@redhat.com

	pr c/66516
	* tree.h (DECL_IS_GCC_BUILTIN): New macro.
	* doc/extend.texi (Other Builtins): Document when the address of
	a built-in function can be taken.

gcc/c/ChangeLog

2015-07-04  Martin Sebor  mse...@redhat.com

	pr c/66516
	* c-tree.h (c_reject_gcc_builtin): New function.
	* c-typeck.c (convert_arguments, parser_build_unary_op): Call it.
	(build_conditional_expr, c_cast_expr, convert_for_assignment): Same.
	(build_binary_op, c_objc_common_truthvalue_conversion): Same.
	(c_reject_gcc_builtin): Define function.

gcc/cp/ChangeLog

2015-07-04  Martin Sebor  mse...@redhat.com

	pr c/66516
	* cp-tree.h (cp_reject_gcc_builtin): New function.
	* call.c (build_conditional_expr_1): Call it.
	(convert_arg_to_ellipsis, convert_for_arg_passing): Same.
	* pt.c (convert_template_argument): Same.
	* typeck.c (cp_build_binary_op, cp_build_addr_expr_strict): Same.
	(cp_build_unary_op, build_static_cast_1, build_reinterpret_cast_1):
	Same.
	(cp_build_c_cast, convert_for_assignment, convert_for_initialization):
	Same.
	(cp_reject_gcc_builtin): Define function.

gcc/testsuite/ChangeLog

2015-07-04  Martin Sebor  mse...@redhat.com

	pr c/66516
	* g++.dg/addr_builtin-1.C: New test.
	* gcc.dg/addr_builtin-1.c: New test.

diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index 28b58c6..6a492c8 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -655,6 +655,7 @@ extern tree c_finish_transaction (location_t, tree, int);
 extern bool c_tree_equal (tree, tree);
 extern tree c_build_function_call_vec (location_t, veclocation_t, tree,
    vectree, va_gc *, vectree, va_gc *);
+extern bool c_reject_gcc_builtin (const_tree, location_t = UNKNOWN_LOCATION);

 /* Set to 0 at beginning of a function definition, set to 1 if
a return statement that specifies a return value is seen.  */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 636e0bb..d27ace2 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -3343,6 +3343,10 @@ convert_arguments (location_t loc, veclocation_t arg_loc, tree typelist,
 	  error (invalid_func_diag);
 	  return -1;
 	}
+  else if (TREE_CODE (val) == ADDR_EXPR  c_reject_gcc_builtin (val))
+	{
+	  return -1;
+	}
   else
 	/* Convert `short' and `char' to full-size `int'.  */
 	parmval = default_conversion (val);
@@ -3380,12 +3384,20 @@ parser_build_unary_op (location_t loc, enum tree_code code, struct c_expr arg)
 {
   struct c_expr result;

-  result.value = build_unary_op (loc, code, arg.value, 0);
   result.original_code = code;
   result.original_type = NULL;

+  if (c_reject_gcc_builtin (arg.value))
+{
+  result.value = error_mark_node;
+}
+  else
+{
+  result.value = build_unary_op (loc, code, arg.value, 0);
+
   if (TREE_OVERFLOW_P (result.value)  !TREE_OVERFLOW_P (arg.value))
 	overflow_warning (loc, result.value);
+}

   return result;
 }
@@ -4486,6

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-07-02 Thread Joseph Myers 
On Sun, 28 Jun 2015, Martin Sebor wrote:

 2015-06-28  Martin Sebor  mse...@redhat.com
 
   pr c/66516
   * c-tree.h (c_validate_addressable): New function.
   * c-typeck.c (convert_arguments, parser_build_unary_op): Call it.
   (build_conditional_expr, c_cast_expr, convert_for_assignment): Same.
   (build_binary_op, c_objc_common_truthvalue_conversion): Same.
   (c_validate_addressable): Define function.

I don't think c_validate_addressable is a good name - given that it's 
called for lots of things that aren't addressable, in contexts in which 
there is no need for them to be addressable, and doesn't do checks of 
addressability in contexts where they are actually needed and done 
elsewhere (e.g. checks for not taking the address of a register variable).  
The question seems to be something more like: is the expression used as an 
operand something it's OK to use as an operand at all?

What is the logic for the list of functions above being a complete list of 
the places that need changes?

 @@ -4477,11 +4486,22 @@ build_conditional_expr (location_t colon_loc, tree 
 ifexp, bool ifexp_bcp,
|| TREE_CODE (TREE_TYPE (op2)) == ERROR_MARK)
  return error_mark_node;
 
 +  if (TREE_CODE (TREE_TYPE (ifexp)) == POINTER_TYPE
 +   !c_validate_addressable (ifexp,
 +   EXPR_LOCATION (TREE_OPERAND (ifexp, 0
 +return error_mark_node;

How can ifexp be of pointer type?  It's undergone truthvalue conversion 
and should always be of type int at this point (but in any case, you can't 
refer to TREE_OPERAND (ifexp, 0) without knowing what sort of expression 
it is).

 +/* For EXPR that is an ADDR_EXPR or whose type is a FUNCTION_TYPE,
 +   determines whether its operand can have its address taken issues
 +   an error pointing to the location LOC.
 +   Operands that cannot have their address taken are builtin functions
 +   that have no library fallback (no other kinds of expressions are
 +   considered).
 +   Returns true when the expression can have its address taken and
 +   false otherwise.  */

Apart from the naming issue, the comment says nothing about the semantics 
of the function for an argument that's not of that form.

 +  error_at (loc, builtin functions must be directly called);

built-in (see codingconventions.html).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-06-28 Thread Martin Sebor 

Attached is a rewrite of the patch to enforce that GCC builtin
functions with no library equivalents are only used to make
calls or cast to void (or in sizeof and _Alignof expressions
as a GCC extension). This version of the patch also addresses
the requests made in responses to the first patch.

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

Martin
2015-06-28  Martin Sebor  mse...@redhat.com

	pr c/66516
	* tree.h (DECL_IS_GCC_BUILTIN): New macro.
	* doc/extend.texi (Other Builtins): Document when the address of
	a builtin function can be taken.

2015-06-28  Martin Sebor  mse...@redhat.com

	pr c/66516
	* c-tree.h (c_validate_addressable): New function.
	* c-typeck.c (convert_arguments, parser_build_unary_op): Call it.
	(build_conditional_expr, c_cast_expr, convert_for_assignment): Same.
	(build_binary_op, c_objc_common_truthvalue_conversion): Same.
	(c_validate_addressable): Define function.

2015-06-28  Martin Sebor  mse...@redhat.com

	pr c/66516
	* call.c (build_conditional_expr_1): Call c_validate_addressable.
	(convert_arg_to_ellipsis, convert_for_arg_passing): Same.
	* cp-tree.h (cp_validate_addressable): New function.
	* pt.c (convert_template_argument): Call it.
	* typeck.c (cp_build_binary_op, cp_build_addr_expr_strict): Same.
	(cp_build_unary_op, build_static_cast_1, build_reinterpret_cast_1):
	Same.
	(cp_build_c_cast, convert_for_assignment, convert_for_initialization):
	Same.
	(cp_validate_addressable): Define function.

2015-06-28  Martin Sebor  mse...@redhat.com

	pr c/66516
	* g++.dg/addr_builtin-1.C: New test.
	* gcc.dg/addr_builtin-1.c: New test.
	* gcc.dg/lto/pr54702_1.c: Add a missing include directive.

diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index 28b58c6..4219129 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -655,6 +655,7 @@ extern tree c_finish_transaction (location_t, tree, int);
 extern bool c_tree_equal (tree, tree);
 extern tree c_build_function_call_vec (location_t, veclocation_t, tree,
    vectree, va_gc *, vectree, va_gc *);
+extern bool c_validate_addressable (const_tree, location_t = UNKNOWN_LOCATION);

 /* Set to 0 at beginning of a function definition, set to 1 if
a return statement that specifies a return value is seen.  */
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 8e2696a..5fd3669 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -3340,6 +3340,10 @@ convert_arguments (location_t loc, veclocation_t arg_loc, tree typelist,
 	  error (invalid_func_diag);
 	  return -1;
 	}
+  else if (TREE_CODE (val) == ADDR_EXPR  !c_validate_addressable (val))
+	{
+	  return -1;
+	}
   else
 	/* Convert `short' and `char' to full-size `int'.  */
 	parmval = default_conversion (val);
@@ -3376,13 +3380,18 @@ struct c_expr
 parser_build_unary_op (location_t loc, enum tree_code code, struct c_expr arg)
 {
   struct c_expr result;
-
-  result.value = build_unary_op (loc, code, arg.value, 0);
   result.original_code = code;
   result.original_type = NULL;

+  if (c_validate_addressable (arg.value))
+{
+  result.value = build_unary_op (loc, code, arg.value, 0);
+
   if (TREE_OVERFLOW_P (result.value)  !TREE_OVERFLOW_P (arg.value))
 	overflow_warning (loc, result.value);
+}
+  else
+  result.value = error_mark_node;

   return result;
 }
@@ -4477,11 +4486,22 @@ build_conditional_expr (location_t colon_loc, tree ifexp, bool ifexp_bcp,
   || TREE_CODE (TREE_TYPE (op2)) == ERROR_MARK)
 return error_mark_node;

+  if (TREE_CODE (TREE_TYPE (ifexp)) == POINTER_TYPE
+   !c_validate_addressable (ifexp,
+  EXPR_LOCATION (TREE_OPERAND (ifexp, 0
+return error_mark_node;
+
   type1 = TREE_TYPE (op1);
   code1 = TREE_CODE (type1);
   type2 = TREE_TYPE (op2);
   code2 = TREE_CODE (type2);

+  if (code1 == POINTER_TYPE  !c_validate_addressable (op1))
+return error_mark_node;
+
+  if (code2 == POINTER_TYPE  !c_validate_addressable (op2))
+return error_mark_node;
+
   /* C90 does not permit non-lvalue arrays in conditional expressions.
  In C99 they will be pointers by now.  */
   if (code1 == ARRAY_TYPE || code2 == ARRAY_TYPE)
@@ -5220,6 +5240,10 @@ c_cast_expr (location_t loc, struct c_type_name *type_name, tree expr)
   type = groktypename (type_name, type_expr, type_expr_const);
   warn_strict_prototypes = saved_wsp;

+  if (TREE_CODE (expr) == ADDR_EXPR  !VOID_TYPE_P (type)
+   !c_validate_addressable (expr))
+return error_mark_node;
+
   ret = build_c_cast (loc, type, expr);
   if (type_expr)
 {
@@ -5859,6 +5883,10 @@ convert_for_assignment (location_t location, location_t expr_loc, tree type,
   rhs = require_complete_type (rhs);
   if (rhs == error_mark_node)
 return error_mark_node;
+
+  if (coder == POINTER_TYPE  !c_validate_addressable (rhs))
+return error_mark_node;
+
   /* A non-reference type can convert to a reference.  This handles
  va_start, va_copy and possibly port built-ins.  */
   if (codel == REFERENCE_TYPE  coder != REFERENCE_TYPE)
@@ -10336,6

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-06-23 Thread Martin Sebor 

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

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-06-23 Thread Marek Polacek 
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

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-06-23 Thread Jakub Jelinek 
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

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-06-22 Thread Marek Polacek 
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

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-06-22 Thread Martin Sebor 

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

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-06-22 Thread Marek Polacek 
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  mse...@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

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin function

2015-06-22 Thread Joseph Myers 
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).

-- 
Joseph S. Myers
jos...@codesourcery.com

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

2015-06-21 Thread Martin Sebor 

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;
  ^
2015-06-21  Martin Sebor  mse...@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,