diff mbox series

run early sprintf warning after SSA (PR 100325)

Message ID a1923ce5-562a-4a9b-cc4d-e34656d94bc2@gmail.com
State New
Headers show
Series run early sprintf warning after SSA (PR 100325) | expand

Commit Message

Martin Sebor May 4, 2021, 10:15 p.m. UTC 
With no optimization, -Wformat-overflow and -Wformat-truncation
runs early to detect a subset of simple bugs.  But as it turns out,
the pass runs just a tad too early, before SSA.  That causes it to
miss a class of problems that can easily be detected once code is
in SSA form, and I would expect might also cause false positives.

The attached change moves the sprintf pass just after pass_build_ssa,
similar to other early flow-sensitive warnings (-Wnonnull-compare and
-Wuninitialized).

Martin

Comments

Richard Biener May 5, 2021, 7:26 a.m. UTC | #1 
On Wed, May 5, 2021 at 1:32 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> With no optimization, -Wformat-overflow and -Wformat-truncation
> runs early to detect a subset of simple bugs.  But as it turns out,
> the pass runs just a tad too early, before SSA.  That causes it to
> miss a class of problems that can easily be detected once code is
> in SSA form, and I would expect might also cause false positives.
>
> The attached change moves the sprintf pass just after pass_build_ssa,
> similar to other early flow-sensitive warnings (-Wnonnull-compare and
> -Wuninitialized).

Makes sense.  I suppose walloca might also benefit from SSA - it seems
to do range queries which won't work quite well w/o SSA?

Thus OK.

Thanks,
Richard.

> Martin
Aldy Hernandez May 6, 2021, 2:32 p.m. UTC | #2 
On 5/5/21 9:26 AM, Richard Biener wrote:
> On Wed, May 5, 2021 at 1:32 AM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> With no optimization, -Wformat-overflow and -Wformat-truncation
>> runs early to detect a subset of simple bugs.  But as it turns out,
>> the pass runs just a tad too early, before SSA.  That causes it to
>> miss a class of problems that can easily be detected once code is
>> in SSA form, and I would expect might also cause false positives.
>>
>> The attached change moves the sprintf pass just after pass_build_ssa,
>> similar to other early flow-sensitive warnings (-Wnonnull-compare and
>> -Wuninitialized).
> 
> Makes sense.  I suppose walloca might also benefit from SSA - it seems
> to do range queries which won't work quite well w/o SSA?

The early Walloca pass that runs without optimization doesn't do much, 
as we've never had ranges so early.  All it does is diagnose _every_ 
call to alloca(), if -Walloca is passed:

   // The first time this pass is called, it is called before
   // optimizations have been run and range information is unavailable,
   // so we can only perform strict alloca checking.
   if (first_time_p)
     return warn_alloca != 0;

Though, I suppose we could move the first alloca pass after SSA is 
available and make it the one and only pass, since ranger only needs 
SSA.  However, I don't know how well this would work without value 
numbering or CSE.  For example, for gcc.dg/Walloca-4.c the gimple is:

   <bb 2> :
   _1 = rear_ptr_9(D) - w_10(D);
   _2 = (long unsigned int) _1;
   if (_2 <= 4095)
     goto <bb 3>; [INV]
   else
     goto <bb 4>; [INV]

   <bb 3> :
   _3 = rear_ptr_9(D) - w_10(D);
   _4 = (long unsigned int) _3;
   src_16 = __builtin_alloca (_4);
   goto <bb 5>; [INV]

No ranges can be determined for _4.  However, if either FRE or DOM run, 
as they do value numbering and CSE respectively, we could easily 
determine a range as the above would become:

  <bb 2> :
   _1 = rear_ptr_9(D) - w_10(D);
   _2 = (long unsigned int) _1;
   if (_2 <= 4095)
     goto <bb 3>; [INV]
   else
     goto <bb 4>; [INV]

   <bb 3> :
   src_16 = __builtin_alloca (_2);
   goto <bb 5>; [INV]

I'm inclined to leave the first alloca pass before SSA runs, as it 
doesn't do anything with ranges.  If anyone's open to a simple -O0 CSE 
type pass, it would be a different story.  Thoughts?

