Build error in model_merger in v9.3.0

[aakashb-kayzen]

Describe the bug

The following steps results in the following build error :

Build commands :

cmake -S . -B build -DCMAKE_BUILD_TYPE:STRING="Release" -DCMAKE_TOOLCHAIN_FILE:FILEPATH="./ext_libs/vcpkg/scripts/buildsystems/vcpkg.cmake" -DFMT_SYS_DEP:BOOL="ON" -DRAPIDJSON_SYS_DEP:BOOL="ON" -DSPDLOG_SYS_DEP:BOOL="ON" -DVW_BOOST_MATH_SYS_DEP:BOOL="ON" -DVW_GTEST_SYS_DEP:BOOL="ON" -DVW_ZLIB_SYS_DEP:BOOL="ON" -DBUILD_TESTING:BOOL="OFF" -DBUILD_JAVA:BOOL="ON" -DVW_INSTALL="ON" -DCMAKE_INSTALL_PREFIX=/home/aakash/install

cmake --build build

cmake --install build

Build Error :

/home/platform/aakash/vowpal_wabbit3/vowpalwabbit/model_merger/src/main.cc: In function 'void logger_output_func(void*, VW::io::log_level, const string&)':
/home/platform/aakash/vowpal_wabbit3/vowpalwabbit/model_merger/src/main.cc:50:38: error: no matching function for call to 'std::basic_string<char>::erase(__gnu_cxx::__normal_iterator<char*, std::basic_string<char> >, std::basic_string<char>::const_iterator)'
   50 |       newline_stripped_message.cend());
      |                                      ^
In file included from /opt/rh/devtoolset-9/root/usr/include/c++/9/string:55,
                 from /opt/rh/devtoolset-9/root/usr/include/c++/9/stdexcept:39,
                 from /opt/rh/devtoolset-9/root/usr/include/c++/9/array:39,
                 from /home/platform/aakash/vowpal_wabbit3/vowpalwabbit/common/include/vw/common/vw_exception.h:7,
                 from /home/platform/aakash/vowpal_wabbit3/vowpalwabbit/model_merger/src/main.cc:5:
/opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:4698:7: note: candidate: 'std::basic_string<_CharT, _Traits, _Alloc>& std::basic_string<_CharT, _Traits, _Alloc>::erase(std::basic_string<_CharT, _Traits, _Alloc>::size_type, std::basic_string<_CharT, _Traits, _Alloc>::size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::basic_string<_CharT, _Traits, _Alloc>::size_type = long unsigned int]'
 4698 |       erase(size_type __pos = 0, size_type __n = npos)
      |       ^~~~~
/opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:4698:23: note:   no known conversion for argument 1 from '__gnu_cxx::__normal_iterator<char*, std::basic_string<char> >' to 'std::basic_string<char>::size_type' {aka 'long unsigned int'}
 4698 |       erase(size_type __pos = 0, size_type __n = npos)
      |             ~~~~~~~~~~^~~~~~~~~
/opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:4714:7: note: candidate: 'std::basic_string<_CharT, _Traits, _Alloc>::iterator std::basic_string<_CharT, _Traits, _Alloc>::erase(std::basic_string<_CharT, _Traits, _Alloc>::iterator) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::basic_string<_CharT, _Traits, _Alloc>::iterator = __gnu_cxx::__normal_iterator<char*, std::basic_string<char> >; typename _Alloc::rebind<_CharT>::other::pointer = char*]'
 4714 |       erase(iterator __position)
      |       ^~~~~
/opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:4714:7: note:   candidate expects 1 argument, 2 provided
/opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:4734:7: note: candidate: 'std::basic_string<_CharT, _Traits, _Alloc>::iterator std::basic_string<_CharT, _Traits, _Alloc>::erase(std::basic_string<_CharT, _Traits, _Alloc>::iterator, std::basic_string<_CharT, _Traits, _Alloc>::iterator) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>; std::basic_string<_CharT, _Traits, _Alloc>::iterator = __gnu_cxx::__normal_iterator<char*, std::basic_string<char> >; typename _Alloc::rebind<_CharT>::other::pointer = char*]'
 4734 |       erase(iterator __first, iterator __last);
      |       ^~~~~
/opt/rh/devtoolset-9/root/usr/include/c++/9/bits/basic_string.h:4734:40: note:   no known conversion for argument 2 from '__normal_iterator<const char*,[...]>' to '__normal_iterator<char*,[...]>'
 4734 |       erase(iterator __first, iterator __last);
      |                               ~~~~~~~~~^~~~~~

