[Bug c++/106434] New: Spurious -Wnull-dereference when using std::unique_copy()

[Bug c++/106434] New: Spurious -Wnull-dereference when using std::unique_copy()

[vz-gcc at zeitlins dot org]

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

            Bug ID: 106434
           Summary: Spurious -Wnull-dereference when using
                    std::unique_copy()
           Product: gcc
           Version: 12.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: vz-gcc at zeitlins dot org
  Target Milestone: ---

The following simple example shows the problem with g++ 12, which didn't exist
with the previous versions:

% g++ -v
Using built-in specs.
COLLECT_GCC=/usr/bin/g++
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/12/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none:amdgcn-amdhsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 12.1.0-7'
--with-bugurl=file:///usr/share/doc/gcc-12/README.Bugs
--enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,m2 --prefix=/usr
--with-gcc-major-version-only --program-suffix=-12
--program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id
--libexecdir=/usr/lib --without-included-gettext --enable-threads=posix
--libdir=/usr/lib --enable-nls --enable-clocale=gnu --enable-libstdcxx-debug
--enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new
--enable-gnu-unique-object --disable-vtable-verify --enable-plugin
--enable-default-pie --with-system-zlib --enable-libphobos-checking=release
--with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch
--disable-werror --enable-cet --with-arch-32=i686 --with-abi=m64
--with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic
--enable-offload-targets=nvptx-none=/build/gcc-12-aYRw0H/gcc-12-12.1.0/debian/tmp-nvptx/usr,amdgcn-amdhsa=/build/gcc-12-aYRw0H/gcc-12-12.1.0/debian/tmp-gcn/usr
--enable-offload-defaulted --without-cuda-driver --enable-checking=release
--build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 12.1.0 (Debian 12.1.0-7)
% cat unique_copy.cpp
#include <algorithm>
#include <iterator>
#include <string>
#include <vector>

std::vector<std::string> const& parms();

int main() {
    std::vector<std::string> all;

    for(auto const& i : parms()) {
        all.push_back(i);
    }

    std::vector<std::string> unique;
    std::insert_iterator iin(unique, unique.begin()); // EXCHANGE THESE LINES
    std::sort(all.begin(), all.end());                // TO SUPPRESS THE
WARNING
    std::unique_copy(all.begin(), all.end(), iin);
}
% g++ -std=c++20 -c -O2 -Wnull-dereference unique_copy.cpp
In file included from /usr/include/c++/12/string:53,
                 from /usr/include/c++/12/bits/locale_classes.h:40,
                 from /usr/include/c++/12/bits/ios_base.h:41,
                 from /usr/include/c++/12/streambuf:41,
                 from /usr/include/c++/12/bits/streambuf_iterator.h:35,
                 from /usr/include/c++/12/iterator:66,
                 from unique_copy.cpp:2:
In constructor ‘constexpr std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::_Alloc_hider::_Alloc_hider(std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::pointer, _Alloc&&) [with _CharT = char; _Traits =
std::char_traits<char>; _Alloc = std::allocator<char>]’,
    inlined from ‘constexpr std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>::basic_string(const std::__cxx11::basic_string<_CharT, _Traits,
_Alloc>&) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc =
std::allocator<char> ’ at /usr/include/c++/12/bits/basic_string.h:540:9,
    inlined from ‘constexpr decltype (::new(void*(0)) _Tp)
std::construct_at(_Tp*, _Args&& ...) [with _Tp = __cxx11::basic_string<char>;
_Args = {const __cxx11::basic_string<char, char_traits<char>, allocator<char>
>&}]’ at /usr/include/c++/12/bits/stl_construct.h:97:14,
    inlined from ‘static constexpr void
std::allocator_traits<std::allocator<_CharT> >::construct(allocator_type&,
_Up*, _Args&& ...) [with _Up = std::__cxx11::basic_string<char>; _Args = {const
std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>
>&}; _Tp = std::__cxx11::basic_string<char>]’ at
/usr/include/c++/12/bits/alloc_traits.h:518:21,
    inlined from ‘constexpr std::vector<_Tp, _Alloc>::iterator std::vector<_Tp,
