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 > 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
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
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 SeborPR 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
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
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
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
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 SeborPR 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
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
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
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
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
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
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
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
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
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
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
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
[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
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
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
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
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
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
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
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
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
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
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
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,