Aldy
Martin Sebor May 7, 2021, 12:12 a.m. UTC | #3 
On 5/6/21 8:32 AM, Aldy Hernandez wrote:
> 
> 
> On 5/5/21 9:26 AM, Richard Biener wrote:
>> On Wed, May 5, 2021 at 1:32 AM Martin Sebor via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>> With no optimization, -Wformat-overflow and -Wformat-truncation
>>> runs early to detect a subset of simple bugs.  But as it turns out,
>>> the pass runs just a tad too early, before SSA.  That causes it to
>>> miss a class of problems that can easily be detected once code is
>>> in SSA form, and I would expect might also cause false positives.
>>>
>>> The attached change moves the sprintf pass just after pass_build_ssa,
>>> similar to other early flow-sensitive warnings (-Wnonnull-compare and
>>> -Wuninitialized).
>>
>> Makes sense.  I suppose walloca might also benefit from SSA - it seems
>> to do range queries which won't work quite well w/o SSA?
> 
> The early Walloca pass that runs without optimization doesn't do much, 
> as we've never had ranges so early.  All it does is diagnose _every_ 
> call to alloca(), if -Walloca is passed:
> 
>    // The first time this pass is called, it is called before
>    // optimizations have been run and range information is unavailable,
>    // so we can only perform strict alloca checking.
>    if (first_time_p)
>      return warn_alloca != 0;
> 
> Though, I suppose we could move the first alloca pass after SSA is 
> available and make it the one and only pass, since ranger only needs 
> SSA.  However, I don't know how well this would work without value 
> numbering or CSE.  For example, for gcc.dg/Walloca-4.c the gimple is:
> 
>    <bb 2> :
>    _1 = rear_ptr_9(D) - w_10(D);
>    _2 = (long unsigned int) _1;
>    if (_2 <= 4095)
>      goto <bb 3>; [INV]
>    else
>      goto <bb 4>; [INV]
> 
>    <bb 3> :
>    _3 = rear_ptr_9(D) - w_10(D);
>    _4 = (long unsigned int) _3;
>    src_16 = __builtin_alloca (_4);
>    goto <bb 5>; [INV]
> 
> No ranges can be determined for _4.  However, if either FRE or DOM run, 
> as they do value numbering and CSE respectively, we could easily 
> determine a range as the above would become:
> 
>   <bb 2> :
>    _1 = rear_ptr_9(D) - w_10(D);
>    _2 = (long unsigned int) _1;
>    if (_2 <= 4095)
>      goto <bb 3>; [INV]
>    else
>      goto <bb 4>; [INV]
> 
>    <bb 3> :
>    src_16 = __builtin_alloca (_2);
>    goto <bb 5>; [INV]
> 
> I'm inclined to leave the first alloca pass before SSA runs, as it 
> doesn't do anything with ranges.  If anyone's open to a simple -O0 CSE 
> type pass, it would be a different story.  Thoughts?

Improving the analysis at -O0 and getting better warnings that are
more consistent with what is issued with optimization would be very
helpful (as as long as it doesn't compromise debugging experience
of course).

Martin

> 
> Aldy
>
Richard Biener May 7, 2021, 9:34 a.m. UTC | #4 
On Fri, May 7, 2021 at 2:12 AM Martin Sebor <msebor@gmail.com> wrote:
>
> On 5/6/21 8:32 AM, Aldy Hernandez wrote:
> >
> >
> > On 5/5/21 9:26 AM, Richard Biener wrote:
> >> On Wed, May 5, 2021 at 1:32 AM Martin Sebor via Gcc-patches
> >> <gcc-patches@gcc.gnu.org> wrote:
> >>>
> >>> With no optimization, -Wformat-overflow and -Wformat-truncation
> >>> runs early to detect a subset of simple bugs.  But as it turns out,
> >>> the pass runs just a tad too early, before SSA.  That causes it to
> >>> miss a class of problems that can easily be detected once code is
> >>> in SSA form, and I would expect might also cause false positives.
> >>>
> >>> The attached change moves the sprintf pass just after pass_build_ssa,
> >>> similar to other early flow-sensitive warnings (-Wnonnull-compare and
> >>> -Wuninitialized).
> >>
> >> Makes sense.  I suppose walloca might also benefit from SSA - it seems
> >> to do range queries which won't work quite well w/o SSA?
> >
> > The early Walloca pass that runs without optimization doesn't do much,
> > as we've never had ranges so early.  All it does is diagnose _every_
> > call to alloca(), if -Walloca is passed:
> >
> >    // The first time this pass is called, it is called before
> >    // optimizations have been run and range information is unavailable,
> >    // so we can only perform strict alloca checking.
> >    if (first_time_p)
> >      return warn_alloca != 0;
> >
> > Though, I suppose we could move the first alloca pass after SSA is
> > available and make it the one and only pass, since ranger only needs
> > SSA.  However, I don't know how well this would work without value
> > numbering or CSE.  For example, for gcc.dg/Walloca-4.c the gimple is:
> >
> >    <bb 2> :
> >    _1 = rear_ptr_9(D) - w_10(D);
> >    _2 = (long unsigned int) _1;
> >    if (_2 <= 4095)
> >      goto <bb 3>; [INV]
> >    else
> >      goto <bb 4>; [INV]
> >
> >    <bb 3> :
> >    _3 = rear_ptr_9(D) - w_10(D);
> >    _4 = (long unsigned int) _3;
> >    src_16 = __builtin_alloca (_4);
> >    goto <bb 5>; [INV]
> >
> > No ranges can be determined for _4.  However, if either FRE or DOM run,
> > as they do value numbering and CSE respectively, we could easily
> > determine a range as the above would become:
> >
> >   <bb 2> :
> >    _1 = rear_ptr_9(D) - w_10(D);
> >    _2 = (long unsigned int) _1;
> >    if (_2 <= 4095)
> >      goto <bb 3>; [INV]
> >    else
> >      goto <bb 4>; [INV]
> >
> >    <bb 3> :
> >    src_16 = __builtin_alloca (_2);
> >    goto <bb 5>; [INV]
> >
> > I'm inclined to leave the first alloca pass before SSA runs, as it
> > doesn't do anything with ranges.  If anyone's open to a simple -O0 CSE
> > type pass, it would be a different story.  Thoughts?
>
> Improving the analysis at -O0 and getting better warnings that are
> more consistent with what is issued with optimization would be very
> helpful (as as long as it doesn't compromise debugging experience
> of course).

