Unbreak builds with old C++ compilers #55290
Conversation
New C++ versions have deprecated "register" variables and restricted "volatile" semantics, so new gcc's will emit warnings when they see that syntax. Zephyr uses both in our C headers (though we should probably get rid of register and unify with C++'s volatile model), so we're disabling the resulting warnings. But OLD gcc variants (like xcc, sigh) don't understand new -Wvolatile and -Wregister on the command line, so they get confused. Limit the uses to the standard versions for which gcc would emit warnigns; xcc doesn't support those anyway. Signed-off-by: Andy Ross <andyross@google.com>
andyross
requested review from
stephanosio,
nashif,
carlescufi,
galak,
MaureenHelm and
tejlmand
as code owners
zephyrbot
added
area: Build System
area: C++
area: Base OS
zephyrbot
requested review from
alexanderwachter,
dcpleung,
jeremybettis and
nordicjm
|
Back link to a bunch of discussion in #53405 which is relevant |
|
The one thing I would ask if you are willing to do another rev, would be to add a few test permutations to |
|
Just stumbled on #55204 with more relevant discussion |
That seems like a great idea. See final commit, unless I messed things up. |
|
GCC 4.2.0 is at this point, 16 years old. So my question is what problems or inefficiencies are being introduced by rolling back the standard level we are using and having to support an older version? And for how much longer do we need to support such an old compiler? |
|
Until in-tree projects aren't using it, basically. I don't like xcc either, but it's what existing source code using current Zephyr is being built with. If we don't want to support it we should remove the existing integration and tell SOF to fork the project or whatever. And FWIW: in this case I think the justification was pretty thin for the features we were using. The constexpr template wasn't providing much value, and the other stuff was just bitrot. I do see that the new C++98 test variant pulled up more failures though, so I need to take a look at those. |
|
Two more patches to clean up issues detected with CONFIG_STD_CPP98=y. |
|
Ugh, one more. Pretty sure this is it. |
|
Gah, turns out the host compiler on my machine supports -std=c++2b, but the one in CI is older than the SDK and doesn't. Add a platform_exclude. NOW it should work, right? |
| #define __z_log2d(x) (32 - __builtin_clz(x) - 1) | ||
| #define __z_log2q(x) (64 - __builtin_clzll(x) - 1) | ||
| #define __z_log2(x) (sizeof(__typeof__(x)) > 4 ? __z_log2q(x) : __z_log2d(x)) |
There was a problem hiding this comment.
From "include/sys/util: Rework _Generic & C++11 usage":
Let's back out to a more portable (heh) implementation using gcc
__typeof__. This keeps the advantage of expanding to a single
__builtin_clz()for common case usage and is result-compatible with
the new LOG2/LOG2CEIL/NHPOT.
In general, I think that is a wrong solution to resolving issues like this -- if any, what we should be doing is to keep the existing standard conforming implementations and add another GCC-specific implementation for the corner case.
But, __typeof__ is not the only form of GNU-ism here and I am afraid "standard conformance" is already a lost cause unless we completely rework this, which I am sure is beyond the scope of this PR.
What we should have done in the first place is to add these macros in the toolchain abstraction layer such that any toolchain-specific constructs such as built-ins can be handled there.
Not asking this to be fixed in this PR; but, we should eventually look into moving these macros to the toolchain abstraction layer. cc @cfriedt
There was a problem hiding this comment.
It's important to note here that for the actual features at hand, the GCC implementation is significantly more portable (identical six-line implementation for AFAIK literally every compiler ever used to build zephyr) than the first cut (50 lines containing two different implementations across variant generic APIs that broke at least one in-tree Zephyr user).
I mean... yes, standards are good. But we need to be realistic about things too.
There was a problem hiding this comment.
the GCC implementation is significantly more portable
Until you use something other than GCC (or a compiler that intentionally follows what GCC does; notoriously, Clang). It really is not "portable" unless you live in a world where there are only two compilers: GCC and Clang.
I have no problem with using the GNU extensions where it makes sense; but, we should always provide a standard conforming solution alongside.
There was a problem hiding this comment.
Yeah yeah, I know, I've been through enough of these arguments to know I have zero interest in pursuing them. I'm just telling you straight up that the definition for "portable" you have chosen to defend means "works on fewer compilers" and not "works on more compilers", which is IMHO exactly backwards.
But it's irrelevant anyway because regardless of the semantic decisions about what "portable" means xcc is an in-tree toolchain that we have to support, period (at least until SOF moves to xt-clang on all its targets, though even then that's an old clang).
There was a problem hiding this comment.
I'm just telling you straight up that the definition for "portable" you have chosen to defend means "works on fewer compilers" and not "works on more compilers", which is IMHO exactly backwards.
Once again, the problem I have here is not the use of the __typeof__ GNU extension -- it is the removal of the existing implementations that do not require __typeof__.
p.s. But yeah, it is already a lost cause because __typeof__ is not the only problem here. As I noted above, all the built-ins being used here already broke compiler agnosticity and I am not asking for any changes in this particular PR.
There was a problem hiding this comment.
Sure, and a TYPESIZE() predicate would be al this implementation needs. But that's a whole lot of code to write and maintain for... really not a lot of value. I mean, I can't even name a compiler in use in the embedded world that doesn't support typeof(), and the only one I can think of outside it (MSVC) has had an equivalent decltype() for decades.
Don't expose C++11 APIs unless we're building for that platform Signed-off-by: Andy Ross <andyross@google.com> squashme cdecl
The recently-added LOG2 family of macros were implemented using either a compile-time expanded C11 _Generic expression or a C++11 constexpr template. As it happens, Cadence xt-xcc supports neither of these constructs, and integration for the next-generation xt-clang[1] hasn't arrived yet. Let's back out to a more portable (heh) implementation using gcc __typeof__. This keeps the advantage of expanding to a single __builtin_clz() for common case usage and is result-compatible with the new LOG2/LOG2CEIL/NHPOT. The one downside is that the generic code was able to be used in a constant context, something that statement expressions can't do. So the resulting macros are (like the earlier LOG2CEIL was) unhygienic and multi-expand their arguments. I added a warning to this effect in the docs. [1] Which is still based on an ancient clang tree, but more recent than their GCC 4.2 compiler. Signed-off-by: Andy Ross <andyross@google.com>
andyross
force-pushed
the
xcplusplus
branch
2 times, most recently
from
365cafd
to
b1db1fe
Compare
Add some cases to enumerate all the C++ standard variants that the SDK gcc supports (there are MANY) to prevent compatibility regressions in the OS headers. Signed-off-by: Andy Ross <andyross@google.com>
Empty structs are incompatible when building with C++, which specifies their size as 1 byte. Technically they are illegal in C; GCC has always supported them as an extension, but with a size of *zero* bytes. The upshot is that we can't have them in headers that may be presented to a C++ compiler. Add a placeholder field. We only get the resulting compiler warning with -std=c++98 it seems like, which is why this went undetected. Signed-off-by: Andy Ross <andyross@google.com>
This feature didn't appear until C++11, so don't expose our wrapper for it in C++98. Signed-off-by: Andy Ross <andyross@google.com>
This C11 feature doesn't work when building C++ code, but the C++ version is based on C++11's static_assert(), which we don't have in -std=c++98 either. Signed-off-by: Andy Ross <andyross@google.com>
We weren't explicit about what we mean by "C++ support" as it relates to standards versions, and it's been causing some friction in the tree vs. some of our cruftier toolchains. Add a paragraph to clarify things. Fixes zephyrproject-rtos#55204 Signed-off-by: Andy Ross <andyross@google.com>
|
Yet more stuff turned up that works on the SDK (and my local gcc 12.2.1) but not the CI host compiler. May just have to keep banging on this until they all fall out. |
|
sizeof() only works on lvalues or type names. You can "sizeof(int)" or "sizeof(*foo)", but you can't "sizeof(*foo+2)" or "sizeof(atan(dy, dx))" |
|
It's often hard to get things "done" while getting them "right", especially in this area. I'm impressed by how this PR does both - with a negative diff on top. Thanks for the free class. |
Cadence xcc can't handle some of the recent C++11 code. (There are third party out of tree SOF modules using C++ too that aren't ready for xt-clang). Let's slow walk the transition a bit and recompatibilize things for older compilers.