Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unbreak builds with old C++ compilers #55290

Merged
merged 8 commits into from
Mar 2, 2023
Merged

Unbreak builds with old C++ compilers #55290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ae7687d
cmake/compiler: Limit warning flag usage to compatible toolchains
andyross Feb 28, 2023
76ed36e
minimal libcpp: Un-11-ify cstddef
andyross Feb 28, 2023
39df479
include/sys/util: Rework _Generic & C++11 usage
andyross Feb 28, 2023
5fd7457
tests/lib/cpp/cxx: Add compatibility cases for C++ standards
andyross Mar 1, 2023
b704345
drivers/gna: No empty structs
andyross Mar 1, 2023
8815b3a
sys/cbprintf: Limit C++ rvalue references to compatible standards
andyross Mar 1, 2023
1d50c83
toolchain/gcc: Limit use of _Static_assert
andyross Mar 1, 2023
bc3450c
doc: Clarify C++ standards support
andyross Mar 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions cmake/compiler/gcc/compiler_flags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,15 @@ set_compiler_property(TARGET compiler-cpp PROPERTY nostdincxx "-nostdinc++")
# Required C++ flags when using gcc
set_property(TARGET compiler-cpp PROPERTY required "-fcheck-new")

# GCC compiler flags for C++ dialects
# GCC compiler flags for C++ dialect: "register" variables and some
# "volatile" usage generates warnings by default in standard versions
# higher than 17 and 20 respectively. Zephyr uses both, so turn off
# the warnings where needed (but only on the compilers that generate
# them, older toolchains like xcc don't understand the command line
# flags!)
set_property(TARGET compiler-cpp PROPERTY dialect_cpp98 "-std=c++98")
set_property(TARGET compiler-cpp PROPERTY dialect_cpp11 "-std=c++11" "-Wno-register")
set_property(TARGET compiler-cpp PROPERTY dialect_cpp14 "-std=c++14" "-Wno-register")
set_property(TARGET compiler-cpp PROPERTY dialect_cpp11 "-std=c++11")
set_property(TARGET compiler-cpp PROPERTY dialect_cpp14 "-std=c++14")
set_property(TARGET compiler-cpp PROPERTY dialect_cpp17 "-std=c++17" "-Wno-register")
set_property(TARGET compiler-cpp PROPERTY dialect_cpp2a "-std=c++2a"
"-Wno-register" "-Wno-volatile")
Expand Down
6 changes: 6 additions & 0 deletions doc/develop/languages/cpp/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ and the included compiler must be supported by the Zephyr build system. The
is supported by Zephyr, and the features and their availability documented
here assume the use of the Zephyr SDK.

The default C++ standard level (i.e. the language enforced by the
compiler flags passed) for Zephyr apps is C++11. Other standards are
available via kconfig choice, for example
:kconfig:option:`CONFIG_STD_CPP98`. The oldest standard supported and
tested in Zephyr is C++98.

