Page MenuHomeFreeBSD

kyua: fix gcc13 builds
ClosedPublic

Authored by brooks on Jul 19 2024, 3:49 PM.
Tags
None
Referenced Files
F102790730: D46041.diff
Sun, Nov 17, 6:07 AM
Unknown Object (File)
Fri, Nov 15, 5:47 AM
Unknown Object (File)
Wed, Oct 30, 1:47 PM
Unknown Object (File)
Sat, Oct 19, 8:15 AM
Unknown Object (File)
Oct 17 2024, 1:57 PM
Unknown Object (File)
Sep 29 2024, 1:25 PM
Unknown Object (File)
Sep 29 2024, 11:28 AM
Unknown Object (File)
Sep 22 2024, 2:54 AM
Subscribers

Details

Summary

For some reason execenv::exec() isn't successfully marked noreturn,
but calling methods are so gcc complains. Work around this by adding
explicit __builtin_unreachable() calls.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This is absolutely the wrong fix, but my C++ isn't good enough to plumb why the noreturn annotations are mismatched.

This is absolutely the wrong fix, but my C++ isn't good enough to plumb why the noreturn annotations are mismatched.

I had started looking at this but also couldn't figure out why gcc is unhappy. All of the exec() implementations I found already seem to be suitably annotated with UTILS_NORETURN.

I'm ok with committing this workaround for now (could you please include a "Fixes:" tag in the commit log message?). @igor.ostapenko_pm.me would you be able to investigate this when you get a chance? To reproduce it you can install the amd64-gcc13 package and build world with CROSS_TOOLCHAIN=amd64-gcc13.

This revision is now accepted and ready to land.Jul 19 2024, 4:09 PM
imp accepted this revision.EditedJul 19 2024, 4:28 PM

Oh my that's a big, ugly hammer.

But.... #define UTILS_NORETURN __attribute__((noreturn)) should be fine. Maybe __noreturn__ in case noreturn is defined as something funky?
But also from cdefs.h:

#define __dead2         __attribute__((__noreturn__))
...
#if defined(__cplusplus) && __cplusplus >= 201103L
#define _Noreturn               [[noreturn]]
#else
#define _Noreturn               __dead2
#endif

so (a) it isn't coming from us that I can see and maybe (b) [[noreturn]] might be a good substitute? It looks like we compile with c++11, so a naked [[noreturn]] for that #define wouldn't be horrible...

Regardless of all that, what's here is OK enough to commit if my musings aren't helpful at getting to a root cause (and even if they are and you'd rather not bother).

After some experiments, I think that [[noreturn]] is not a solution that either clang or g++ will accept.

I am not very knowledgeable about C++ so with that caveat, I think that a virtual noreturn method just doesn't mean what it seems like it might on first glance.

vali% cat noreturn.cpp 
#if 1
#define	MY_NORETURN	[[noreturn]]
#elif 0
#define	MY_NORETURN	__attribute__((__noreturn__))
#elif 0
#define	MY_NORETURN
#endif

class c {
public:
	virtual void f(void) MY_NORETURN = 0;
};

static void __attribute__((__unused__)) MY_NORETURN
f(class c &c)
{
	c.f();
}

With #define MY_NORETURN [[noreturn]], g++ warns that 'noreturn' attribute does not apply to types and clang outright rejects it with a similar complaint, error: 'noreturn' attribute cannot be applied to types. With __attribute__((__noreturn__)), clang accepts the code but g++ issues warning: 'noreturn' function does return.

I think the right solution is don't define virtual functions with noreturn. You could always define a non-virtual noreturn to call the virtual and assert or throw on return.

This revision was automatically updated to reflect the committed changes.

Yes, it seems GCC has incomplete support for this, while Clang works with both the virtual function declaration and with its override. Kyua is based on a typical OOP polymorphism case #3:

#include <cstdlib>

class Interface {
public:
        virtual void method() const __attribute__((noreturn)) = 0;
};

class Impl : public Interface {
public:
        void method() const __attribute__((noreturn));
};

void Impl::method() const
{
        exit(1);
}

void exec() __attribute__((noreturn));
void exec()
{
        // Case 1: Direct subclass usage
        //
        //   Impl().method();
        //
        // clang: OK, requires only Impl being attributed
        // gcc:   OK, requires only Impl being attributed


        // Case 2: Direct subclass usage via pointer
        //
        //   (new Impl())->method();
        //
        // clang: OK, requires only Impl being attributed
        // gcc:   FAIL, does not get it from both the Interface and the Impl


        // Case 3: Indirect subclass usage via pointer to its super
        //
             Interface *x = new Impl();
             x->method();
        //
        // clang: OK, requires only Interface being attributed
        // gcc:   FAIL, no support to understand the case
}

int main(void)
{
        exec();
}

I'm thinking to check gcc14 and search for an existing GCC PR to promote it or file a new one. The solution applied looks to be the simplest one for now.

It would be appreciated to hear comments about the impact of this gcc build breakage. I would like to extract some lessons learned here, thus it would be good to know which non-default builds are worth checking as a homework for such patches.