[Bug tree-optimization/103176] New: -foptimize-strlen causes stringop-overflow warning

[Bug tree-optimization/103176] New: -foptimize-strlen causes stringop-overflow warning

[josiah_vanderzee at mediacombb dot net]

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

            Bug ID: 103176
           Summary: -foptimize-strlen causes stringop-overflow warning
           Product: gcc
           Version: 10.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: josiah_vanderzee at mediacombb dot net
  Target Milestone: ---

The following warning appears while compiling
https://github.com/minetest/irrlicht with at least -O2. Compiling with
-fno-optimize-strlen makes the warning disappear. Diffing a binary compiled
with the optimization and one compiled without show that there is no difference
in the generated code.The relevant functions are inlined (and I verified this
by hand) so all of it is present in the binary. The warning follows.

I have considered the view that the warning is desirable; the code is very
dangerous; and I am not claiming that the warning is incorrect. The problem
here is that for some reason the warning only occurs when strlen optimizations
are enabled. This doesn't seem logical to me.

NOTE: The binaries I diffed were generated by compiling the flattenFilepath()
function as a unit test.

[49/99] ccache /usr/bin/c++ -DIRRLICHT_EXPORTS -D_IRR_STATIC_LIB_ -I../include
-I../source/Irrlicht -O3 -fPIC -Wall -pipe -fno-exceptions -fno-rtti
-std=gnu++11 -MD -MT source/Irrlicht/CMakeFiles/IRRIOOBJ.dir/CFileSystem.cpp.o
-MF source/Irrlicht/CMakeFiles/IRRIOOBJ.dir/CFileSystem.cpp.o.d -o
source/Irrlicht/CMakeFiles/IRRIOOBJ.dir/CFileSystem.cpp.o -c
../source/Irrlicht/CFileSystem.cpp
In file included from ../include/coreutil.h:8,
                 from ../include/IReadFile.h:9,
                 from ../include/IFileArchive.h:8,
                 from ../include/IFileSystem.h:9,
                 from ../source/Irrlicht/CFileSystem.h:8,
                 from ../source/Irrlicht/CFileSystem.cpp:7:
In member function ‘irr::core::string<T> irr::core::string<T,
TAlloc>::subString(irr::u32, irr::s32, bool) const [with T = char; TAlloc =
irr::core::irrAllocator<char>]’,
    inlined from ‘virtual irr::io::path&
irr::io::CFileSystem::flattenFilename(irr::io::path&, const path&) const’ at
../source/Irrlicht/CFileSystem.cpp:679:58:
../include/irrString.h:937:19: warning: writing 1 byte into a region of size 0
[-Wstringop-overflow=]
  937 |   o.array[length] = 0;
      |   ~~~~~~~~~~~~~~~~^~~
In file included from ../include/irrString.h:9,
                 from ../include/coreutil.h:8,
                 from ../include/IReadFile.h:9,
                 from ../include/IFileArchive.h:8,
                 from ../include/IFileSystem.h:9,
                 from ../source/Irrlicht/CFileSystem.h:8,
                 from ../source/Irrlicht/CFileSystem.cpp:7:
../include/irrAllocator.h: In member function ‘virtual irr::io::path&
irr::io::CFileSystem::flattenFilename(irr::io::path&, const path&) const’:
../include/irrAllocator.h:60:22: note: at offset [1, -1] to an object with size
1 allocated by ‘operator new’ here
   60 |   return operator new(cnt);
      |          ~~~~~~~~~~~~^~~~~
In file included from ../include/coreutil.h:8,
                 from ../include/IReadFile.h:9,
                 from ../include/IFileArchive.h:8,
                 from ../include/IFileSystem.h:9,
                 from ../source/Irrlicht/CFileSystem.h:8,
                 from ../source/Irrlicht/CFileSystem.cpp:7:
In member function ‘irr::core::string<T> irr::core::string<T,
TAlloc>::subString(irr::u32, irr::s32, bool) const [with T = char; TAlloc =
irr::core::irrAllocator<char>]’,
    inlined from ‘virtual irr::io::path
irr::io::CFileSystem::getFileBasename(const path&, bool) const’ at
../source/Irrlicht/CFileSystem.cpp:655:73:
../include/irrString.h:937:19: warning: writing 1 byte into a region of size 0
[-Wstringop-overflow=]
  937 |   o.array[length] = 0;
      |   ~~~~~~~~~~~~~~~~^~~
In file included from ../include/irrString.h:9,
                 from ../include/coreutil.h:8,
                 from ../include/IReadFile.h:9,
                 from ../include/IFileArchive.h:8,
                 from ../include/IFileSystem.h:9,
                 from ../source/Irrlicht/CFileSystem.h:8,
                 from ../source/Irrlicht/CFileSystem.cpp:7:
