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
