[PR,middle-end/85598] make -Wprintf* pass use loop info for PHI's
diff mbox series

Message ID d84f438f-1966-3a1b-efa5-cbcb7b103381@redhat.com
State New
Headers show
Series
  • [PR,middle-end/85598] make -Wprintf* pass use loop info for PHI's
Related show

Commit Message

Aldy Hernandez Jan. 31, 2019, 7:39 a.m. UTC
Hi folks.

The problem here is that Wprintf* uses the evrp_range_analyzer engine, 
and doesn't understand that the x_5 != 256 conditional below should make 
the RANGE [0, 255], not [0, 256].

   <bb 3> [local count: 1063004407]:
   # RANGE [0, 256] NONZERO 511
   # x_10 = PHI <0(2), x_5(3)>
   snprintf (&temp, 4, "%%%02X", x_10);
   # RANGE [1, 256] NONZERO 511
   x_5 = x_10 + 1;
   if (x_5 != 256)
     goto <bb 3>; [98.99%]
   else
     goto <bb 4>; [1.01%]

This PR is handled quite easily in the ranger work, but alas there is 
nothing for this release.  Therefore, I'd like to dedicate as little 
time as possible to this PR this time around.

Various possible solutions are discussed in the PR.  I think an easy way 
is to lean on loop analysis to refine the PHI result.  It turns out the 
evrp engine already does this if SCEV data is available.

As Richard mentioned in the PR, we should avoid code differences 
dependent on -W* flags.  I have put the loop init code in the pass 
itself, but am open to moving it outside, perhaps early in the gate 
function ??.

I also took the liberty of calling loop_optimizer_init() with the 
smallest subset of LOOPS_NORMAL which could still fix the problem 
(LOOPS_HAVE_PREHEADERS).  This avoids unnecessary changes to the CFG. 
But really, I'm just guessing here.  LOOPS_NORMAL would also work, 
albeit creating forwarder blocks.

Tested on x86-64 Linux.

What do you think?

Aldy

Comments

Richard Biener Jan. 31, 2019, 9:52 a.m. UTC | #1
On Thu, Jan 31, 2019 at 8:40 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> Hi folks.
>
> The problem here is that Wprintf* uses the evrp_range_analyzer engine,
> and doesn't understand that the x_5 != 256 conditional below should make
> the RANGE [0, 255], not [0, 256].
>
>    <bb 3> [local count: 1063004407]:
>    # RANGE [0, 256] NONZERO 511
>    # x_10 = PHI <0(2), x_5(3)>
>    snprintf (&temp, 4, "%%%02X", x_10);
>    # RANGE [1, 256] NONZERO 511
>    x_5 = x_10 + 1;
>    if (x_5 != 256)
>      goto <bb 3>; [98.99%]
>    else
>      goto <bb 4>; [1.01%]
>
> This PR is handled quite easily in the ranger work, but alas there is
> nothing for this release.  Therefore, I'd like to dedicate as little
> time as possible to this PR this time around.
>
> Various possible solutions are discussed in the PR.  I think an easy way
> is to lean on loop analysis to refine the PHI result.  It turns out the
> evrp engine already does this if SCEV data is available.
>
> As Richard mentioned in the PR, we should avoid code differences
> dependent on -W* flags.  I have put the loop init code in the pass
> itself, but am open to moving it outside, perhaps early in the gate
> function ??.
>
> I also took the liberty of calling loop_optimizer_init() with the
> smallest subset of LOOPS_NORMAL which could still fix the problem
> (LOOPS_HAVE_PREHEADERS).  This avoids unnecessary changes to the CFG.
> But really, I'm just guessing here.  LOOPS_NORMAL would also work,
> albeit creating forwarder blocks.
>
> Tested on x86-64 Linux.
>
> What do you think?