../include/irrAllocator.h: In member function ‘virtual irr::io::path
irr::io::CFileSystem::getFileBasename(const path&, bool) const’:
../include/irrAllocator.h:60:22: note: at offset [1, -1] to an object with size
1 allocated by ‘operator new’ here
   60 |   return operator new(cnt);
      |          ~~~~~~~~~~~~^~~~~
In file included from ../include/coreutil.h:8,
                 from ../include/IReadFile.h:9,
                 from ../include/IFileArchive.h:8,
                 from ../include/IFileSystem.h:9,
                 from ../source/Irrlicht/CFileSystem.h:8,
                 from ../source/Irrlicht/CFileSystem.cpp:7:
In member function ‘irr::core::string<T> irr::core::string<T,
TAlloc>::subString(irr::u32, irr::s32, bool) const [with T = char; TAlloc =
irr::core::irrAllocator<char>]’,
    inlined from ‘virtual irr::io::path
irr::io::CFileSystem::getRelativeFilename(const path&, const path&) const’ at
../include/coreutil.h:158:70:
../include/irrString.h:937:19: warning: writing 1 byte into a region of size 0
[-Wstringop-overflow=]
  937 |   o.array[length] = 0;
      |   ~~~~~~~~~~~~~~~~^~~
In file included from ../include/irrString.h:9,
                 from ../include/coreutil.h:8,
                 from ../include/IReadFile.h:9,
                 from ../include/IFileArchive.h:8,
                 from ../include/IFileSystem.h:9,
                 from ../source/Irrlicht/CFileSystem.h:8,
                 from ../source/Irrlicht/CFileSystem.cpp:7:
