[Bug tree-optimization/94335] New: False positive -Wstringop-overflow warning with -O2

[Bug tree-optimization/94335] New: False positive -Wstringop-overflow warning with -O2

[romain.geissler at amadeus dot com]

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

            Bug ID: 94335
           Summary: False positive -Wstringop-overflow warning with -O2
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: romain.geissler at amadeus dot com
  Target Milestone: ---

Hi,

The following code emits a false positive -Wstringop-overflow warning with gcc
trunk with -O2 and -O3.

It's reduced manually from a much more complex scenario, where we try to define
an iterator that can be stored in shared memory, thus referencing object
through relative addresses rather than absolute addresses. In the warning of
this scenario, I don't see how we can access offset -424242 as the initial
pointed address is never nullptr.

Note: gcc 8/9 do not emit such warning.


#include <iterator>
#include <memory>
#include <cassert>
#include <limits>
#include <cstring>

template <class T>
struct relative_address_iterator {
    typedef T value;
    typedef T *pointer;
    typedef const T *const_pointer;
    typedef T &reference;
    typedef const T &const_reference;

    typedef T value_type;
    typedef ptrdiff_t difference_type;
    typedef std::forward_iterator_tag iterator_category;


    off_t d;
    static constexpr off_t kEmptyPointer = std::numeric_limits<off_t>::min();

    /** Constructs the object from a pointer.
     */
    relative_address_iterator(pointer t)
    {
        if (t)
        {
            d = (char *)t - (char *)this;
        }   
        else
        {
            d = kEmptyPointer;
        }   
    }   

    /**  Copy constructor.
     */
    relative_address_iterator(const relative_address_iterator &r)
    {
        if (r.d != kEmptyPointer)
        {
            d = (char *)&r - (char *)this + r.d;
        }   
        else
        {
            // Normally this should not happen in this example, but gcc thinks
it does,
            // as the error message references the special value -424242.
            // d = kEmptyPointer;
            d = -424242;
        }   
    }   

    /** Initializes the pointee with the given value.
     */
    void setVal(const_reference value) { *(pointer)((char *)this + d) = value;
}

    /** Dereferences the object.
     */
    operator pointer()
    {
        if (d == kEmptyPointer) return nullptr;
        return ((pointer)((char *)this + d));
    }   

    /** Preincrement operator.
     */
    relative_address_iterator &operator++()
    {
        d += sizeof(value);
        return (*this);
    }   
};  

void f()
{
    char* aDummyBuffer = static_cast<char*>(malloc(10));
    memset(aDummyBuffer, 10, 0);
    relative_address_iterator<char> it(aDummyBuffer);
    relative_address_iterator<char> itCopy(it);

    itCopy.setVal('A');

    char* aDummySource = static_cast<char*>(malloc(10));
    memset(aDummySource, 10, 0);
    std::copy(relative_address_iterator<char>(aDummySource),
            relative_address_iterator<char>(aDummySource + 5),
            it);
}           

int main()  
{
    f();
}



In member function 'void
relative_address_iterator<T>::setVal(relative_address_iterator<T>::const_reference)
[with T = char]',
    inlined from 'void f()' at <source>:82:18:
<source>:56:71: warning: writing 1 byte into a region of size 0
[-Wstringop-overflow=]
   56 |     void setVal(const_reference value) { *(pointer)((char *)this + d) =
value; }
      |                                         
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
<source>: In function 'void f()':
<source>:80:37: note: at offset -424242 to object 'itCopy' with size 8 declared
here
   80 |     relative_address_iterator<char> itCopy(it);
      |                                     ^~~~~~
Compiler returned: 0


Cheers,
Romain