As far as I understand fold_return_value is true/false independent
of -W* flags (but dependent on -fprintf-return-value or so).  This means
that your patch avoids the risk of CFG changes dependent on
-W* flags.  This means you could safely use LOOPS_NORMAL
(I'm not sure SCEV is happy with just pre-headers, unfortunately
its exact requirements are not documented... - at least I see
unconditional dereferences of loop->latch which rules out
LOOPS_NO_CFG_MANIPULATION, likewise loop_preheader_edge
is mentioned).

That said, LOOPS_HAVE_PREHEADERS probably works well
enough for both SCEV and niter analysis.

Thus, the patch is OK if you adjust the comment a bit, it's
perfectly safe to init loops conditional on optimization and
running in the gate isn't wanted.  I'm also not sure why
this should go away with on-demand info -- working in
the EVRP DOM walker exactly provides on-demand info
(just the DOM style one relies on SCEV / niter analysis
for any backedge work).

Richard.

>
> Aldy
Aldy Hernandez Jan. 31, 2019, 2:30 p.m. UTC | #2
On 1/31/19 4:52 AM, Richard Biener wrote:
> On Thu, Jan 31, 2019 at 8:40 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>> Hi folks.
>>
>> The problem here is that Wprintf* uses the evrp_range_analyzer engine,
>> and doesn't understand that the x_5 != 256 conditional below should make
>> the RANGE [0, 255], not [0, 256].
>>
>>     <bb 3> [local count: 1063004407]:
>>     # RANGE [0, 256] NONZERO 511
>>     # x_10 = PHI <0(2), x_5(3)>
>>     snprintf (&temp, 4, "%%%02X", x_10);
>>     # RANGE [1, 256] NONZERO 511
>>     x_5 = x_10 + 1;
>>     if (x_5 != 256)
>>       goto <bb 3>; [98.99%]
>>     else
>>       goto <bb 4>; [1.01%]
>>
>> This PR is handled quite easily in the ranger work, but alas there is
>> nothing for this release.  Therefore, I'd like to dedicate as little
>> time as possible to this PR this time around.
>>
>> Various possible solutions are discussed in the PR.  I think an easy way
>> is to lean on loop analysis to refine the PHI result.  It turns out the
>> evrp engine already does this if SCEV data is available.
>>
>> As Richard mentioned in the PR, we should avoid code differences
>> dependent on -W* flags.  I have put the loop init code in the pass
>> itself, but am open to moving it outside, perhaps early in the gate
>> function ??.
>>
>> I also took the liberty of calling loop_optimizer_init() with the
>> smallest subset of LOOPS_NORMAL which could still fix the problem
>> (LOOPS_HAVE_PREHEADERS).  This avoids unnecessary changes to the CFG.
>> But really, I'm just guessing here.  LOOPS_NORMAL would also work,
>> albeit creating forwarder blocks.
>>
>> Tested on x86-64 Linux.
>>
>> What do you think?
> 
> As far as I understand fold_return_value is true/false independent
> of -W* flags (but dependent on -fprintf-return-value or so).  This means
> that your patch avoids the risk of CFG changes dependent on
> -W* flags.  This means you could safely use LOOPS_NORMAL

But isn't my code inside of ::execute, which is still gated by 
fold_return_value AND all the -W* flags:?

bool
pass_sprintf_length::gate (function *)
{
   return ((warn_format_overflow > 0
	   || warn_format_trunc > 0
	   || flag_printf_return_value)
	  && (optimize > 0) == fold_return_value);
}

Thus, I think we need to move the loop initialization somewhere else ??.

> (I'm not sure SCEV is happy with just pre-headers, unfortunately
> its exact requirements are not documented... - at least I see
> unconditional dereferences of loop->latch which rules out
> LOOPS_NO_CFG_MANIPULATION, likewise loop_preheader_edge
> is mentioned).
> 
> That said, LOOPS_HAVE_PREHEADERS probably works well
> enough for both SCEV and niter analysis.
> 
> Thus, the patch is OK if you adjust the comment a bit, it's
> perfectly safe to init loops conditional on optimization and
> running in the gate isn't wanted.  I'm also not sure why
> this should go away with on-demand info -- working in
> the EVRP DOM walker exactly provides on-demand info
> (just the DOM style one relies on SCEV / niter analysis
> for any backedge work).

