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

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

The C++ parts are OK. Jason

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

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

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 >

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

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?

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

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

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

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. >

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[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

Re: [PATCH] c/66516 - missing diagnostic on taking the address of a builtin 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.

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

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,

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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