[Bug tree-optimization/101830] New: Incorrect error messages beginning with r12-2591 (backward jump threader)

[Bug tree-optimization/101830] New: Incorrect error messages beginning with r12-2591 (backward jump threader)

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

            Bug ID: 101830
           Summary: Incorrect error messages beginning with r12-2591
                    (backward jump threader)
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: wschmidt at gcc dot gnu.org
  Target Milestone: ---

I've been adding some code to the rs6000 back end that hasn't yet been part of
the build.  As I got ready to commit the patch to start building the code, I
found that it has recently stopped building due to bogus error messages.  These
started with r12-2591 (new backward jump threader) but is presumably also
related to the new pass for array bounds checking.

$ $GCC_INSTALL/bin/g++ $GCC_SRC/gcc/config/rs6000/rs6000-gen-builtins.c -c -O2
-Wall -Werror
/home/wschmidt/newgcc/gcc/config/rs6000/rs6000-gen-builtins.c: In function 'int
match_bracketed_pair(typeinfo*, char, char, restriction)':
/home/wschmidt/newgcc/gcc/config/rs6000/rs6000-gen-builtins.c:824:22: error:
array subscript 1024 is above array bounds of 'char [1024]'
[-Werror=array-bounds]
  824 |       if (linebuf[pos] != ',')
      |           ~~~~~~~~~~~^
/home/wschmidt/newgcc/gcc/config/rs6000/rs6000-gen-builtins.c:186:13: note:
while referencing 'linebuf'
  186 | static char linebuf[LINELEN];
      |             ^~~~~~~
/home/wschmidt/newgcc/gcc/config/rs6000/rs6000-gen-builtins.c:843:22: error:
array subscript 1024 is above array bounds of 'char [1024]'
[-Werror=array-bounds]
  843 |       if (linebuf[pos] != close)
      |           ~~~~~~~~~~~^
/home/wschmidt/newgcc/gcc/config/rs6000/rs6000-gen-builtins.c:186:13: note:
while referencing 'linebuf'
  186 | static char linebuf[LINELEN];
      |             ^~~~~~~

...and many more, all referencing uses of linebuf[pos].

There are several problems with these messages:

(1) linebuf and pos are global variables, and the compiler cannot tell whether
or not there are problems with array bounds accesses here. Indeed, pos is only
incremented by a function called "safe_inc_pos" that ensures we *don't* ever
access beyond the end of linebuf.

(2) The error message is far too certain of itself!  It says that we have
definitely addressed out of bounds when that is certainly not known to be true.

(3) Some uses of linebuf[pos] are flagged, and others are not, often within the
same function.

You can reproduce this as above with the code that is upstream in trunk.

This is holding up committing some approved patches, so I appreciate anything
you can do to sort this out.

[Bug tree-optimization/101830] Incorrect error messages beginning with r12-2591 (backward jump threader)

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |12.0
   Target Milestone|---                         |12.0
             Status|UNCONFIRMED                 |NEW
           Keywords|                            |diagnostic, rejects-valid
             Target|                            |powerpc*-*-*
     Ever confirmed|0                           |1
                 CC|                            |aldyh at gcc dot gnu.org,
                   |                            |bergner at gcc dot gnu.org,
                   |                            |msebor at gcc dot gnu.org,
                   |                            |segher at gcc dot gnu.org
   Last reconfirmed|                            |2021-08-09

--- Comment #1 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
COnfirmed.

[Bug tree-optimization/101830] Incorrect error messages beginning with r12-2591 (backward jump threader)

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
It might be useful to get the preprocessed source.

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #3 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Created attachment 51281
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51281&action=edit
Preprocessed source

Per request, preprocessed source.

Compile with "g++ rs6000-gen-builtins.ii -c -O2 -Wall -Werror" to reproduce.

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #4 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Bill Schmidt from comment #0)
> (1) linebuf and pos are global variables, and the compiler cannot tell
> whether or not there are problems with array bounds accesses here. Indeed,
> pos is only incremented by a function called "safe_inc_pos" that ensures we
> *don't* ever access beyond the end of linebuf.

Most warnings are heuristic, largely by need, and this is true here as well.
-Werror is a terrible idea.  You can use it during development if you don't
trust the programmers (which might be yourself) will see the messages, but
otherwise it is just a destructive hindrance.  That said...

> (2) The error message is far too certain of itself!  It says that we have
> definitely addressed out of bounds when that is certainly not known to be
> true.

And its language claims it is even more certain that this was linepos+1024,
which you say can never be true?

It is possible there is some other bug in your program, which combined with
aggressive optimisations elsewhere gives the access to that address, but :-/
(Looking at -fdump-tree-all-all dumps should tell).

More likely is the heuristic just gets the wrong result.  This happens so
often that this warning does not belong in -Wall, and not even in -W (since
there is no simple workaround for it at all, not even any reliable
workaround).

And yes, whenever some warning includes any heuristic (i.e. almost all
warnings), the warning message should not say "is wrong".  Diagnostics should
never lie.  Not ever.  This used to be a big strength of GCC's diagnostics.

All such messages should say something like "may be wrong".  This is a much
nicer, more respectful way to talk to the compiler user anyway, even if it
could not be wrong :-)


> This is holding up committing some approved patches, so I appreciate
> anything you can do to sort this out.

Configure with --disable-werror if you want more sanity.  Of course people
will then be angry with you if you commit something that triggers something
bad :-/  Maybe that builds character?

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #5 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
If I commit the build patch, everyone who doesn't build with --disable-werror
will blame me for breaking bootstrap.