I believe there are backedge smarts in the ranger, at least for the 
simple cases.  But I'll change the comment about the on-demand code 
curing everything, into some reminder to remove the SCEV stuff if/when 
we have a more precise range mechanism.  I'd hate for this loop 
optimization requirement to survive past our need for it.

Aldy
Jakub Jelinek Jan. 31, 2019, 2:45 p.m. UTC | #3
On Thu, Jan 31, 2019 at 09:30:13AM -0500, Aldy Hernandez wrote:
> > > (LOOPS_HAVE_PREHEADERS).  This avoids unnecessary changes to the CFG.
> > > But really, I'm just guessing here.  LOOPS_NORMAL would also work,
> > > albeit creating forwarder blocks.
> > > 
> > > Tested on x86-64 Linux.
> > > 
> > > What do you think?
> > 
> > As far as I understand fold_return_value is true/false independent
> > of -W* flags (but dependent on -fprintf-return-value or so).  This means
> > that your patch avoids the risk of CFG changes dependent on
> > -W* flags.  This means you could safely use LOOPS_NORMAL
> 
> But isn't my code inside of ::execute, which is still gated by
> fold_return_value AND all the -W* flags:?
> 
> bool
> pass_sprintf_length::gate (function *)
> {
>   return ((warn_format_overflow > 0
> 	   || warn_format_trunc > 0
> 	   || flag_printf_return_value)
> 	  && (optimize > 0) == fold_return_value);
> }
> 
> Thus, I think we need to move the loop initialization somewhere else ??.

Then perhaps turn the gate into just return (optimize > 0) == fold_return_value;
and in execute do what you're adding and guard the rest with the above
condition?  As -fprintf-return-value is on by default for C-family, it shouldn't
change much.
+ adjust comment on the gate of course.

	Jakub
Richard Biener Jan. 31, 2019, 2:56 p.m. UTC | #4
On Thu, Jan 31, 2019 at 3:45 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Thu, Jan 31, 2019 at 09:30:13AM -0500, Aldy Hernandez wrote:
> > > > (LOOPS_HAVE_PREHEADERS).  This avoids unnecessary changes to the CFG.
> > > > But really, I'm just guessing here.  LOOPS_NORMAL would also work,
> > > > albeit creating forwarder blocks.
> > > >
> > > > Tested on x86-64 Linux.
> > > >
> > > > What do you think?
> > >
> > > As far as I understand fold_return_value is true/false independent
> > > of -W* flags (but dependent on -fprintf-return-value or so).  This means
> > > that your patch avoids the risk of CFG changes dependent on
> > > -W* flags.  This means you could safely use LOOPS_NORMAL
> >
> > But isn't my code inside of ::execute, which is still gated by
> > fold_return_value AND all the -W* flags:?
> >
> > bool
> > pass_sprintf_length::gate (function *)
> > {
> >   return ((warn_format_overflow > 0
> >          || warn_format_trunc > 0
> >          || flag_printf_return_value)
> >         && (optimize > 0) == fold_return_value);
> > }
> >
> > Thus, I think we need to move the loop initialization somewhere else ??.
>
> Then perhaps turn the gate into just return (optimize > 0) == fold_return_value;
> and in execute do what you're adding and guard the rest with the above
> condition?  As -fprintf-return-value is on by default for C-family, it shouldn't
> change much.
> + adjust comment on the gate of course.

Don't you alreday have

@@ -4200,10 +4202,34 @@ pass_sprintf_length::execute (function *fun)
   init_target_to_host_charmap ();

   calculate_dominance_info (CDI_DOMINATORS);