[Bug tree-optimization/94335] False positive -Wstringop-overflow warning with -O2

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #1 from Martin Sebor <msebor at gcc dot gnu.org> ---
This type of warning is new GCC 10; it was added in the commit below.  It works
as designed here.  It sees the following IL (the memset calls don't do
anything).  The MEM[] = 65; statement is what triggers it.

  <bb 2> [local count: 1073741824]:
  aDummyBuffer_4 = malloc (10);
  it ={v} {CLOBBER};
  if (aDummyBuffer_4 != 0B)
    goto <bb 3>; [70.00%]
  else
    goto <bb 18>; [30.00%]

  <bb 3> [local count: 751619281]:
  _24 = aDummyBuffer_4 - &it;
  it.d = _24;
  itCopy ={v} {CLOBBER};
  if (_24 != -9223372036854775808)
    goto <bb 4>; [94.29%]
  else
    goto <bb 5>; [5.71%]

  <bb 4> [local count: 708669601]:
  _23 = aDummyBuffer_4 - &itCopy;
  itCopy.d = _23;
  *aDummyBuffer_4 = 65;
  aDummySource_97 = malloc (10);
  D.40357 ={v} {CLOBBER};
  _17 = aDummyBuffer_4 - &D.40357;
  D.40357.d = _17;
  goto <bb 6>; [100.00%]

  <bb 5> [local count: 365072224]:
  itCopy.d = -424242;
  MEM[(char *)&itCopy + -424242B] = 65;   <<< warning here
  aDummySource_105 = malloc (10);
  D.40357 ={v} {CLOBBER};
  D.40357.d = -424242;
  ...
  <bb 18> [local count: 322122544]:
  it.d = -9223372036854775808;
  itCopy ={v} {CLOBBER};
  goto <bb 5>; [100.00%]


It doesn't matter (much) whether the initial address is or can be null (the
warning persists even with operator new that doesn't return null or when the
ctor never does set d to  kEmptyPointer).  The branch of the code that sets d
to -424242 isn't eliminated because the pointer subtraction in either ctor
could, as far as GCC can tell, result in the same value as kEmptyPointer.

Asserting that the subtraction doesn't result in such a value, for instance
like so:
            if (d == kEmptyPointer) __builtin_unreachable ();
and also guaranteeing that the initial address isn't null (e.g., by using
operator new) eliminates the warning.

Short of teaching GCC that the magnitude of the difference between any two
pointers must be less than PTRDIFF_MAX I don't think there's anything that can
be done do improve things (either codegen, or avoid the warning in this case). 
 I'll leave this report unresolved in case someone feels otherwise.

commit b631bdb3c16e85f35d38e39b3d315c35e4a5747c
Author: Martin Sebor <msebor@redhat.com>
Date:   Thu Jul 25 00:29:17 2019 +0000

    PR tree-optimization/91183 - strlen of a strcpy result with a conditional
source not folded

    PR tree-optimization/91183 - strlen of a strcpy result with a conditional
source not folded
    PR tree-optimization/86688 - missing -Wstringop-overflow using a non-string
local array in strnlen with excessive bound

[Bug tree-optimization/94335] False positive -Wstringop-overflow warning with -O2

[romain.geissler at amadeus dot com]

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

--- Comment #2 from Romain Geissler <romain.geissler at amadeus dot com> ---
Thanks for the explanation.

However few observations:
 - Is it really expected that the wording of the warning seems to imply gcc
knows for sure that there is an invalid access ? What is the warning meant to
find ? Potential issues, or real issues gcc can prove will happen 100% of the
time ? Here I see that you pasted:

  if (_24 != -9223372036854775808)
    goto <bb 4>; [94.29%]
  else
    goto <bb 5>; [5.71%]

so gcc itself is able to see that the most likely case is that we go in basic
block 4 rather than 5 which emits this warning.

So either the wording of the warning shall be updated to reflect that "maybe"
the code wrong, either if possible we should try to make a different when gcc
is sure and when it is not (like -Wuninitialized vs -Wmaybe-uninitialized).

 - Second observations, how do you suggest we teach gcc that this is indeed a
false positive and we want the warning to be silenced. You mentioned "if (d ==
kEmptyPointer) __builtin_unreachable;", does this statement result in
effectively having a new basic block branching where one side terminates the
program and the other runs the actual expected behavior of "setVal". In other
words, will the condition in the "if" really be emitted by the codegen and
evaluated at runtime, or do I have the guarantee it will not emit new branches
and only teach the optimizer that some cases are impossible ? In the case the
answer is that it will emit a real codegen branch potentially impacting
runtime, do you think it's worth adding a __builtin_assume in gcc like clang
has (not in gcc 10 obviously, but in gcc 11 during stage 1) ?

Cheers,
Romain

[Bug tree-optimization/94335] False positive -Wstringop-overflow warning with -O2

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
You should be using (intptr_t)t - (intptr_t)this when computing the relative
pointer, not sure if that makes a difference but pointer difference between
pointers to different objects invokes undefined behavior.

[Bug tree-optimization/94335] False positive -Wstringop-overflow warning with -O2

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |marxin at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-03-26
             Status|UNCONFIRMED                 |NEW

