Re: [PATCH] Verify __builtin_unreachable and __builtin_trap are not called with arguments

[Richard Biener]

On Fri, Apr 22, 2016 at 9:40 PM, Martin Jambor  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  
>> 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  
>> >
>> > * 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
>>
>>

Re: [PATCH] Verify __builtin_unreachable and __builtin_trap are not called with arguments

[Martin Jambor]

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

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.

Martin

> Richard.
> 
> >Thanks,
> >
> >Martin
> >
> >
> >2016-04-20  Martin Jambor  
> >
> > * 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
> 
> 

Re: [PATCH] Verify __builtin_unreachable and __builtin_trap are not called with arguments

[Richard Biener]

On April 22, 2016 7:04:31 PM GMT+02:00, Martin Jambor  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)?

Richard.

>Thanks,
>
>Martin
>
>
>2016-04-20  Martin Jambor  
>
>   * 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

[PATCH] Verify __builtin_unreachable and __builtin_trap are not called with arguments

[Martin Jambor]

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?

Thanks,

Martin


2016-04-20  Martin Jambor  

* 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
-- 
2.8.1