On Fri, Apr 22, 2016 at 9:40 PM, Martin Jambor <mjam...@suse.cz> wrote:
> Hi,
>
> On Fri, Apr 22, 2016 at 09:24:31PM +0200, Richard Biener wrote:
>> On April 22, 2016 7:04:31 PM GMT+02:00, Martin Jambor <mjam...@suse.cz>
>> wrote:
>> >Hi,
>> >
>> >this patch adds verification that __builtin_unreachable and
>> >__builtin_trap are not called with arguments. The problem with calls
>> >to them with arguments is that functions like gimple_call_builtin_p
>> >return false on them, because they return true only when
>> >gimple_builtin_call_types_compatible_p does. One manifestation of
>> >that was PR 61591 where undefined behavior sanitizer did not replace
>> >such calls with its thing as it should, but there might be others.
>> >
>> >I have included __builtin_trap in the verification because they often
>> >seem to be handled together but can either remove it or add more
>> >builtins if people think it better. I concede it is a bit arbitrary.
>> >
>> >Honza said he has seen __builtin_unreachable calls with parameters in
>> >LTO builds of Firefox, so it seems this might actually trigger, but I
>> >also think we do not want such calls in the IL.
>> >
>> >I have bootstrapped and tested this on x86_64-linux (with all
>> >languages and Ada) and have also run a C, C++ and Fortran LTO
>> >bootstrap with the patch on the same architecture. OK for trunk?
>>
>> Shouldn't we simply error in the FEs for this given the builtins
>> essentially have a prototype? That is, error for non-matching args
>> for the __built-in_ variant of _all_ builtins (treat them as
>> prototyped)?
>>
>
> We do that. It is just that at times we produce a call to
> __builtin_unreachable internally. The only instance I know of is IPA
> figuring out a call cannot happen in a legal program (for example
> because it would lead to a call of abstract virtual functions) but
> perhaps there are other places where we do it.
Ah, I see...
> I thought we have fixed the issue of IPA leaving behind arguments in
> the calls to __builtin_unreachable it produced and this verification
> would simply made sure the bug does not come back but Honza's
> observation suggests that it still sometimes happens.
... so the patch is ok if you put a comment before it resembling the above.
Thanks,
Richard.
> Martin
>
>> Richard.
>>
>> >Thanks,
>> >
>> >Martin
>> >
>> >
>> >2016-04-20 Martin Jambor <mjam...@suse.cz>
>> >
>> > * tree-cfg.c (verify_gimple_call): Check that calls to
>> > __builtin_unreachable or __builtin_trap do not have actual arguments.
>> >---
>> > gcc/tree-cfg.c | 20 ++++++++++++++++++++
>> > 1 file changed, 20 insertions(+)
>> >
>> >diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
>> >index 04e46fd..3385164 100644
>> >--- a/gcc/tree-cfg.c
>> >+++ b/gcc/tree-cfg.c
>> >@@ -3414,6 +3414,26 @@ verify_gimple_call (gcall *stmt)
>> > return true;
>> > }
>> >
>> >+ if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
>> >+ {
>> >+ switch (DECL_FUNCTION_CODE (fndecl))
>> >+ {
>> >+ case BUILT_IN_UNREACHABLE:
>> >+ case BUILT_IN_TRAP:
>> >+ if (gimple_call_num_args (stmt) > 0)
>> >+ {
>> >+ /* Built-in unreachable with parameters might not be caught by
>> >+ undefined behavior santizer. */
>> >+ error ("__builtin_unreachable or __builtin_trap call with "
>> >+ "arguments");
>> >+ return true;
>> >+ }
>> >+ break;
>> >+ default:
>> >+ break;
>> >+ }
>> >+ }
>> >+
>> > /* ??? The C frontend passes unpromoted arguments in case it
>> > didn't see a function declaration before the call. So for now
>> > leave the call arguments mostly unverified. Once we gimplify
>>
>>