When compiling a source file, the build system selects the C++ compiler based
on the suffix (extension) of the files. Files identified with either a **cpp**
or a **cxx** suffix are compiled using the C++ compiler. For example,
Expand Down
3 changes: 3 additions & 0 deletions include/zephyr/drivers/gna.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ extern "C" {
* Currently empty.
*/
struct gna_config {
#ifdef __cplusplus
char no_empty_structs; /* technically a gcc C extension */
#endif
};

/**
Expand Down
2 changes: 2 additions & 0 deletions include/zephyr/sys/cbprintf_cxx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,12 @@ struct z_cbprintf_cxx_remove_reference < T & > {
typedef T type;
};

#if __cplusplus >= 201103L
template < typename T >
struct z_cbprintf_cxx_remove_reference < T && > {
typedef T type;
};
#endif

template < typename T >
struct z_cbprintf_cxx_remove_cv {
Expand Down
104 changes: 19 additions & 85 deletions include/zephyr/sys/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,87 +29,6 @@
/** @brief Number of bits that make up a type */
#define NUM_BITS(t) (sizeof(t) * 8)

#ifdef __cplusplus
template <typename T> static inline constexpr int __z_log2_impl(T x)
{
if (sizeof(x) < sizeof(unsigned int)) {
return ((x) < 1) * (-1) +
((x) >= 1) * (NUM_BITS(unsigned int) - __builtin_clz(x) - 1);
} else if (sizeof(x) <= sizeof(unsigned long long)) {
return ((x) < 1) * (-1) +
((x) >= 1) * (NUM_BITS(unsigned long long) - __builtin_clzll(x) - 1);
}

/* No declaration of __ASSERT is available here */
static_assert(sizeof(x) <= sizeof(unsigned long long), "unsupported type for LOG2()");

return -1;
}

template <typename T> static inline constexpr int __z_log2ceil_impl(T x)
{
if (sizeof(x) < sizeof(unsigned int)) {
return (x > 1) * (NUM_BITS(unsigned int) - __builtin_clz(x - 1));
} else if (sizeof(x) <= sizeof(unsigned long long)) {
return (x > 1) * (NUM_BITS(unsigned long long) - __builtin_clzll(x - 1));
}

/* No declaration of __ASSERT is available here */
static_assert(sizeof(x) <= sizeof(unsigned long long), "unsupported type for LOG2CEIL()");

return -1;
}

template <typename T> static inline constexpr uint64_t __z_nhpot_impl(T x)
{
int l2c = __z_log2ceil_impl(x);

return (l2c != NUM_BITS(unsigned long long)) * (1ULL << l2c);
}
#else
#define __z_log2_impl(x) \
(((x) < 1) * (-1) + \
((x) >= 1) * _Generic((x), char \
: (NUM_BITS(unsigned int) - __builtin_clz(x) - 1), unsigned char \
: (NUM_BITS(unsigned int) - __builtin_clz(x) - 1), short \
: (NUM_BITS(unsigned int) - __builtin_clz(x) - 1), unsigned short \
: (NUM_BITS(unsigned int) - __builtin_clz(x) - 1), int \
: (NUM_BITS(unsigned int) - __builtin_clz(x) - 1), unsigned int \
: (NUM_BITS(unsigned int) - __builtin_clz(x) - 1), long \
: (NUM_BITS(unsigned long) - __builtin_clzl(x) - 1), unsigned long \
: (NUM_BITS(unsigned long) - __builtin_clzl(x) - 1), long long \
: (NUM_BITS(unsigned long long) - __builtin_clzll(x) - 1), \
unsigned long long \
: (NUM_BITS(unsigned long long) - __builtin_clzll(x) - 1)))

#define __z_log2ceil_impl(x) \
(((x) > 1) * \
_Generic((x), char \
: (NUM_BITS(unsigned int) - __builtin_clz((x)-1)), unsigned char \
: (NUM_BITS(unsigned int) - __builtin_clz((x)-1)), short \
: (NUM_BITS(unsigned int) - __builtin_clz((x)-1)), unsigned short \
: (NUM_BITS(unsigned int) - __builtin_clz((x)-1)), int \
: (NUM_BITS(unsigned int) - __builtin_clz((x)-1)), unsigned int \
: (NUM_BITS(unsigned int) - __builtin_clz((x)-1)), long \
: (NUM_BITS(unsigned long) - __builtin_clzl((x)-1)), unsigned long \
: (NUM_BITS(unsigned long) - __builtin_clzl((x)-1)), long long \
: (NUM_BITS(unsigned long long) - __builtin_clzll((x)-1)), unsigned long long \
: (NUM_BITS(unsigned long long) - __builtin_clzll((x)-1))))

#define __z_nhpot_impl(x) \
_Generic((x), char \
: (1UL << LOG2CEIL(x)), unsigned char \
: (1UL << LOG2CEIL(x)), short \
: (1UL << LOG2CEIL(x)), unsigned short \
: (1ULL << LOG2CEIL(x)), int \
: (1ULL << LOG2CEIL(x)), unsigned int \
: (1ULL << LOG2CEIL(x)), long \
: (1ULL << LOG2CEIL(x)), unsigned long \
: (1ULL << LOG2CEIL(x)), long long \
: (1ULL << LOG2CEIL(x)), unsigned long long \
: (1ULL << LOG2CEIL(x)))
#endif

#ifdef __cplusplus
extern "C" {
#endif
Expand Down Expand Up @@ -592,34 +511,49 @@ char *utf8_trunc(char *utf8_str);
*/
char *utf8_lcpy(char *dst, const char *src, size_t n);

#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))
Comment on lines +514 to +516
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@stephanosio stephanosio Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* @brief Compute log2(x)
*
* @note This macro expands its argument multiple times (to permit use
* in constant expressions), which must not have side effects.
*
* @param x An unsigned integral value
*
* @param x value to compute logarithm of (positive only)
*
* @return log2(x) when 1 <= x <= max(x), -1 when x < 1
*/
#define LOG2(x) __z_log2_impl(x)
#define LOG2(x) ((x) < 1 ? -1 : __z_log2(x))

/**
* @brief Compute ceil(log2(x))
*
* @note This macro expands its argument multiple times (to permit use
* in constant expressions), which must not have side effects.
*
* @param x An unsigned integral value
*
* @return ceil(log2(x)) when 1 <= x <= max(type(x)), 0 when x < 1
*/
#define LOG2CEIL(x) __z_log2ceil_impl(x)
#define LOG2CEIL(x) ((x) < 1 ? 0 : __z_log2((x)-1) + 1)

/**
* @brief Compute 2^ceil(log2(x))
* @brief Compute next highest power of two
*
* Equivalent to 2^ceil(log2(x))
*
* @note This macro expands its argument multiple times (to permit use
* in constant expressions), which must not have side effects.
*
* @param x An unsigned integral value
*
* @return 2^ceil(log2(x)) or 0 if 2^ceil(log2(x)) would saturate 64-bits
*/
#define NHPOT(x) __z_nhpot_impl(x)
#define NHPOT(x) ((x) < 1 ? 1 : ((x) > (1ULL<<63) ? 0 : 1ULL << LOG2CEIL(x)))

#ifdef __cplusplus
}
Expand Down
9 changes: 6 additions & 3 deletions include/zephyr/toolchain/gcc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,14 @@
#define BUILD_ASSERT(EXPR, MSG...) static_assert(EXPR, "" MSG)

/*
* GCC 4.6 and higher have the C11 _Static_assert built in, and its
* GCC 4.6 and higher have the C11 _Static_assert built in and its
* output is easier to understand than the common BUILD_ASSERT macros.
* Don't use this in C++98 mode though (which we can hit, as
* static_assert() is not available)
*/
#elif (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)) || \
(__STDC_VERSION__) >= 201100
#elif !defined(__cplusplus) && \
((__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)) || \
(__STDC_VERSION__) >= 201100)
#define BUILD_ASSERT(EXPR, MSG...) _Static_assert(EXPR, "" MSG)
#else
#define BUILD_ASSERT(EXPR, MSG...)
Expand Down
2 changes: 2 additions & 0 deletions lib/cpp/minimal/include/cstddef
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
namespace std {
using ::ptrdiff_t;
using ::size_t;
#if __cplusplus >= 201103L
using ::max_align_t;
using nullptr_t = decltype(nullptr);
#endif
}