+  bool optimizing_late = optimize > 0 && fold_return_value;
+  if (optimizing_late)
+    {
+      /* ?? We should avoid changing the CFG as much as possible.
...
+      loop_optimizer_init (LOOPS_HAVE_PREHEADERS);
+      scev_initialize ();
+    }

so loops are only initialized if fold_return_value is true?  Ah - but that's
the pass parameter from params.def rather than the flag to enable
the folding...  So indeed, change it to include && flag_printf_return_value

Richard.

>         Jakub
Jakub Jelinek Jan. 31, 2019, 3:02 p.m. UTC | #5
On Thu, Jan 31, 2019 at 03:56:12PM +0100, Richard Biener wrote:
> Don't you alreday have
> 
> @@ -4200,10 +4202,34 @@ pass_sprintf_length::execute (function *fun)
>    init_target_to_host_charmap ();
> 
>    calculate_dominance_info (CDI_DOMINATORS);
> +  bool optimizing_late = optimize > 0 && fold_return_value;
> +  if (optimizing_late)
> +    {
> +      /* ?? We should avoid changing the CFG as much as possible.
> ...
> +      loop_optimizer_init (LOOPS_HAVE_PREHEADERS);
> +      scev_initialize ();
> +    }
> 
> so loops are only initialized if fold_return_value is true?  Ah - but that's
> the pass parameter from params.def rather than the flag to enable
> the folding...  So indeed, change it to include && flag_printf_return_value

fold_return_value is not the same thing as flag_printf_return_value,
the former is just a bool whether it is the -O0 or -O1+ version of the pass.
So, optimizing_late doesn't make much sense, one can use optimize > 0
directly instead.

If changing the above to && flag_printf_return_value then people will
complain that they get the false positive warning with -Wall
-fno-printf-return-value.

	Jakub
Richard Biener Jan. 31, 2019, 3:05 p.m. UTC | #6
On Thu, Jan 31, 2019 at 4:02 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Thu, Jan 31, 2019 at 03:56:12PM +0100, Richard Biener wrote:
> > Don't you alreday have
> >
> > @@ -4200,10 +4202,34 @@ pass_sprintf_length::execute (function *fun)
> >    init_target_to_host_charmap ();
> >
> >    calculate_dominance_info (CDI_DOMINATORS);
> > +  bool optimizing_late = optimize > 0 && fold_return_value;
> > +  if (optimizing_late)
> > +    {
> > +      /* ?? We should avoid changing the CFG as much as possible.
> > ...
> > +      loop_optimizer_init (LOOPS_HAVE_PREHEADERS);
> > +      scev_initialize ();
> > +    }
> >
> > so loops are only initialized if fold_return_value is true?  Ah - but that's
> > the pass parameter from params.def rather than the flag to enable
> > the folding...  So indeed, change it to include && flag_printf_return_value
>
> fold_return_value is not the same thing as flag_printf_return_value,
> the former is just a bool whether it is the -O0 or -O1+ version of the pass.
> So, optimizing_late doesn't make much sense, one can use optimize > 0
> directly instead.
>
> If changing the above to && flag_printf_return_value then people will
> complain that they get the false positive warning with -Wall
> -fno-printf-return-value.

Sure - too bad then.  Another option would be to make SCEV / niter analysis
"safe" to be called with LOOPS_AVOID_CFG_MANIPULATIONs (multiple
latches, multiple entries into headers, etc.).

Richard.

>         Jakub
Jakub Jelinek Jan. 31, 2019, 3:36 p.m. UTC | #7
On Thu, Jan 31, 2019 at 04:05:51PM +0100, Richard Biener wrote:
> > On Thu, Jan 31, 2019 at 03:56:12PM +0100, Richard Biener wrote:
> > > Don't you alreday have
> > >
> > > @@ -4200,10 +4202,34 @@ pass_sprintf_length::execute (function *fun)
> > >    init_target_to_host_charmap ();
> > >
> > >    calculate_dominance_info (CDI_DOMINATORS);
> > > +  bool optimizing_late = optimize > 0 && fold_return_value;
> > > +  if (optimizing_late)
> > > +    {
> > > +      /* ?? We should avoid changing the CFG as much as possible.
> > > ...
> > > +      loop_optimizer_init (LOOPS_HAVE_PREHEADERS);
> > > +      scev_initialize ();
> > > +    }
> > >
> > > so loops are only initialized if fold_return_value is true?  Ah - but that's
> > > the pass parameter from params.def rather than the flag to enable
> > > the folding...  So indeed, change it to include && flag_printf_return_value
> >
> > fold_return_value is not the same thing as flag_printf_return_value,
> > the former is just a bool whether it is the -O0 or -O1+ version of the pass.
> > So, optimizing_late doesn't make much sense, one can use optimize > 0
> > directly instead.
> >
> > If changing the above to && flag_printf_return_value then people will
> > complain that they get the false positive warning with -Wall
> > -fno-printf-return-value.
> 
> Sure - too bad then.  Another option would be to make SCEV / niter analysis
> "safe" to be called with LOOPS_AVOID_CFG_MANIPULATIONs (multiple
> latches, multiple entries into headers, etc.).

But then that isn't likely going to make it into GCC9.

Do we care that much about -W* not affecting code generation when we have
many -W* options that do affect it and aren't going to be fixed in GCC9 (and
unlikely later on either - mainly the C++ caching related ones), plus
it could affect code generation only if one uses -fno-printf-return-value
explicitly?

	Jakub
Richard Biener Feb. 1, 2019, 9:05 a.m. UTC | #8
On Thu, Jan 31, 2019 at 4:36 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Thu, Jan 31, 2019 at 04:05:51PM +0100, Richard Biener wrote:
> > > On Thu, Jan 31, 2019 at 03:56:12PM +0100, Richard Biener wrote:
> > > > Don't you alreday have
> > > >
> > > > @@ -4200,10 +4202,34 @@ pass_sprintf_length::execute (function *fun)
> > > >    init_target_to_host_charmap ();
> > > >
> > > >    calculate_dominance_info (CDI_DOMINATORS);
> > > > +  bool optimizing_late = optimize > 0 && fold_return_value;
> > > > +  if (optimizing_late)
> > > > +    {
> > > > +      /* ?? We should avoid changing the CFG as much as possible.
> > > > ...
> > > > +      loop_optimizer_init (LOOPS_HAVE_PREHEADERS);
> > > > +      scev_initialize ();
> > > > +    }
> > > >
> > > > so loops are only initialized if fold_return_value is true?  Ah - but that's
> > > > the pass parameter from params.def rather than the flag to enable
> > > > the folding...  So indeed, change it to include && flag_printf_return_value
> > >
> > > fold_return_value is not the same thing as flag_printf_return_value,
> > > the former is just a bool whether it is the -O0 or -O1+ version of the pass.
> > > So, optimizing_late doesn't make much sense, one can use optimize > 0
> > > directly instead.
> > >
> > > If changing the above to && flag_printf_return_value then people will
> > > complain that they get the false positive warning with -Wall
> > > -fno-printf-return-value.
> >
> > Sure - too bad then.  Another option would be to make SCEV / niter analysis
> > "safe" to be called with LOOPS_AVOID_CFG_MANIPULATIONs (multiple
> > latches, multiple entries into headers, etc.).
>
> But then that isn't likely going to make it into GCC9.
>
> Do we care that much about -W* not affecting code generation when we have
> many -W* options that do affect it and aren't going to be fixed in GCC9 (and
> unlikely later on either - mainly the C++ caching related ones), plus
> it could affect code generation only if one uses -fno-printf-return-value
> explicitly?

We definitely shouldn't willingly add new instances.  Why do we care
about people getting false positives when they willingly disable
-fprintf-return-value?

Richard.

>         Jakub
Jakub Jelinek Feb. 1, 2019, 9:09 a.m. UTC | #9
On Fri, Feb 01, 2019 at 10:05:02AM +0100, Richard Biener wrote:
> We definitely shouldn't willingly add new instances.  Why do we care
> about people getting false positives when they willingly disable
> -fprintf-return-value?

The -fprintf-return-value switch is about code transformations, perhaps
somebody wants warnings but doesn't want the return value to be ignored or
assumed to be such and such (e.g. because his C library doesn't do exactly
what GCC is expecting it to do).

Anyway, not that important to me, if you prefer adding "&&
flag_printf_return_value" to Aldy's changes, let's go ahead with that.

	Jakub
Aldy Hernandez Feb. 22, 2019, 9:57 a.m. UTC | #10
On 1/31/19 9:56 AM, Richard Biener wrote:
> On Thu, Jan 31, 2019 at 3:45 PM Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> On Thu, Jan 31, 2019 at 09:30:13AM -0500, Aldy Hernandez wrote:
>>>>> (LOOPS_HAVE_PREHEADERS).  This avoids unnecessary changes to the CFG.
>>>>> But really, I'm just guessing here.  LOOPS_NORMAL would also work,
>>>>> albeit creating forwarder blocks.
>>>>>
>>>>> Tested on x86-64 Linux.
>>>>>
>>>>> What do you think?
>>>>
>>>> As far as I understand fold_return_value is true/false independent
>>>> of -W* flags (but dependent on -fprintf-return-value or so).  This means
>>>> that your patch avoids the risk of CFG changes dependent on
>>>> -W* flags.  This means you could safely use LOOPS_NORMAL
>>>
>>> But isn't my code inside of ::execute, which is still gated by
>>> fold_return_value AND all the -W* flags:?
>>>
>>> bool
>>> pass_sprintf_length::gate (function *)
>>> {
>>>    return ((warn_format_overflow > 0
>>>           || warn_format_trunc > 0
>>>           || flag_printf_return_value)
>>>          && (optimize > 0) == fold_return_value);
>>> }
>>>
>>> Thus, I think we need to move the loop initialization somewhere else ??.
>>
>> Then perhaps turn the gate into just return (optimize > 0) == fold_return_value;
>> and in execute do what you're adding and guard the rest with the above
>> condition?  As -fprintf-return-value is on by default for C-family, it shouldn't
>> change much.
>> + adjust comment on the gate of course.
> 
> Don't you alreday have
> 
> @@ -4200,10 +4202,34 @@ pass_sprintf_length::execute (function *fun)
>     init_target_to_host_charmap ();
> 
>     calculate_dominance_info (CDI_DOMINATORS);
> +  bool optimizing_late = optimize > 0 && fold_return_value;
> +  if (optimizing_late)
> +    {
> +      /* ?? We should avoid changing the CFG as much as possible.
> ...
> +      loop_optimizer_init (LOOPS_HAVE_PREHEADERS);
> +      scev_initialize ();
> +    }
> 
> so loops are only initialized if fold_return_value is true?  Ah - but that's
> the pass parameter from params.def rather than the flag to enable
> the folding...  So indeed, change it to include && flag_printf_return_value

I believe the flag_printf_return_value change was the final agreed upon 
solution.

Tested on x86-64 Linux.

OK for trunk?
gcc/

	PR middle-end/85598
	* gimple-ssa-sprintf.c (pass_sprintf_length::execute): Enable loop
	analysis for pass.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 8e6016fc42f..48580411eb9 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -65,6 +65,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-propagate.h"
 #include "calls.h"
 #include "cfgloop.h"
+#include "tree-scalar-evolution.h"
+#include "tree-ssa-loop.h"
 #include "intl.h"
 #include "langhooks.h"
 
@@ -4200,10 +4202,22 @@ pass_sprintf_length::execute (function *fun)
   init_target_to_host_charmap ();
 
   calculate_dominance_info (CDI_DOMINATORS);
+  bool optimizing_late = optimize > 0 && flag_printf_return_value;
+  if (optimizing_late)
+    {
+      loop_optimizer_init (LOOPS_NORMAL);
+      scev_initialize ();
+    }
 
   sprintf_dom_walker sprintf_dom_walker;
   sprintf_dom_walker.walk (ENTRY_BLOCK_PTR_FOR_FN (fun));
 
+  if (optimizing_late)
+    {
+      scev_finalize ();
+      loop_optimizer_finalize ();
+    }
+
   /* Clean up object size info.  */
   fini_object_sizes ();
   return 0;
diff --git a/gcc/testsuite/gcc.dg/pr85598.c b/gcc/testsuite/gcc.dg/pr85598.c
new file mode 100644
index 00000000000..c84b63f8084
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr85598.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+extern void __chk_fail (void);
+extern int snprintf (char *, size_t, const char *, ...);
+
+int main()
+{
+    char temp[100];
+    unsigned int x;
+    char *str = temp;
+    for(x=0; x<256; ++x) {
+      snprintf(str, 4, "%%%02X", x);
+    }
+}
Richard Biener Feb. 22, 2019, 12:24 p.m. UTC | #11
On Fri, Feb 22, 2019 at 10:57 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> On 1/31/19 9:56 AM, Richard Biener wrote:
> > On Thu, Jan 31, 2019 at 3:45 PM Jakub Jelinek <jakub@redhat.com> wrote:
> >>
> >> On Thu, Jan 31, 2019 at 09:30:13AM -0500, Aldy Hernandez wrote:
> >>>>> (LOOPS_HAVE_PREHEADERS).  This avoids unnecessary changes to the CFG.
> >>>>> But really, I'm just guessing here.  LOOPS_NORMAL would also work,
> >>>>> albeit creating forwarder blocks.
> >>>>>
> >>>>> Tested on x86-64 Linux.
> >>>>>
> >>>>> What do you think?
> >>>>
> >>>> As far as I understand fold_return_value is true/false independent
> >>>> of -W* flags (but dependent on -fprintf-return-value or so).  This means
> >>>> that your patch avoids the risk of CFG changes dependent on
> >>>> -W* flags.  This means you could safely use LOOPS_NORMAL
> >>>
> >>> But isn't my code inside of ::execute, which is still gated by
> >>> fold_return_value AND all the -W* flags:?
> >>>
> >>> bool
> >>> pass_sprintf_length::gate (function *)
> >>> {
> >>>    return ((warn_format_overflow > 0
> >>>           || warn_format_trunc > 0
> >>>           || flag_printf_return_value)
> >>>          && (optimize > 0) == fold_return_value);
> >>> }
> >>>
> >>> Thus, I think we need to move the loop initialization somewhere else ??.
> >>
> >> Then perhaps turn the gate into just return (optimize > 0) == fold_return_value;
> >> and in execute do what you're adding and guard the rest with the above
> >> condition?  As -fprintf-return-value is on by default for C-family, it shouldn't
> >> change much.
> >> + adjust comment on the gate of course.
> >
> > Don't you alreday have
> >
> > @@ -4200,10 +4202,34 @@ pass_sprintf_length::execute (function *fun)
> >     init_target_to_host_charmap ();
> >
> >     calculate_dominance_info (CDI_DOMINATORS);
> > +  bool optimizing_late = optimize > 0 && fold_return_value;
> > +  if (optimizing_late)
> > +    {
> > +      /* ?? We should avoid changing the CFG as much as possible.
> > ...
> > +      loop_optimizer_init (LOOPS_HAVE_PREHEADERS);
> > +      scev_initialize ();
> > +    }
> >
> > so loops are only initialized if fold_return_value is true?  Ah - but that's
> > the pass parameter from params.def rather than the flag to enable
> > the folding...  So indeed, change it to include && flag_printf_return_value
>
> I believe the flag_printf_return_value change was the final agreed upon
> solution.
>
> Tested on x86-64 Linux.
>
> OK for trunk?

OK.

Richard.
Jakub Jelinek Feb. 22, 2019, 12:34 p.m. UTC | #12
On Fri, Feb 22, 2019 at 01:24:38PM +0100, Richard Biener wrote:
> > I believe the flag_printf_return_value change was the final agreed upon
> > solution.
> >
> > Tested on x86-64 Linux.
> >
> > OK for trunk?
> 
> OK.

Just a nit, could you please s/optimizing_late/use_scev/ or something
similar?  With the flag_printf_* in there, it isn't really about optimizing
late, but whether we do or don't want to use scev.

	Jakub
Aldy Hernandez Feb. 22, 2019, 1:46 p.m. UTC | #13
On 2/22/19 7:34 AM, Jakub Jelinek wrote:
> On Fri, Feb 22, 2019 at 01:24:38PM +0100, Richard Biener wrote:
>>> I believe the flag_printf_return_value change was the final agreed upon
>>> solution.
>>>
>>> Tested on x86-64 Linux.
>>>
>>> OK for trunk?
>>
>> OK.
> 
> Just a nit, could you please s/optimizing_late/use_scev/ or something
> similar?  With the flag_printf_* in there, it isn't really about optimizing
> late, but whether we do or don't want to use scev.

Done.  Committed.

Patch
diff mbox series

commit 189e32856dc4656931a66d4da0be81abb2eceb46
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Wed Jan 30 12:25:25 2019 +0100

            PR middle-end/85598
            * gimple-ssa-sprintf.c (pass_sprintf_length::execute): Build
            optimizer info when running late and optimizing.

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 8e6016fc42f..973452fd209 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -65,6 +65,8 @@  along with GCC; see the file COPYING3.  If not see
 #include "tree-ssa-propagate.h"
 #include "calls.h"
 #include "cfgloop.h"
+#include "tree-scalar-evolution.h"
+#include "tree-ssa-loop.h"
 #include "intl.h"
 #include "langhooks.h"
 
@@ -4200,10 +4202,34 @@  pass_sprintf_length::execute (function *fun)
   init_target_to_host_charmap ();
 
   calculate_dominance_info (CDI_DOMINATORS);
+  bool optimizing_late = optimize > 0 && fold_return_value;
+  if (optimizing_late)
+    {
+      /* ?? We should avoid changing the CFG as much as possible.
+	 This is a warning pass, after all.  Thus, use
+	 LOOPS_HAVE_PREHEADERS instead of LOOPS_NORMAL.  This avoids
+	 extensive changes to the CFG, without the full hammer of
+	 AVOID_CFG_MANIPULATIONS which would cripple SCEV.
+
+	 ?? Should we run this outside of the pass (early in the gate
+	 function).
+
+	 FIXME: This should go away, when we have finer grained or
+	 on-demand range information.  */
+      loop_optimizer_init (LOOPS_HAVE_PREHEADERS);
+      scev_initialize ();
+    }
+
 
   sprintf_dom_walker sprintf_dom_walker;
   sprintf_dom_walker.walk (ENTRY_BLOCK_PTR_FOR_FN (fun));
 
+  if (optimizing_late)
+    {
+      scev_finalize ();
+      loop_optimizer_finalize ();
+    }
+
   /* Clean up object size info.  */
   fini_object_sizes ();
   return 0;
diff --git a/gcc/testsuite/gcc.dg/pr85598.c b/gcc/testsuite/gcc.dg/pr85598.c
new file mode 100644
index 00000000000..c84b63f8084
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr85598.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+extern void __chk_fail (void);
+extern int snprintf (char *, size_t, const char *, ...);
+
+int main()
+{
+    char temp[100];
+    unsigned int x;
+    char *str = temp;
+    for(x=0; x<256; ++x) {
+      snprintf(str, 4, "%%%02X", x);
+    }
+}