p-ranav / argparse

Argument Parser for Modern C++
MIT License
2.72k stars 250 forks source link

`-Wrestrict` warning on Alpine 3.18, using GCC 12 and C++20, unclear if actual bug #358

Closed ericonr closed 6 months ago

ericonr commented 6 months ago

Hi!

I noticed an error with my project which uses argparse when updating the compile flags, though I was using an older version of argparse. I tried reproducing it with the version from master and it still happened. I didn't dig into the implementation headers where the error cropped up, but I was able to "fix" it in argparse itself. It feels like a compiler bug, but I'd appreciate a second look before opening one (and it doesn't happen with Clang).

In file included from /usr/include/c++/12.2.1/string:40,
                 from /usr/include/c++/12.2.1/bits/locale_classes.h:40,
                 from /usr/include/c++/12.2.1/bits/ios_base.h:41,
                 from /usr/include/c++/12.2.1/iomanip:40,
                 from include/argparse/argparse.hpp:43,
                 from main.cpp:1:
In static member function 'static constexpr std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)',
    inlined from 'static constexpr void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_S_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.h:423:21,
    inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Allocator>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.tcc:532:22,
    inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.h:2171:19,
    inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::insert(size_type, const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.h:1928:22,
    inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Allocator> std::operator+(const _CharT*, __cxx11::basic_string<_CharT, _Traits, _Allocator>&&) [with _CharT = char; _Traits = char_traits<char>; _Alloc = allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.h:3541:36,
    inlined from 'void argparse::ArgumentParser::parse_args(const std::vector<std::__cxx11::basic_string<char> >&)' at include/argparse/argparse.hpp:1834:35:
/usr/include/c++/12.2.1/bits/char_traits.h:431:56: warning: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' accessing 9223372036854775810 or more bytes at offsets [2, 9223372036854775807] and 1 may overlap up to 9223372036854775813 bytes at offset -3 [-Wrestrict]
  431 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
      |                                        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
In static member function 'static constexpr std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)',
    inlined from 'static constexpr void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_S_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.h:423:21,
    inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Allocator>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.tcc:532:22,
    inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.h:2171:19,
    inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::insert(size_type, const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.h:1928:22,
    inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Allocator> std::operator+(const _CharT*, __cxx11::basic_string<_CharT, _Traits, _Allocator>&&) [with _CharT = char; _Traits = char_traits<char>; _Alloc = allocator<char>]' at /usr/include/c++/12.2.1/bits/basic_string.h:3541:36,
    inlined from 'void argparse::ArgumentParser::parse_args(const std::vector<std::__cxx11::basic_string<char> >&)' at include/argparse/argparse.hpp:1836:35:
/usr/include/c++/12.2.1/bits/char_traits.h:431:56: warning: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' accessing 9223372036854775810 or more bytes at offsets [2, 9223372036854775807] and 1 may overlap up to 9223372036854775813 bytes at offset -3 [-Wrestrict]
  431 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
      |                                        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

In order to reproduce it, you need:

main.cpp:

#include "include/argparse/argparse.hpp"

int main(int argc, char *argv[])
{
    argparse::ArgumentParser args("bench-cp", "1.0", argparse::default_arguments::help);
    args.add_argument("-b").help("device number").required();

    args.parse_args(argc, argv);
}

Makefile with the base set of flags I needed to reproduce it:

CXXFLAGS  = -Wall -std=c++20 -O3

main:

The patch to fix it, which I believe simply changes the operator used:

diff --git a/include/argparse/argparse.hpp b/include/argparse/argparse.hpp
index 921a4b4..1269863 100644
--- a/include/argparse/argparse.hpp
+++ b/include/argparse/argparse.hpp
@@ -1831,9 +1831,9 @@ public:
         for (Argument *arg : group.m_elements) {
           if (i + 1 == size) {
             // last
-            argument_names += "'" + arg->get_usage_full() + "' ";
+            argument_names += std::string("'") + arg->get_usage_full() + std::string("' ");
           } else {
-            argument_names += "'" + arg->get_usage_full() + "' or ";
+            argument_names += std::string("'") + arg->get_usage_full() + std::string("' or ");
           }
           i += 1;
         }
ericonr commented 6 months ago

Found out it's already been reported https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105329 from the link in https://github.com/open-telemetry/opentelemetry-cpp/issues/2134

Would you like to apply my suggested fix (which I think can be simplified to the first "'" string for both expressions), or should it be considered a compiler bug and the issue closed?

p-ranav commented 6 months ago

I'll happily accept the PR :) Thanks for the report, and for your findings!