Re: [PR middle-end/85598] make -Wprintf* pass use loop info for PHI's

[Aldy Hernandez]

On 1/31/19 9:56 AM, Richard Biener wrote:
On Thu, Jan 31, 2019 at 3:45 PM Jakub Jelinek <ja...@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);
+    }
+}