Message ID | 20160422170431.GB13517@virgil.suse.cz |
---|---|
State | New |
Headers | show |
On April 22, 2016 7:04:31 PM GMT+02:00, Martin Jambor <mjambor@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)? Richard. >Thanks, > >Martin > > >2016-04-20 Martin Jambor <mjambor@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
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 <mjambor@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. 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 <mjambor@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 > >
On Fri, Apr 22, 2016 at 9:40 PM, Martin Jambor <mjambor@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 <mjambor@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 <mjambor@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 >> >>
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