_Alloc>::insert(const_iterator, const value_type&) [with _Tp =
std::__cxx11::basic_string<char>; _Alloc =
std::allocator<std::__cxx11::basic_string<char> >]’ at
/usr/include/c++/12/bits/vector.tcc:145:30,
    inlined from ‘constexpr std::insert_iterator<_Container>&
std::insert_iterator<_Container>::operator=(const typename
_Container::value_type&) [with _Container =
std::vector<std::__cxx11::basic_string<char> >]’ at
/usr/include/c++/12/bits/stl_iterator.h:964:26,
    inlined from ‘constexpr _OutputIterator
std::__unique_copy(_ForwardIterator, _ForwardIterator, _OutputIterator,
_BinaryPredicate, forward_iterator_tag, output_iterator_tag) [with
_ForwardIterator = __gnu_cxx::__normal_iterator<__cxx11::basic_string<char>*,
vector<__cxx11::basic_string<char> > >; _OutputIterator =
insert_iterator<vector<__cxx11::basic_string<char> > >; _BinaryPredicate =
__gnu_cxx::__ops::_Iter_equal_to_iter]’ at
/usr/include/c++/12/bits/stl_algo.h:1002:17,
    inlined from ‘constexpr _OIter std::unique_copy(_IIter, _IIter, _OIter)
[with _IIter = __gnu_cxx::__normal_iterator<__cxx11::basic_string<char>*,
vector<__cxx11::basic_string<char> > >; _OIter =
insert_iterator<vector<__cxx11::basic_string<char> > >]’ at
/usr/include/c++/12/bits/stl_algo.h:4474:32,
    inlined from ‘int main()’ at unique_copy.cpp:18:21:
/usr/include/c++/12/bits/basic_string.h:204:43: warning: null pointer
dereference [-Wnull-dereference]
  204 |         : allocator_type(std::move(__a)), _M_p(__dat) { }
      |                                           ^~~~~~~~~~~
[...several more pages of similar output with more -Wnull-dereference
occurrences snipped...]


I could work around this bug by simply moving the call to std::sort() before
insert_iterator ctor (i.e. just exchanging the marked lines avoids the
warning), but it looks wrong to have to use such voodoo just to make my real
code compile without warnings again.

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Spurious -Wnull-dereference |[12/13 Regression] Spurious
                   |when using                  |-Wnull-dereference when
                   |std::unique_copy()          |using std::unique_copy()
                   |                            |since
                   |                            |r12-5187-g1ae8edf5f73ca5c3
                 CC|                            |jwakely.gcc at gmail dot com,
                   |                            |marxin at gcc dot gnu.org

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with r12-5187-g1ae8edf5f73ca5c3.

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-08-12
             Status|UNCONFIRMED                 |NEW
           Priority|P3                          |P2
     Ever confirmed|0                           |1
   Target Milestone|---                         |12.2

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[joshua.r.marshall.1991 at gmail dot com]

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

--- Comment #2 from Josh Marshall <joshua.r.marshall.1991 at gmail dot com> ---
How involved should I be on this?

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Josh Marshall from comment #2)
> How involved should I be on this?

You can ignore it.

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This is a compiler bug, not a library bug, and a reduced testcase will fail
before r12-5187 because it was already latent in the compiler.

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
We diagnose

MEM[(struct _Alloc_hider *)0B]._M_p = 16B;

that's conditional on if (_178 == 0B) so probably constant propagated from
that.  That on the other hand is conditional on if (_178 != _179) with

_178 = unique.D.69649._M_impl.D.68955._M_finish;
_179 = unique.D.69649._M_impl.D.68955._M_end_of_storage;

from vector::insert:

    insert(iterator __position, const value_type& __x)
#endif
    { 
      const size_type __n = __position - begin();
      if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage)
        if (__position == end())
          { 
            _GLIBCXX_ASAN_ANNOTATE_GROW(1);
            _Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
                                     __x);
            ++this->_M_impl._M_finish;
            _GLIBCXX_ASAN_ANNOTATE_GREW(1);

for some reason __position is NULL here, from iin.iter._M_current = 0B; it
seems.