../include/irrAllocator.h: In member function ‘virtual irr::io::path
irr::io::CFileSystem::getRelativeFilename(const path&, const path&) const’:
../include/irrAllocator.h:60:22: note: at offset [1, -1] to an object with size
1 allocated by ‘operator new’ here
   60 |   return operator new(cnt);
      |          ~~~~~~~~~~~~^~~~~```

[Bug tree-optimization/103176] -foptimize-strlen causes stringop-overflow warning

[josiah_vanderzee at mediacombb dot net]

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

Josiah VanderZee <josiah_vanderzee at mediacombb dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |https://github.com/minetest
                   |                            |/irrlicht

--- Comment #1 from Josiah VanderZee <josiah_vanderzee at mediacombb dot net> ---
Correction: the note should say flattenFilename(), not flattenFilepath

[Bug tree-optimization/103176] -foptimize-strlen causes stringop-overflow warning

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2021-11-10
     Ever confirmed|0                           |1
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #2 from Martin Sebor <msebor at gcc dot gnu.org> ---
Historically, flow-dependent warnings in GCC have relied on optimization.  Some
work without it and just yield better results with it (e.g., -Wuninitialized),
others require it and won't do anything otherwise (e.g., -Warray-bounds).  This
is starting to change so that more warnings are useful even without
optimization, but only slowly, and to a limited extent (e.g., to see across
function call boundaries requires inlining and likely will for the foreseeable
future).  Some warnings like -Wstringop-overflow are a hybrid: some instances
are issued only during the strlen optimization, others at other times.  None of
these warnings are free of false positives (in fact, none ever are).  Some are
unavoidable due to limits inherent in computing, some due to design trade-offs,
others are incidental (i.e., bugs).  To tell where the instances in comment #0
fall we ask for a test case, or at least the translation unit, so that we can
easily reproduce and analyze them (see https://gcc.gnu.org/bugs/).  I set the
status to WAITING until you have provided one.

[Bug tree-optimization/103176] -foptimize-strlen causes stringop-overflow warning

[josiah_vanderzee at mediacombb dot net]

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

--- Comment #3 from Josiah VanderZee <josiah_vanderzee at mediacombb dot net> ---
Created attachment 51768
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51768&action=edit
Test case to reproduce strange -Wstringop-overflow warning

I'm worried this test case is too large to conveniently go through and
investigate. I tried to make a much smaller test case, but removing any part of
the attached code seems to completely break it. I read through the subString
implementation with great care, and I believe the warning is wrong, it is
impossible to access a -1 offset, unless there is undefined behavior going on.

The warning can be fixed by declaring a temporary variable temp in place of the
expression length + 1, and using temp - 1 to set the null terminator at the
end. I thought this meant that the warning simply saw the array[length] = 0 as
a common "wrong" pattern, but that was not enough to reproduce it, so clearly
something stranger is afoot.

[Bug tree-optimization/103176] -foptimize-strlen causes stringop-overflow warning

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #4 from Martin Sebor <msebor at gcc dot gnu.org> ---
Thanks for the test case.  The warning is based on the IL below.  In basic
block 16 the result of operator new is assigned to _134, and in basic block 19
the code stores zero at an negative offset from _134.  So the warning is doing
its job here: the invalid statement exists in the emitted program, it just may
be unreachable and GCC can't prove that.

  <bb 16> [local count: 19040066]:
  _82 = len_49 - lastpos.2_4;
  length_83 = (int) _82;
  o ={v} {CLOBBER};
  _134 = operator new (1);       >>> _134
  goto <bb 18>; [100.00%]
  ...
  <bb 18> [local count: 19040066]:
  MEM[(char *)_134] = 0;
  _85 = length_83 + 1;
  _86 = (unsigned int) _85;
  if (_86 == 0)
    goto <bb 19>; [80.71%]
  else
    goto <bb 20>; [19.29%]

  <bb 19> [local count: 25513689]:
  MEM[(char *)_134 + -1B] = 0;   <<< -Wstringop-overflow
  goto <bb 26>; [100.00%]

The cause of the warning is in the subString() function where it can't prove
that the length argument won't become negative:

        string<T> subString(unsigned begin, int length) const
        {
                // if start after string
                // or no proper substring length
                if ((length <= 0) || (begin>=size()))
                        return string<T>{ "" };
                // clamp length to maximal value
                if ((length+begin) > size())
                        length = size()-begin;   <<< length not proven to
become -1

                string<T> o;
                o.reserve(length+1);

                for (int i=0; i<length; ++i)
                        o.array[i] = array[i+begin];

                o.array[length] = 0;             <<< store to o.array[-1]
emitted
                o.used = length + 1;

                return o;
        }

Adding the following just before the definition of the string o prevents the
warning.  Another alternative is to make both length and the loop control
variable unsigned (making variables that represent sizes unsigned and keeping
them that is a good practice since it communicates that constraint to the
compiler).

                if (length < 0)
                  __builtin_unreachable ();

In general, unless the compiler can prove that some expression won't reach a
value that's invalid in some context, it can substitute the value for it in the
process of optimizing surrounding code, which can then lead to the invalid code
materializing even if it's not actually in the original source and even if the
emitted invalid object code isn't reachable.  GCC warnings that are designed to
look for this invalid code then trigger on it.  Resolving these warnings can
often help GCC generate better object code (the __builtin_unreachable() trick
isn't necessarily always the best way; it's only appropriate if the
precondition is in the source and GCC loses track of it).  Long story short,
there is nothing for us to do to.

[Bug tree-optimization/103176] -foptimize-strlen causes stringop-overflow warning

[egallager at gcc dot gnu.org]

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

--- Comment #5 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #4) 
> Adding the following just before the definition of the string o prevents the
> warning.  Another alternative is to make both length and the loop control
> variable unsigned (making variables that represent sizes unsigned and
> keeping them that is a good practice since it communicates that constraint
> to the compiler).
> 
> 		if (length < 0)
> 		  __builtin_unreachable ();
> 
> In general, unless the compiler can prove that some expression won't reach a
> value that's invalid in some context, it can substitute the value for it in
> the process of optimizing surrounding code, which can then lead to the
> invalid code materializing even if it's not actually in the original source
> and even if the emitted invalid object code isn't reachable.  GCC warnings
> that are designed to look for this invalid code then trigger on it. 
> Resolving these warnings can often help GCC generate better object code (the
> __builtin_unreachable() trick isn't necessarily always the best way; it's
> only appropriate if the precondition is in the source and GCC loses track of
> it).  Long story short, there is nothing for us to do to.

I wonder if it'd be possible to get the warning to emit a note saying "note:
make this variable unsigned to avoid this warning" or something? It'd be
helpful if the compiler could communicate how to fix warnings like this to the
user so they don't get confused and end up having to file bug reports to find
out what's going on...

(...or maybe add a more general purpose "-Wsuggest-types" flag that suggests
changing the types of variables based on common heuristics such as the one
expressed here? e.g. making sizes unsigned, or possibly using VRP to suggest
the smallest type that can contain all values that the variable can be proved
to ever possibly hold... see for example bug 78798 where I called it
"-Wsuggest-bool")

[Bug tree-optimization/103176] -foptimize-strlen causes stringop-overflow warning

[josiah_vanderzee at mediacombb dot net]

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

--- Comment #6 from Josiah VanderZee <josiah_vanderzee at mediacombb dot net> ---
Thank you, I am grateful that you took your time to explain this so well. I am
sorry I opened a bug report without a deeper investigation into whether it was
definitely a bug.

[Bug tree-optimization/103176] -foptimize-strlen causes stringop-overflow warning

[msebor at gcc dot gnu.org]

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

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
You're welcome and no need to apologize.  We want to improve the warnings (and
the rest of the compiler) and these reports help us understand both the
limitations and opportunities for improvements, or at least get a better sense
of where the pain points for users are.

The challenge with warning offering any type of suggestions is that unless they
are pretty much guaranteed to help they could do more harm then good in the
cases when they don't.  This makes such suggestions pretty much limited to
simple flow-insensitive warnings (those issued by language front ends).