[Bug tree-optimization/94335] False positive -Wstringop-overflow warning with -O2

[romain.geissler at amadeus dot com]

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

--- Comment #4 from Romain Geissler <romain.geissler at amadeus dot com> ---
Thanks Richard.

Indeed, beyond the false positive described in this bug, our whole code that
implement "relative pointer" is I think quite hacky and not very compiler
friendly (around alignment, aliasing rule, pointer arithmetic). We should
review and update it a lot. Actually a similar class exists in
Boost.Interprocess under the name "offset_ptr", and they did write this in
their header:
https://github.com/boostorg/interprocess/blob/develop/include/boost/interprocess/offset_ptr.hpp

//Note: using the address of a local variable to point to another address
//is not standard conforming and this can be optimized-away by the compiler.
//Non-inlining is a method to remain illegal but correct

//Undef BOOST_INTERPROCESS_OFFSET_PTR_INLINE_XXX if your compiler can inline
//this code without breaking the library

any time they need to deal with pointer to offset conversion, or vice-versa. I
happens that we also suffer from similar problems and had to put attribute
"noinline" "randomly" in places to "fix" (actually workaround our poor
understanding of how the compiler works) problems we are seeing with the
behavior of this library when compiled with optimizations. We should obviously
review greatly in depths what we are doing which seems wrong in many places.

PS Martin: I am ok to leave unresolved, just I think the wording of the error
should be somehow updated so that the fact that it's only a possibility for not
a certainty, other of your warnings around string management do print the range
of value that gcc found out is possible, and that helps in understanding and
fixing/silencing the warnings.

[Bug tree-optimization/94335] False positive -Wstringop-overflow warning with -O2

[msebor at gcc dot gnu.org]

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

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
Few middle-end warnings consider control flow -- most simply look at a single
statement at a time and consider ranges of argument values (if any).  Those
that do consider control flow (e.g., -Wreturn-local-addr) only do so for PHI
nodes that, for the most part, are the result of ?: expressions.  We have work
underway to improve the way GCC computes and exposes range information that we
expect will help improve the accuracy in cases where there's more than a single
range.  I've also been thinking about changing some existing warnings to
consider control flow to improve both their accuracy and efficacy, but this
would only make a difference for PHI nodes.

I haven't thought too much about using branch probabilities to decide whether
to issue a maybe kind of a warning.  Right now, a statement in the IL with a
constant out-of-bounds argument triggers a definitive warning regardless of how
likely the branch it's in is executed.  It might be something to explore,
though I would expect it to quickly turn most warnings in non-trivial code into
the maybe kind.

Using 'if (condition) __builtin_unreachable ();' shouldn't have an adverse
effect on efficiency.  Rather I'd expect it to improve codegen since it tells
GCC that and any code that would otherwise be reachable after it can be
eliminated.  I would expect its effects to be comparable to __builtin_assume
(condition).

[Bug tree-optimization/94335] False positive -Wstringop-overflow warning with -O2

[kal.conley at dectris dot com]

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

kal.conley at dectris dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kal.conley at dectris dot com

--- Comment #6 from kal.conley at dectris dot com ---
We are hitting this warning too with:

#include <stdint.h>
#include <vector>

int main() {
    std::vector<uint64_t> inputs(2);
    std::vector<uint8_t> outputs{inputs.begin(), inputs.end()};
    outputs.back() = 1;
    return 0;
}

Regards,
Kal

[Bug tree-optimization/94335] [10/11 Regression] False positive -Wstringop-overflow warning with -O2

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |10.1.0, 11.0
             Blocks|                            |88443
            Summary|False positive              |[10/11 Regression] False
                   |-Wstringop-overflow warning |positive
                   |with -O2                    |-Wstringop-overflow warning
                   |                            |with -O2

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
Thanks for the small test case!  The warning for reference is:

pr94335-c6.C: In function ‘int main()’:
pr94335-c6.C:8:20: warning: writing 1 byte into a region of size 0
[-Wstringop-overflow=]
    8 |     outputs.back() = 1;
      |     ~~~~~~~~~~~~~~~^~~
In file included from
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu/bits/c++allocator.h:33,
                 from
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/bits/allocator.h:46,
                 from
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/vector:64,
                 from pr94335-c6.C:3:
