| Message ID | 20170102193634.GJ21933@tucnak |
|---|---|
| State | New |
| Headers | show |
On 01/02/2017 12:36 PM, Jakub Jelinek wrote: > Hi! > > Even when we know the exact return value of a snprintf call with 0 length > (i.e. one that doesn't write anything into a buffer), there can be %n > directives in the format string that have side-effects that shouldn't be > discarded. > > The following patch avoids the replacement of the snprintf call > with just the constant if there is any %n directive. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Thanks. The fix looks good to me. I would suggest to also update the comment above the nowrite member to say something like this: /* True for bounded functions like snprintf that specify a zero-size buffer as a request to compute the size of output without actually writing any. NOWRITE is cleared in response to the %n directive which has side-effects similar to writing output. */ Martin > > 2017-01-02 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/78965 > * gimple-ssa-sprintf.c (pass_sprintf_length::compute_format_length): > Change first argument from const call_info & to call_info &. For %n > set info.nowrite to false. > > * gcc.dg/pr78965.c: New test. > > --- gcc/gimple-ssa-sprintf.c.jj 2017-01-01 12:45:35.000000000 +0100 > +++ gcc/gimple-ssa-sprintf.c 2017-01-02 12:09:29.902945255 +0100 > @@ -131,7 +131,7 @@ public: > void handle_gimple_call (gimple_stmt_iterator*); > > struct call_info; > - bool compute_format_length (const call_info &, format_result *); > + bool compute_format_length (call_info &, format_result *); > }; > > bool > @@ -2357,7 +2357,7 @@ add_bytes (const pass_sprintf_length::ca > that caused the processing to be terminated early). */ > > bool > -pass_sprintf_length::compute_format_length (const call_info &info, > +pass_sprintf_length::compute_format_length (call_info &info, > format_result *res) > { > /* The variadic argument counter. */ > @@ -2624,6 +2624,9 @@ pass_sprintf_length::compute_format_leng > return false; > > case 'n': > + /* %n has side-effects even when nothing is actually printed to > + any buffer. */ > + info.nowrite = false; > break; > > case 'c': > --- gcc/testsuite/gcc.dg/pr78965.c.jj 2017-01-02 12:12:15.959803429 +0100 > +++ gcc/testsuite/gcc.dg/pr78965.c 2017-01-02 12:11:06.000000000 +0100 > @@ -0,0 +1,14 @@ > +/* PR tree-optimization/78965 */ > +/* { dg-do run { target c99_runtime } } */ > +/* { dg-options "-O2" } */ > +/* { dg-add-options c99_runtime } */ > + > +int > +main () > +{ > + int a = 5, b = 6; > + int c = __builtin_snprintf (0, 0, "a%nb%nc", &a, &b); > + if (a + b + c != 6) > + __builtin_abort (); > + return 0; > +} > > Jakub >
On Mon, Jan 02, 2017 at 03:51:14PM -0700, Martin Sebor wrote: > On 01/02/2017 12:36 PM, Jakub Jelinek wrote: > > Hi! > > > > Even when we know the exact return value of a snprintf call with 0 length > > (i.e. one that doesn't write anything into a buffer), there can be %n > > directives in the format string that have side-effects that shouldn't be > > discarded. > > > > The following patch avoids the replacement of the snprintf call > > with just the constant if there is any %n directive. > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > Thanks. The fix looks good to me. I would suggest to also update > the comment above the nowrite member to say something like this: > > /* True for bounded functions like snprintf that specify a zero-size > buffer as a request to compute the size of output without actually > writing any. NOWRITE is cleared in response to the %n directive > which has side-effects similar to writing output. */ LGTM, thanks. I'll adjust the comment if it is approved. > > 2017-01-02 Jakub Jelinek <jakub@redhat.com> > > > > PR tree-optimization/78965 > > * gimple-ssa-sprintf.c (pass_sprintf_length::compute_format_length): > > Change first argument from const call_info & to call_info &. For %n > > set info.nowrite to false. > > > > * gcc.dg/pr78965.c: New test. Jakub
On January 2, 2017 8:36:34 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote: >Hi! > >Even when we know the exact return value of a snprintf call with 0 >length >(i.e. one that doesn't write anything into a buffer), there can be %n >directives in the format string that have side-effects that shouldn't >be >discarded. > >The following patch avoids the replacement of the snprintf call >with just the constant if there is any %n directive. > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. Richard. >2017-01-02 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/78965 > * gimple-ssa-sprintf.c (pass_sprintf_length::compute_format_length): > Change first argument from const call_info & to call_info &. For %n > set info.nowrite to false. > > * gcc.dg/pr78965.c: New test. > >--- gcc/gimple-ssa-sprintf.c.jj 2017-01-01 12:45:35.000000000 +0100 >+++ gcc/gimple-ssa-sprintf.c 2017-01-02 12:09:29.902945255 +0100 >@@ -131,7 +131,7 @@ public: > void handle_gimple_call (gimple_stmt_iterator*); > > struct call_info; >- bool compute_format_length (const call_info &, format_result *); >+ bool compute_format_length (call_info &, format_result *); > }; > > bool >@@ -2357,7 +2357,7 @@ add_bytes (const pass_sprintf_length::ca > that caused the processing to be terminated early). */ > > bool >-pass_sprintf_length::compute_format_length (const call_info &info, >+pass_sprintf_length::compute_format_length (call_info &info, > format_result *res) > { > /* The variadic argument counter. */ >@@ -2624,6 +2624,9 @@ pass_sprintf_length::compute_format_leng > return false; > > case 'n': >+ /* %n has side-effects even when nothing is actually printed to >+ any buffer. */ >+ info.nowrite = false; > break; > > case 'c': >--- gcc/testsuite/gcc.dg/pr78965.c.jj 2017-01-02 12:12:15.959803429 >+0100 >+++ gcc/testsuite/gcc.dg/pr78965.c 2017-01-02 12:11:06.000000000 +0100 >@@ -0,0 +1,14 @@ >+/* PR tree-optimization/78965 */ >+/* { dg-do run { target c99_runtime } } */ >+/* { dg-options "-O2" } */ >+/* { dg-add-options c99_runtime } */ >+ >+int >+main () >+{ >+ int a = 5, b = 6; >+ int c = __builtin_snprintf (0, 0, "a%nb%nc", &a, &b); >+ if (a + b + c != 6) >+ __builtin_abort (); >+ return 0; >+} > > Jakub
--- gcc/gimple-ssa-sprintf.c.jj 2017-01-01 12:45:35.000000000 +0100 +++ gcc/gimple-ssa-sprintf.c 2017-01-02 12:09:29.902945255 +0100 @@ -131,7 +131,7 @@ public: void handle_gimple_call (gimple_stmt_iterator*); struct call_info; - bool compute_format_length (const call_info &, format_result *); + bool compute_format_length (call_info &, format_result *); }; bool @@ -2357,7 +2357,7 @@ add_bytes (const pass_sprintf_length::ca that caused the processing to be terminated early). */ bool -pass_sprintf_length::compute_format_length (const call_info &info, +pass_sprintf_length::compute_format_length (call_info &info, format_result *res) { /* The variadic argument counter. */ @@ -2624,6 +2624,9 @@ pass_sprintf_length::compute_format_leng return false; case 'n': + /* %n has side-effects even when nothing is actually printed to + any buffer. */ + info.nowrite = false; break; case 'c': --- gcc/testsuite/gcc.dg/pr78965.c.jj 2017-01-02 12:12:15.959803429 +0100 +++ gcc/testsuite/gcc.dg/pr78965.c 2017-01-02 12:11:06.000000000 +0100 @@ -0,0 +1,14 @@ +/* PR tree-optimization/78965 */ +/* { dg-do run { target c99_runtime } } */ +/* { dg-options "-O2" } */ +/* { dg-add-options c99_runtime } */ + +int +main () +{ + int a = 5, b = 6; + int c = __builtin_snprintf (0, 0, "a%nb%nc", &a, &b); + if (a + b + c != 6) + __builtin_abort (); + return 0; +}