| 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 | expand |
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
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
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
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
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
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
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
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
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
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); + } +}
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.
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
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.
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);
+ }
+}
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