/build/gcc-master/x86_64-pc-linux-gnu/libstdc++-v3/include/ext/new_allocator.h:115:41:
note: at offset 0 to an object with size 0 allocated by ‘operator new’ here
  115 |  return static_cast<_Tp*>(::operator new(__n * sizeof(_Tp)));
      |                           ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~

The warning in this case is actually due to a different problem, this one in
the warning infrastructure itself.  The relevant IL the warning works with
(-fdump-tree-strlen) is below:

main ()
{
  ...
  unsigned char * _48;
  ...
  unsigned char * _59;
  ...
  <bb 2> [local count: 1073741825]:
  ...
  _59 = operator new (2);

  <bb 3> [local count: 1073007519]:
  outputs.D.19139._M_impl.D.18482._M_start = _59;
  _48 = _59 + 2;
  ...
  MEM[(value_type &)_48 + 18446744073709551615] = 1;   <<< warning here
(18446744073709551615 == -2)
  ...
}

To determine the size of what _48 points to the warning code calls the
compute_objsize() function.  It returns zero because of a design limitation: it
returns the size of the remaining space in the object (i.e., the full size of
the pointed-to object minus the offset, which is 2 in this case).  The caller
has no way to increase the size it gets back by its negative offset (the large
number which is -2).

I have a rewritten the function to avoid this (and other problems with it) and
expect to have a fix for GCC 11, and possibly even for GCC 10.2.

Since this is a different problem than the originally reported bug I don't want
to use it to track it.  Feel free to open a separate bug for it.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88443
[Bug 88443] [meta-bug] bogus/missing -Wstringop-overflow warnings

[Bug tree-optimization/94335] [10/11 Regression] False positive -Wstringop-overflow warning with -O2

[msebor at gcc dot gnu.org]

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

--- Comment #8 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to kal.conley from comment #6)

For reference, this was also submitted as pr95353 and is now fixed on trunk
(GCC 11).

The test case in comment #0 still triggers a warning.

[Bug tree-optimization/94335] [10/11 Regression] False positive -Wstringop-overflow warning with -O2

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
   Target Milestone|---                         |10.2

[Bug tree-optimization/94335] [10/11 Regression] False positive -Wstringop-overflow warning with -O2

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.2                        |10.3

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10.2 is released, adjusting target milestone.

[Bug tree-optimization/94335] [10/11 Regression] False positive -Wstringop-overflow warning with -O2

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|2020-03-26 00:00:00         |2021-1-21

--- Comment #10 from Martin Sebor <msebor at gcc dot gnu.org> ---
No change in GCC 11.

[Bug tree-optimization/94335] [10/11 Regression] False positive -Wstringop-overflow warning with -O2

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.3                        |10.4

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10.3 is being released, retargeting bugs to GCC 10.4.

[Bug tree-optimization/94335] [10/11/12 Regression] False positive -Wstringop-overflow warning with -O2

[pinskia at gcc dot gnu.org]

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

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The warning for the one in comment #0 is gone on the trunk
The warning for the testcase in comment #6 is gone in GCC 11.1.0.

[Bug tree-optimization/94335] [10/11/12 Regression] False positive -Wstringop-overflow warning with -O2

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=102216

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #12)
> The warning for the testcase in comment #6 is gone in GCC 11.1.0.

But that looks like on accident (there is some early inlining differences) and
not really fixing the problem and there becomes a missing optimization.  I Have
a patch for the missed optimization (it is the same as PR 102216).

[Bug tree-optimization/94335] [10/11/12 Regression] False positive -Wstringop-overflow warning with -O2

[pinskia at gcc dot gnu.org]

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

--- Comment #14 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to kal.conley from comment #6)
> We are hitting this warning too with:

The missed optimization in this one was fixed with r12-5465-g911b633803dcbb298.

[Bug tree-optimization/94335] [10/11/12/13 Regression] False positive -Wstringop-overflow warning with -O2

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.4                        |10.5

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 10.4 is being released, retargeting bugs to GCC 10.5.

[Bug tree-optimization/94335] [11/12/13/14 Regression] False positive -Wstringop-overflow warning with -O2

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |11.5

--- Comment #16 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10 branch is being closed.

[Bug tree-optimization/94335] [11/12/13/14 Regression] False positive -Wstringop-overflow warning with -O2

[law at gcc dot gnu.org]

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

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |law at gcc dot gnu.org
         Resolution|---                         |FIXED

--- Comment #17 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Per c#12, c#13, c#14.