I thought perhaps the way safe_inc_pos was implemented might have made it
possible for the warning machinery to make this error, but I experimented with
changes and nothing helps.

Just to be clear:  The code is fine.  The warning is bogus.

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #6 from Martin Sebor <msebor at gcc dot gnu.org> ---
I've only looked at the first warning so far.  It's issued for the access in bb
8:

  <bb 5> [local count: 4057510040]:
  pos.80_31 = pos;
  if (pos.80_31 <= 1023)
    goto <bb 8>; [96.34%]
  else
    goto <bb 6>; [3.66%]

  <bb 8> [local count: 256307115]:
  # pos.80_21 = PHI <pos.80_81(36)>
  _1 = linebuf[pos.80_21];   <<< -Warray-bounds
  ...

The index is in the range [1024, INT_MAX] so the warning is correct given the
IL. There isn't much I see that could be improved about the diagnostic except
mentioning the range of the subscript rather than just its lower bound.  This
instance of the warning or its phrasing also haven't changed in years.  It's
not the result of a recent enhancement or a questionable heuristic but simply
reflects a change in the IL, and it's always been phrased as "is out of
bounds".  No "may be out of bounds" form exists, never has, and adding one
wouldn't help in this instance.

That said, since pos is a global variable, the test in safe_inc_pos() that
would otherwise constrain its value only has that effect in the absence of
intervening statements that might overwrite it.  You might get a better result
with a pair of "setter" and "getter" functions where the latter asserted the
range via __builtin_unreachable() before returning the variable.  Otherwise,
the test is what likely is used by the backward threader to introduce the
unreachable branch which isn't eliminated because GCC can't prove the variable
isn't incremented beyond its upper limit.  (Aldy is in a much better position
to explain this.)

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #7 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Sorry, but that IL looks very strange to me.

BB 5 should be going directly to BB 8, and the value of interest along that
path is pos.80_31.  But BB 8 says that it only gets pos.80 from BB 36, and the
value along that path is pos.80_81.

For the index to be in the range [1024, INT_MAX] is absurd on its face, coming
from BB 5 where it is clearly constrained to be within [INT_MIN, 1023].

Can you please explain?  I don't understand this at all.

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #8 from Martin Sebor <msebor at gcc dot gnu.org> ---
Created attachment 51298
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51298&action=edit
Change to test case that avoids -Warray-bounds.

The attached change to test case avoids all -Warray-bounds instances.

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #9 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
But it doesn't explain the bogus IL in the previous message.

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #10 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
As a reminder, the code compiled fine with no warnings until the rewrite of the
back-threader.  Based on the IL example above, it looks to me like the new pass
is not producing a self-consistent CFG in all cases.

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |INVALID

--- Comment #11 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Aha.  There is a coding bug in consume_whitespace () that *could* allow a
buffer overrun.

  while (pos < LINELEN && isspace(linebuf[pos]) && linebuf[pos] != '\n')
    pos++;

Subsequent reading of linebuf[pos] might read past the end of the buffer. 
There should be a guard afterwards if pos exceeds LINELEN - 1.

Fixing this does allow the code to compile with the current compiler.  So, I'm
sorry for the false report.

The warning message is still misleading, saying that there is definitely an
overrun, when there is only the possibility of one.  (We never encounter the
overrun on a rather large set of inputs.)  I'm not sure what can be done about
that.

I think the IL above that looks funky is probably just some sort of cut and
paste problem.  I didn't see the oddity in compiler dumps when I was trying to
find it.  So that's likely a red herring.

Thanks for having a look, Martin!

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|INVALID                     |FIXED

--- Comment #12 from Martin Sebor <msebor at gcc dot gnu.org> ---
No problem.

By the way, it looks to me like safe_inc_pos() also isn't entirely safe since
testing the result of the postincrement lets pos reach 1024 on function return.
 It should either use preincrement or 1023 as the bound.

It's of course possible to issue a more nuanced warning ("may be out of
bounds") for expressions that are invalid only under some condition, in basic
blocks that aren't dominated by function entry.  It has been suggested (and
considered) a number of times before.  The problem with a simplistic solution
like that is that it would result in the vast majority of warnings being
phrased this way, because most are in such blocks.  The only certain warnings
would be either in trivial functions or in the initial basic blocks.  I'm
working on introducing this distinction for PHIs but I don't have any ideas
what to do for problems like this one.

[Bug tree-optimization/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[wschmidt at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #13 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Yes, absolutely right on safe_inc_pos, will address that as well.  Much
obliged!

[Bug target/101830] [12 Regression] Incorrect error messages beginning with r12-2591 (backward jump threader)

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101830

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by William Schmidt <wschmidt@gcc.gnu.org>:

https://gcc.gnu.org/g:30c335ac44ecb4f17645925360177618763d7c48

commit r12-3097-g30c335ac44ecb4f17645925360177618763d7c48
Author: Bill Schmidt <wschmidt@linux.ibm.com>
Date:   Thu Aug 19 16:07:55 2021 -0500

    rs6000: Avoid buffer overruns

    2021-08-19  Bill Schmidt  <wschmidt@linux.ibm.com>

    gcc/
            PR target/101830
            * config/rs6000/rs6000-gen-builtins.c (consume_whitespace):
            Diagnose buffer overrun.
            (safe_inc_pos): Fix overrun detection.
            (match_identifier): Diagnose buffer overrun.
            (match_integer): Likewise.
            (match_to_right_bracket): Likewise.