On Fri, 7 Mar 2025 at 09:10, Thomas Schwinge wrote: > > Hi Jonathan! Good morning,
> On 2025-03-06T19:58:55+0000, Jonathan Wakely <jwak...@redhat.com> wrote: > > On Thu, 6 Mar 2025 at 19:48, Jonathan Wakely wrote: > >> On Thu, 6 Mar 2025 at 14:28, Thomas Schwinge wrote: > >> > In a '-fno-exceptions' configuration: > >> > > >> > In file included from > >> > ../../../../../source-gcc/libstdc++-v3/src/c++20/format.cc:29: > >> > [...]/build-gcc/[...]/libstdc++-v3/include/format: In function ‘void > >> > std::__throw_format_error(const char*)’: > >> > [...]/build-gcc/[...]/libstdc++-v3/include/format:200:36: error: > >> > unused parameter ‘__what’ [-Werror=unused-parameter] > >> > 200 | __throw_format_error(const char* __what) > >> > | ~~~~~~~~~~~~^~~~~~ > >> > > >> > libstdc++-v3/ > >> > * include/bits/c++config [!__cpp_exceptions] > >> > (_GLIBCXX_THROW_OR_ABORT): Reference '_EXC'. > >> > > >> > Co-authored-by: Jonathan Wakely <jwak...@redhat.com> > >> > >> Hmm, I didn't like this change (my original review said "I don't think > >> we want/need this at all, but it could be done like this...") > > We need something to this effect to prevent target libstdc++ build > failure with '--enable-werror' in combination with '-fno-exceptions'. Yes but we could just use [[maybe_unused]] on the __throw_format_error parameter instead. After considering it further, I do think your change is better. > >> and it > >> turns out that it causes diagnostic regressions. > > I had done a before vs. after comparison for powerpc64le-linux-gnu > 'check-gcc-c++ check-target-libstdc++-v3' with > 'RUNTESTFLAGS=--target_board=unix/-fno-exceptions', and not observed any > difference at all. > > Should I regularly be doing any further testing; where are you seeing > these regressions? I have an unpushed commit in my local tree and it includes a new test which started failing due to these "control reaches end of non-void function" warnings: https://gcc.gnu.org/pipermail/gcc-patches/2025-March/677017.html I don't think any of the existing tests observe any difference, so you couldn't have noticed it by running the tests differently. It only shows up for XFAIL tests, and there aren't a huge number of those in the libstdc++ testsuite. That unpushed commit is fixing an issue where std::expected::value()&& was failing to diagnose a condition which the standard says we must diagnose: https://cplusplus.github.io/LWG/issue3940 ("Mandates:" means the implementation must give a diagnostic if that condition isn't met.) There was no diagnostic for -fno-exceptions because the error object was not moved if THROW_OR_ABORT didn't construct an exception, so we failed to diagnose some invalid code. Your commit actually fixed that, because now constructing the exception is always present in the code, even if it's not reachable because it happens after the abort(). > >> The problem is in the > >> front-end so I've filed it as https://gcc.gnu.org/PR119149 > > > > Which seems to be a dup of my own much older https://gcc.gnu.org/PR91388 > > > >> I think we can live with the diagnostic regressions, since it's only > >> for code that's ill-formed anyway, and only for -fno-exceptions. > > Please let me know if there's anything I should do. > > Or, would these regressions be addressed by my original patch, à la: > > -# define _GLIBCXX_THROW_OR_ABORT(_EXC) (__builtin_abort(), (void)(_EXC)) > +# define _GLIBCXX_THROW_OR_ABORT(_EXC) ((void)(_EXC), __builtin_abort()) > > ..., and relying on the compiler to optimize out any dead code before > '__builtin_abort()'? No, we definitely don't want that. Users who want abort() instead of throwing exceptions don't want to pay the cost of constructing and allocating a potentially expensive exceptions object here. I don't think the compiler can tell that it's dead code if it calls non-inline functions, because they have unknown side effects. I don't think you should do anything, let's keep the change. Overall, it seems like an improvement: we don't need to remember to think about [[maybe_unused]] everywhere that the THROW_OR_ABORT macro is used, and it creates parity in other diagnostics for -fexceptions vs -fno-exceptions, because constructing the exception object needs to be valid C++, even if we're using -fno-exceptions so the exception won't actually get thrown.
