| Message ID | 20170102195403.GK21933@tucnak |
|---|---|
| State | New |
| Headers | show |
On 01/02/2017 12:54 PM, Jakub Jelinek wrote: > Hi! > > As my earlier patch to adjust cfns.gperf for C99/C11 builtins has been > committed already, the non-__*_chk builtins (and when using __builtin__*_chk > form even those) should be all noexcept, and when using fortification > the glibc headers declare those properly throw() and ssp includes just > use builtins directly. Therefore I think it is just a waste of time to > add code to handle something that will not really happen in real-world code, > only when somebody declares those incorrectly by hand. > > The following patch therefore just punts on stmt_ends_bb_p (info.callstmt) > rather than purging dead edges and scheduling cfg cleanups on replacement > of call with constant if the compiler thinks it might throw, or when > inserting after the call (also invalid, would need to be inserted on > fallthrough edge instead). Thanks for taking care of this bug! Punting on this case sounds fine. I was not familiar with the stmt_ends_bb_p() function or how it relates to exceptions so I looked it up. It looks to be a wrapper around is_ctrl_stmt() and is_ctrl_altering_stmt(). Of the two, it looks to me like the first one will always return false here because a function call is not a control statement, and the second will end up returning the result of g gimple_call_ctrl_altering_p() because the argument is a GIMPLE_CALL. Would it be appropriate to call gimple_call_ctrl_altering_p() here directly instead? (I ask mainly because to me, unlike the former, the latter clearly conveys that it's being called to determine whether the function call may alter control flow [such as by throwing].) Martin > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2017-01-02 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/78901 > * gimple-ssa-sprintf.c (try_substitute_return_value): Don't change > possibly throwing calls. > > * g++.dg/opt/pr78901.C: New test. > > --- gcc/gimple-ssa-sprintf.c.jj 2017-01-02 12:09:29.000000000 +0100 > +++ gcc/gimple-ssa-sprintf.c 2017-01-02 13:05:08.400755201 +0100 > @@ -2699,9 +2699,15 @@ try_substitute_return_value (gimple_stmt > the output overflows the destination object (but leave it enabled > when the function is bounded because then the behavior is well- > defined). */ > - if (lhs && res.bounded && res.under4k > + if (lhs > + && res.bounded > + && res.under4k > && (info.bounded || res.number_chars <= info.objsize) > - && res.number_chars - 1 <= target_int_max ()) > + && res.number_chars - 1 <= target_int_max () > + /* Not prepared to handle possibly throwing calls here; they shouldn't > + appear in non-artificial testcases, except when the __*_chk routines > + are badly declared. */ > + && !stmt_ends_bb_p (info.callstmt)) > { > tree cst = build_int_cst (integer_type_node, res.number_chars - 1); > > --- gcc/testsuite/g++.dg/opt/pr78901.C.jj 2017-01-02 13:04:24.271329153 +0100 > +++ gcc/testsuite/g++.dg/opt/pr78901.C 2017-01-02 13:03:19.000000000 +0100 > @@ -0,0 +1,18 @@ > +// PR middle-end/78901 > +// { dg-do compile } > +// { dg-options "-O2 -Wno-stringop-overflow" } > + > +extern "C" int __snprintf_chk (char *, __SIZE_TYPE__, int, __SIZE_TYPE__, const char *, ...); > + > +int > +foo (char *c) > +{ > + try > + { > + return __snprintf_chk (c, 64, 0, 32, "%s", "abcdefghijklmnopq"); > + } > + catch (...) > + { > + return -1; > + } > +} > > Jakub >
On Mon, Jan 02, 2017 at 04:38:03PM -0700, Martin Sebor wrote: > Thanks for taking care of this bug! Punting on this case sounds > fine. > > I was not familiar with the stmt_ends_bb_p() function or how it > relates to exceptions so I looked it up. It looks to be a wrapper > around is_ctrl_stmt() and is_ctrl_altering_stmt(). Of the two, it > looks to me like the first one will always return false here because > a function call is not a control statement, and the second will end > up returning the result of g gimple_call_ctrl_altering_p() because > the argument is a GIMPLE_CALL. Would it be appropriate to call > gimple_call_ctrl_altering_p() here directly instead? (I ask mainly > because to me, unlike the former, the latter clearly conveys that > it's being called to determine whether the function call may alter > control flow [such as by throwing].) stmt_ends_bb_p is the general predicate for whether stmt has to end a basic block (and in that case e.g. it is invalid to gsi_insert_after (gsi, g, GSI_NEW_STMT); when gsi_stmt (*gsi) == stmt), so for the !info.nowrite case it is the right predicate, because it tests the property we want to avoid. For the info.nowrite case we care only whether the stmt throws, i.e. if lookup_stmt_eh_lp (stmt) != 0, but as the stmt_ends_bb_p predicate covers all those cases and no others, it is IMO fine too. Jakub
On January 2, 2017 8:54:03 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote: >Hi! > >As my earlier patch to adjust cfns.gperf for C99/C11 builtins has been >committed already, the non-__*_chk builtins (and when using >__builtin__*_chk >form even those) should be all noexcept, and when using fortification >the glibc headers declare those properly throw() and ssp includes just >use builtins directly. Therefore I think it is just a waste of time to >add code to handle something that will not really happen in real-world >code, >only when somebody declares those incorrectly by hand. > >The following patch therefore just punts on stmt_ends_bb_p >(info.callstmt) >rather than purging dead edges and scheduling cfg cleanups on >replacement >of call with constant if the compiler thinks it might throw, or when >inserting after the call (also invalid, would need to be inserted on >fallthrough edge instead). > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. Richard. >2017-01-02 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/78901 > * gimple-ssa-sprintf.c (try_substitute_return_value): Don't change > possibly throwing calls. > > * g++.dg/opt/pr78901.C: New test. > >--- gcc/gimple-ssa-sprintf.c.jj 2017-01-02 12:09:29.000000000 +0100 >+++ gcc/gimple-ssa-sprintf.c 2017-01-02 13:05:08.400755201 +0100 >@@ -2699,9 +2699,15 @@ try_substitute_return_value (gimple_stmt > the output overflows the destination object (but leave it enabled > when the function is bounded because then the behavior is well- > defined). */ >- if (lhs && res.bounded && res.under4k >+ if (lhs >+ && res.bounded >+ && res.under4k > && (info.bounded || res.number_chars <= info.objsize) >- && res.number_chars - 1 <= target_int_max ()) >+ && res.number_chars - 1 <= target_int_max () >+ /* Not prepared to handle possibly throwing calls here; they >shouldn't >+ appear in non-artificial testcases, except when the __*_chk routines >+ are badly declared. */ >+ && !stmt_ends_bb_p (info.callstmt)) > { > tree cst = build_int_cst (integer_type_node, res.number_chars - 1); > >--- gcc/testsuite/g++.dg/opt/pr78901.C.jj 2017-01-02 13:04:24.271329153 >+0100 >+++ gcc/testsuite/g++.dg/opt/pr78901.C 2017-01-02 13:03:19.000000000 >+0100 >@@ -0,0 +1,18 @@ >+// PR middle-end/78901 >+// { dg-do compile } >+// { dg-options "-O2 -Wno-stringop-overflow" } >+ >+extern "C" int __snprintf_chk (char *, __SIZE_TYPE__, int, >__SIZE_TYPE__, const char *, ...); >+ >+int >+foo (char *c) >+{ >+ try >+ { >+ return __snprintf_chk (c, 64, 0, 32, "%s", "abcdefghijklmnopq"); >+ } >+ catch (...) >+ { >+ return -1; >+ } >+} > > Jakub
--- gcc/gimple-ssa-sprintf.c.jj 2017-01-02 12:09:29.000000000 +0100 +++ gcc/gimple-ssa-sprintf.c 2017-01-02 13:05:08.400755201 +0100 @@ -2699,9 +2699,15 @@ try_substitute_return_value (gimple_stmt the output overflows the destination object (but leave it enabled when the function is bounded because then the behavior is well- defined). */ - if (lhs && res.bounded && res.under4k + if (lhs + && res.bounded + && res.under4k && (info.bounded || res.number_chars <= info.objsize) - && res.number_chars - 1 <= target_int_max ()) + && res.number_chars - 1 <= target_int_max () + /* Not prepared to handle possibly throwing calls here; they shouldn't + appear in non-artificial testcases, except when the __*_chk routines + are badly declared. */ + && !stmt_ends_bb_p (info.callstmt)) { tree cst = build_int_cst (integer_type_node, res.number_chars - 1); --- gcc/testsuite/g++.dg/opt/pr78901.C.jj 2017-01-02 13:04:24.271329153 +0100 +++ gcc/testsuite/g++.dg/opt/pr78901.C 2017-01-02 13:03:19.000000000 +0100 @@ -0,0 +1,18 @@ +// PR middle-end/78901 +// { dg-do compile } +// { dg-options "-O2 -Wno-stringop-overflow" } + +extern "C" int __snprintf_chk (char *, __SIZE_TYPE__, int, __SIZE_TYPE__, const char *, ...); + +int +foo (char *c) +{ + try + { + return __snprintf_chk (c, 64, 0, 32, "%s", "abcdefghijklmnopq"); + } + catch (...) + { + return -1; + } +}