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
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.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
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
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.
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.
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).
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
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.
(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.
GCC 10.2 is released, adjusting target milestone.
No change in GCC 11.
GCC 10.3 is being released, retargeting bugs to GCC 10.4.
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.
(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).
(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.
GCC 10.4 is being released, retargeting bugs to GCC 10.5.
GCC 10 branch is being closed.
Per c#12, c#13, c#14.