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

[Lancelot SIX <Lancelot.Six@amd.com>]

On 05/10/2023 13:26, Andrew Burgess wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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...
Hi Andrew

I don't think something like gdb::make_unique<int[]>(2) would compile 
with a c++11 compiler.  So I don't expect that a given statement would 
compile fine in both C++11 and C++17 but with different semantics.

The difference can be fixed, we can have a complete implementation with 
c++11.  It is just that the current state of things, gdb::make_unique is 
not 100% equivalent to std::make_unique.

By "not too late", I mean that we currently have ~20 places using 
gdb::make_unique.  It is still easy to replace them all and just use the 
standard library if available.  If we have a standard version, I don't 
see the point in keeping the gdb:: version.

If you take something like gdb::optional for example, it is used quite 
extensively in the codebase, and I don't think it would be easy to 
remove the it in favor of the standard C++ version any time soon.  We 
can have gdb::optional be an alias to std::optional, but the codebase 
will still continue to use "gdb::optional" independently of how this is 
implemented.

I hope that this clarifies my position regarding gdb::make_unique.

Best,
Lancelot.

> 
> Thanks,
> Andrew
>