I'm a bit lost here.

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
It looks like 129t.thread1 jump threaded things in a way to expose this case
(which is probably unreachable, but ...).

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[redi at gcc dot gnu.org]

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

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
So initially iin.iter._M_current is indeed null, because unique.begin() returns
_M_start (which is null) when the vector is empty:

  std::insert_iterator iin(unique, unique.begin());

But when the vector is empty, this condition is always false:

      if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage)
        if (__position == end())

It's impossible for _M_start to be null unless _M_finish and _M_end_of_storage
are also null.

After the first insertion into the vector all three of _M_start, _M_finish and
_M_end_of_storage are non-null. But after the first insertion
iin.iter._M_current is also non-null.

So we need to add a hint so the compiler knows that the jump threaded
"__position is null but finish != end_of_storage" case is nonsense. It's not
_impossible_, because a dumb user could make it happen, but it violates the
function precondition so is UB.

i.e. this would take that code path:

std::vector<int> v{1,2,3}; // non-empty vector
std::vector<int>::const_iterator null;
v.insert(null, 1); // try to insert at invalid position

But that's UB.


This seems to work (the __glibcxx_assert isn't needed to stop the warning, but
might be a useful assertion).


--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -137,8 +137,14 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
     insert(iterator __position, const value_type& __x)
 #endif
     {
+      __glibcxx_assert(capacity() == 0 || __position != const_iterator());
+
       const size_type __n = __position - begin();
       if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage)
+        {
+          if (__position == const_iterator())
+            __builtin_unreachable();
+
        if (__position == end())
          {
            _GLIBCXX_ASAN_ANNOTATE_GROW(1);
@@ -159,6 +165,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
            _M_insert_aux(__position, __x);
 #endif
          }
+      }
       else
 #if __cplusplus >= 201103L
        _M_realloc_insert(begin() + (__position - cbegin()), __x);

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #8)
> This seems to work (the __glibcxx_assert isn't needed to stop the warning,
> but might be a useful assertion).

Well, it might be if I got the condition right. Doh.

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[redi at gcc dot gnu.org]

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

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Slightly simpler, with correct assertion now:

--- a/libstdc++-v3/include/bits/vector.tcc
+++ b/libstdc++-v3/include/bits/vector.tcc
@@ -139,6 +139,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
     {
       const size_type __n = __position - begin();
       if (this->_M_impl._M_finish != this->_M_impl._M_end_of_storage)
+       {
+         __glibcxx_assert(__position != const_iterator());
+         if (__position == const_iterator())
+           __builtin_unreachable(); // PR 106434
+
          if (__position == end())
            {
              _GLIBCXX_ASAN_ANNOTATE_GROW(1);
@@ -159,6 +164,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
              _M_insert_aux(__position, __x);
 #endif
            }
+       }
       else
 #if __cplusplus >= 201103L
        _M_realloc_insert(begin() + (__position - cbegin()), __x);


The __builtin_unreachable would be unnecessary if __glibcxx_assert expanded to
that when assertions are disabled (so the macro expands to either an assertion,
or a hint that the condition is always true). I think I tried that and it
caused problems though.

[Bug c++/106434] [12/13 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:4ba94abf147fe7778a7541849ce27cafee74df9b

commit r13-4525-g4ba94abf147fe7778a7541849ce27cafee74df9b
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Mon Dec 5 12:53:42 2022 +0000

    libstdc++: Add hint to compiler about vector invariants [PR106434]

    The PR shows a bogus warning where jump threading generates code for the
    undefined case that the insertion point is a value-initialized iterator
    but _M_finish and _M_end_of_storage are unequal (so at least one must be
    non-null). Using __builtin_unreachable() removes the bogus warning. Also
    add an assertion to diagnose undefined misuses of a null iterator here,
    so we don't just silently optimize that undefined code to something
    unsafe.

    libstdc++-v3/ChangeLog:

            PR c++/106434
            * include/bits/vector.tcc (insert(const_iterator, const T&)):
            Add assertion and optimization hint that the iterator for the
            insertion point must be non-null.

[Bug c++/106434] [12 Regression] Spurious -Wnull-dereference when using std::unique_copy() since r12-5187-g1ae8edf5f73ca5c3

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.3                        |12.4

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 12.3 is being released, retargeting bugs to GCC 12.4.