#endif /* ZEPHYR_SUBSYS_CPP_INCLUDE_CSTDDEF_ */
38 changes: 38 additions & 0 deletions tests/lib/cpp/cxx/testcase.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,41 @@ tests:
filter: CONFIG_PICOLIBC_SUPPORTED
extra_configs:
- CONFIG_PICOLIBC=y

# Note: the -std= variants below exclude the host compilers, which
# aren't part of the SDK and can't be managed as part of the test
# suite. (e.g. as of commit time the g++ used in CI didn't support
# C++20/2B and emits a command line error when presented with
# -Wno-pointer-sign or -Werror=implicit-int in C++ mode with
# -std=c++98)
cpp.main.cpp98:
platform_exclude: native_posix native_posix_64
build_only: true
extra_configs:
- CONFIG_STD_CPP98=y
# Note: no "cpp.main.cpp11" as that's the default standard tested above
cpp.main.cpp14:
platform_exclude: native_posix native_posix_64
build_only: true
extra_configs:
- CONFIG_STD_CPP14=y
cpp.main.cpp17:
platform_exclude: native_posix native_posix_64
build_only: true
extra_configs:
- CONFIG_STD_CPP17=y
cpp.main.cpp2A:
platform_exclude: native_posix native_posix_64
build_only: true
extra_configs:
- CONFIG_STD_CPP2A=y
cpp.main.cpp20:
platform_exclude: native_posix native_posix_64
build_only: true
extra_configs:
- CONFIG_STD_CPP20=y
cpp.main.cpp2B:
platform_exclude: native_posix native_posix_64
build_only: true
extra_configs:
- CONFIG_STD_CPP2B=y