Exact options passed to compiler :

/opt/rh/devtoolset-9/root/usr/bin/c++ -DFMT_LOCALE -DSPDLOG_FMT_EXTERNAL -D_FILE_OFFSET_BITS=64 -D__extern_always_inline=inline -I/home/platform/aakash/vowpal_wabbit3/vowpalwabbit/core/include -I/home/platform/aakash/vowpal_wabbit3/build/vowpalwabbit/core/include -I/home/platform/aakash/vowpal_wabbit3/vowpalwabbit/common/include -I/home/platform/aakash/vowpal_wabbit3/vowpalwabbit/explore/include -I/home/platform/aakash/vowpal_wabbit3/vowpalwabbit/allreduce/include -I/home/platform/aakash/vowpal_wabbit3/vowpalwabbit/io/include -I/home/platform/aakash/vowpal_wabbit3/vowpalwabbit/config/include -isystem /home/platform/aakash/vowpal_wabbit3/ext_libs/string-view-lite -isystem /home/platform/aakash/vowpal_wabbit3/build/vcpkg_installed/x64-linux/include -O3 -DNDEBUG -fPIE -fvisibility=hidden -fvisibility-inlines-hidden -Wall -Wextra -Wpedantic -fno-strict-aliasing -msse2 -mfpmath=sse -fno-stack-check -fomit-frame-pointer -pthread -std=c++11 -MD -MT vowpalwabbit/model_merger/CMakeFiles/vw_model_merger_bin.dir/src/main.cc.o -MF CMakeFiles/vw_model_merger_bin.dir/src/main.cc.o.d -o CMakeFiles/vw_model_merger_bin.dir/src/main.cc.o -c /home/platform/aakash/vowpal_wabbit3/vowpalwabbit/model_merger/src/main.cc

The issue is that argument 1 of std::basic_string::erase is of type iterator whereas argument 2 is of type 'const_iterator. It is expected that both should either be of type iteratororconst_iterator`, but mixing both is not allowed.

Please provide guidance on how to compile model_merger.

How to reproduce

cmake -S . -B build -DCMAKE_BUILD_TYPE:STRING="Release" -DCMAKE_TOOLCHAIN_FILE:FILEPATH="./ext_libs/vcpkg/scripts/buildsystems/vcpkg.cmake" -DFMT_SYS_DEP:BOOL="ON" -DRAPIDJSON_SYS_DEP:BOOL="ON" -DSPDLOG_SYS_DEP:BOOL="ON" -DVW_BOOST_MATH_SYS_DEP:BOOL="ON" -DVW_GTEST_SYS_DEP:BOOL="ON" -DVW_ZLIB_SYS_DEP:BOOL="ON" -DBUILD_TESTING:BOOL="OFF" -DBUILD_JAVA:BOOL="ON" -DVW_INSTALL="ON" -DCMAKE_INSTALL_PREFIX=/home/aakash/install

cmake --build build

cmake --install build

Version

36e2335b5234562c1d3c9f71fc6c2936840c45b0

OS

CentOS 7.9.2009 (Core)

Language

C++

Additional context

No response

[jackgerrits]

Looks like a typo in the erase call https://github.com/VowpalWabbit/vowpal_wabbit/blob/5129996594be56359495c6d9f78703495030d6c0/vowpalwabbit/model_merger/src/main.cc#L50

Can you replace this line with:

      newline_stripped_message.end());

Let me know if that fixes the issue for you

[aakashb-kayzen]

Yes, this fixes the issue for me. I wonder how this compiles till now, given 9.3.0 is already released. We should either add this change to mainline or upgrade c++ version to 20 in the CMake configuration files.

[jackgerrits]

Are you aware if there was a change introduced in C++20 that would have caused it?

What compiler and version are you using?

[aakashb-kayzen]

I see the issue has been fixed, thanks for that.

For future documentation, the issue is currently reproducible only on RHEL/CentOS for all versions of gcc >= 9. This is due to the compiler macro _GLIBCXX_USE_CXX11_ABI being forcefully set to 0 in RHEL/CentOS as described here. More about the macro here.

However, as far as I understand, this flag only changes the ABI and all C++11 STL methods should still be available. I am still trying to understand whether this is a compiler bug or an expected behaviour through my stackoverflow question here.