Re: [PATCH RFC] Require c++17 compiler

[Andrew Burgess <aburgess@redhat.com>]

Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> writes:

> Hi Tom,
>
> Thanks for putting this together!  As you know, I am in favor of such
> change (for GDB-15 probably, GDB-14 branching is probably too close)!
>
> I do have a really similar patch available on a local branch.  One
> difference though is that my branch uses a preparatory patch to upgrade
> gdb/ax_cxx_compile_stdcxx.m4 to follow upstream changes to this file.
> This upgrade gives some improvements in the C++17 compiler detection.
> We keep this in the GDB tree because it has some local changes to set
> CXX_DIALECT.  This preparatory patch should be included in this reply.
>
> On Thu, Oct 05, 2023 at 08:54:49AM +0200, Tom de Vries wrote:
>> Since gdb 8.0, we've required a c++11 compiler.
>> 
>> That allowed gdb to be compiled with gcc releases as far back as gcc 4.8,
>> using an explicit -std=c++11 to override the default.
>> 
>> [ Gcc has the following defaults:
>> - before gcc 6: c++98, and
>> - for gcc 6-10: c++14, and
>> - since gcc 11: c++17. ]
>> 
>> There are two arguments in favor of moving to requiring a newer c++ standard:
>> - benefits can be had from using newer c++ features.  We're already using some
>>   of those features using gdb-specific implementations.
>> - when developing gdb on modern platforms with system compiler gcc >= 6,
>>   people can accidentally use c++14/c++17 features in a patch, only to find out
>>   post-commit that it breaks the build on a system with a compiler that only
>>   supports c++11, which is inconvenient and takes time to fix.
>>   [ This could be partially mitigated by if possible also forcing -std=c++11
>>   for such compilers. ]
>
> Thanks for pointing this out, this was one trigger for me to start
> looking into this!
>
>> 
>> The need to keep requiring c++11 comes from porting new gdb releases to older
>> systems with older system compilers.
>> 
>> A review of some relevant distros has shown that in case that requiring c++17
>> results in no longer being able to use the system compiler, an alternative,
>> newer compiler that does support c++17 is readily available.
>> 
>> With nothing holding back the change, require a c++17 compiler to build gdb.
>
> Just for the record, it might be worth mentioning here the general
> policy of the project regarding bumping the C++ required version:
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#When_is_GDB_going_to_start_requiring_C.2B-.2B-NN_.3F
>
>> 
>> The gcc documentation ( https://gcc.gnu.org/projects/cxx-status.html ) states
>> that "Some C++17 features are available since GCC 5, but support was
>> experimental and the ABI of C++17 features was not stable until GCC 9".
>> 
>> Consequently, the NEWS item mentions gcc 9 as an example compiler to use.
>> 
>> My understanding of using gcc 5-8 is that it works as long as gdb doesn't use
>> not yet available language features.  Of course the set of used language
>> features may change in time, so what compiler still works may change.
>> 
>> Problems can arise when shared libs starts to have C++17 based APIs (or
>> somehow expose instantiations of such classes).  If compiled with a compiler
>> in which c++17 support was still experimental, it may be incompatible when
>> linking with:
>> - code compiled with a compiler with non-experimental c++17 support, or
>> - code compiled with a different compiler with experimental c++17 support.
>> 
>> Looking at the current implementation of our only shared lib:
>> libinproctrace.so, I couldn't spot any c++ usage in the API, so I'm assuming
>> this problem is not likely to happen.
>> 
>> Requiring c++17 means we can drop some code:
>> ...
>> $ find gdb* -type f \
>>     | egrep -v "/configure|\.m4" \
>>     | xargs grep "# *if.*__cplusplus.*2014"
>> gdb/cp-support.c:#if __cplusplus >= 201402L
>> gdb/dwarf2/cooked-index.c:#if __cplusplus >= 201402L
>> gdbsupport/gdb_optional.h:#if defined(_GLIBCXX_DEBUG) && __cplusplus >= 201402L
>> gdbsupport/gdb_optional.h:#if defined(_GLIBCXX_DEBUG) && __cplusplus >= 201402L
>> gdbsupport/gdb_unique_ptr.h:#if __cplusplus >= 201402L
>> gdbsupport/array-view.h:#if defined(_GLIBCXX_DEBUG) && __cplusplus >= 201402L
>> gdbsupport/array-view.h:#if defined(_GLIBCXX_DEBUG) && __cplusplus >= 201402L
>> gdbsupport/array-view.h:#if defined(_GLIBCXX_DEBUG) && __cplusplus >= 201402L
>> $ find gdb* -type f \
>>     | egrep -v "/configure|\.m4" \
>>     | xargs grep "# *if.*__cplusplus.*2017"
>> gdb/unittests/string_view-selftests.c:#if __cplusplus < 201703L
>> gdb/disasm.h:#if __cplusplus >= 201703L
>> gdbsupport/invoke-result.h:#if __cplusplus >= 201703L
>> gdbsupport/gdb_string_view.h:#if __cplusplus >= 201703L
>> ...
>> but that's not yet included in this patch.
>
> If this patch is accepted, one change I'd like is to do is drop
> gdb::make_unique, because 1) the current fallback we have for C++11
> compilers is not entirely compatible with std::make_unique (the T[] case
> is not supported properly I think), and 2) it is not widely used yet, so
> "it's not too late" :D

For my own education, is there something which we can write using
gdb::make_unique, which compiles, and does something useful, but which
isn't supported with std::make_unique?

Or something that behaves differently with std::make_unique vs
gdb::make_unique?

You mention the T[] case isn't supported, but that doesn't feel like
something we might be "too late" for -- that would just mean when we
switch to std::make_unique we can do more...

Thanks,
Andrew