I agree.  It shouldn't be too difficult to for example run the VN
propagation part without doing actual elimiation and keep
value-numbers for consumption.  do_rpo_vn (not exported)
might even already support iterate = false, eliminate = false,
it would just need factoring out the init/deinit somewhat.

Of course it will be a lot more expensive to do since it cannot
do "on-demand" value-numbering of interesting SSA names.
I'm not sure that would be possible anyhow.  Though for
the alloca case quickly scanning the function whether there's
any would of course be faster than throwing VN at it.

Oh, and no - we don't want to perform CSE at -O0 (I mean
affecting generated code).

Richard.

> Martin
>
> >
> > Aldy
> >
>
Aldy Hernandez May 7, 2021, 9:49 a.m. UTC | #5 
On 5/7/21 11:34 AM, Richard Biener wrote:
> On Fri, May 7, 2021 at 2:12 AM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 5/6/21 8:32 AM, Aldy Hernandez wrote:
>>>
>>>
>>> On 5/5/21 9:26 AM, Richard Biener wrote:
>>>> On Wed, May 5, 2021 at 1:32 AM Martin Sebor via Gcc-patches
>>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>>
>>>>> With no optimization, -Wformat-overflow and -Wformat-truncation
>>>>> runs early to detect a subset of simple bugs.  But as it turns out,
>>>>> the pass runs just a tad too early, before SSA.  That causes it to
>>>>> miss a class of problems that can easily be detected once code is
>>>>> in SSA form, and I would expect might also cause false positives.
>>>>>
>>>>> The attached change moves the sprintf pass just after pass_build_ssa,
>>>>> similar to other early flow-sensitive warnings (-Wnonnull-compare and
>>>>> -Wuninitialized).
>>>>
>>>> Makes sense.  I suppose walloca might also benefit from SSA - it seems
>>>> to do range queries which won't work quite well w/o SSA?
>>>
>>> The early Walloca pass that runs without optimization doesn't do much,
>>> as we've never had ranges so early.  All it does is diagnose _every_
>>> call to alloca(), if -Walloca is passed:
>>>
>>>     // The first time this pass is called, it is called before
>>>     // optimizations have been run and range information is unavailable,
>>>     // so we can only perform strict alloca checking.
>>>     if (first_time_p)
>>>       return warn_alloca != 0;
>>>
>>> Though, I suppose we could move the first alloca pass after SSA is
>>> available and make it the one and only pass, since ranger only needs
>>> SSA.  However, I don't know how well this would work without value
>>> numbering or CSE.  For example, for gcc.dg/Walloca-4.c the gimple is:
>>>
>>>     <bb 2> :
>>>     _1 = rear_ptr_9(D) - w_10(D);
>>>     _2 = (long unsigned int) _1;
>>>     if (_2 <= 4095)
>>>       goto <bb 3>; [INV]
>>>     else
>>>       goto <bb 4>; [INV]
>>>
>>>     <bb 3> :
>>>     _3 = rear_ptr_9(D) - w_10(D);
>>>     _4 = (long unsigned int) _3;
>>>     src_16 = __builtin_alloca (_4);
>>>     goto <bb 5>; [INV]
>>>
>>> No ranges can be determined for _4.  However, if either FRE or DOM run,
>>> as they do value numbering and CSE respectively, we could easily
>>> determine a range as the above would become:
>>>
>>>    <bb 2> :
>>>     _1 = rear_ptr_9(D) - w_10(D);
>>>     _2 = (long unsigned int) _1;
>>>     if (_2 <= 4095)
>>>       goto <bb 3>; [INV]
>>>     else
>>>       goto <bb 4>; [INV]
>>>
>>>     <bb 3> :
>>>     src_16 = __builtin_alloca (_2);
>>>     goto <bb 5>; [INV]
>>>
>>> I'm inclined to leave the first alloca pass before SSA runs, as it
>>> doesn't do anything with ranges.  If anyone's open to a simple -O0 CSE
>>> type pass, it would be a different story.  Thoughts?
>>
>> Improving the analysis at -O0 and getting better warnings that are
>> more consistent with what is issued with optimization would be very
>> helpful (as as long as it doesn't compromise debugging experience
>> of course).
> 
> I agree.  It shouldn't be too difficult to for example run the VN
> propagation part without doing actual elimiation and keep
> value-numbers for consumption.  do_rpo_vn (not exported)
> might even already support iterate = false, eliminate = false,
> it would just need factoring out the init/deinit somewhat.

Interesting.  This could give good ranges at -O0 and make it possible to 
move all these pesky range needy passes early in the pipeline.

> Of course it will be a lot more expensive to do since it cannot
> do "on-demand" value-numbering of interesting SSA names.
> I'm not sure that would be possible anyhow.  Though for
> the alloca case quickly scanning the function whether there's
> any would of course be faster than throwing VN at it.

That's exact what we do for strict -Walloca warnings.  For 
-Walloca-larger-than=, you need ranges though, so your VN idea would fit 
the bill.

Aldy
diff mbox series

Patch

PR middle-end/100325 - missing warning with -O0 on sprintf overflow with pointer plus offset

gcc/ChangeLog:

	* passes.def (pass_warn_printf): Run after SSA.

gcc/testsuite/ChangeLog:

	* gcc.dg/tree-ssa/builtin-sprintf-warn-26.c: New test.

diff --git a/gcc/passes.def b/gcc/passes.def
index 55e8164d56b..de39fa48b3d 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -45,7 +45,6 @@  along with GCC; see the file COPYING3.  If not see
   NEXT_PASS (pass_warn_function_return);
   NEXT_PASS (pass_coroutine_early_expand_ifns);
   NEXT_PASS (pass_expand_omp);
-  NEXT_PASS (pass_warn_printf);
   NEXT_PASS (pass_walloca, /*strict_mode_p=*/true);
   NEXT_PASS (pass_build_cgraph_edges);
   TERMINATE_PASS_LIST (all_lowering_passes)
@@ -58,6 +57,7 @@  along with GCC; see the file COPYING3.  If not see
   PUSH_INSERT_PASSES_WITHIN (pass_build_ssa_passes)
       NEXT_PASS (pass_fixup_cfg);
       NEXT_PASS (pass_build_ssa);
+      NEXT_PASS (pass_warn_printf);
       NEXT_PASS (pass_warn_nonnull_compare);
       NEXT_PASS (pass_early_warn_uninitialized);
       NEXT_PASS (pass_ubsan);
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-26.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-26.c
index 16a551d9c8d..677b6345c5c 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-26.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-26.c
@@ -22,17 +22,17 @@  void nowarn_4m3 ()
 void warn_2m1 ()
 {
   char *p = a + 2;
-  sprintf (p - 1, "%i", 123);   // { dg-warning "-Wformat-overflow" "pr100325" { xfail *-*-* } }
+  sprintf (p - 1, "%i", 123);   // { dg-warning "-Wformat-overflow" "pr100325" }
 }
 
 void warn_3m1 ()
 {
   char *p = a + 3;
-  sprintf (p - 1, "%i", 12);    // { dg-warning "-Wformat-overflow" "pr100325" { xfail *-*-* } }
+  sprintf (p - 1, "%i", 12);    // { dg-warning "-Wformat-overflow" "pr100325" }
 }
 
 void warn_4m1 ()
 {
   char *p = a + 4;
-  sprintf (p - 1, "%i", 1);     // { dg-warning "-Wformat-overflow" "pr100325" { xfail *-*-* } }
+  sprintf (p - 1, "%i", 1);     // { dg-warning "-Wformat-overflow" "pr100325" }
 }