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.
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
Enable C23 when available and nodiscard feature #1879
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @oshogbo)
Suggestion:
available
Suggestion:
Enable C23 and nodiscard feature when avilable
meson.build
line 21 at r1 (raw file):
# If C23 (or experimetal C23 - C2x) is available, use it. # XXX: Gramine supports older versions of Ubuntu, so newer versions of compilers # may not be available.
When will we be able to drop this if? Please add a TODO and include a specific Ubuntu version explicitly (when dropping support I usually grep the whole repo for the Ubuntu version, I'd like to be able to find this place in the future when doing so).
common/include/nodiscard.h
line 10 at r1 (raw file):
* the nodiscard attribute introduced in C23. However, because Gramine supports * older systems that might not have support for C23, we have to wrap it on our * own and change it to a no-op on systems that don't support it.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @oshogbo and @woju)
meson.build
line 21 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
When will we be able to drop this if? Please add a TODO and include a specific Ubuntu version explicitly (when dropping support I usually grep the whole repo for the Ubuntu version, I'd like to be able to find this place in the future when doing so).
But that's not only about Ubuntu... We officially support Debian, Ubuntu, AlmaLinux, and Alpine. Semi-officially we also support CentOS 8 & 9 and its other derivatives (other than AlmaLinux I mean).
I think @woju had a table with all Gramine dependencies across different OS distros. Is this table still available? And maybe it's time to put this table somewhere in Gramine documentation?
common/include/pal_error.h
line 13 at r1 (raw file):
#include <stddef.h> typedef enum NODISCARD _pal_error_t {
But shouldn't this NODISCARD
attribute make our build fail on compatible GCC versions? I would assume that this would produce tons of warnings on Ubuntu 24.04? (And those warnings will be turned into hard errors because of our meson flags.)
Have you tried it on newer GCCs/Ubuntus? Does anything break? If yes -- and I think it should be yes -- then this PR should go together with the appropriate fixes. And possibly after we merge Ubuntu 24.04 support.
Signed-off-by: Mariusz Zaborski <oshogbo@invisiblethingslab.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @mkow, and @woju)
-- commits
line 2 at r1:
Done.
-- commits
line 2 at r1:
Done.
meson.build
line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But that's not only about Ubuntu... We officially support Debian, Ubuntu, AlmaLinux, and Alpine. Semi-officially we also support CentOS 8 & 9 and its other derivatives (other than AlmaLinux I mean).
I think @woju had a table with all Gramine dependencies across different OS distros. Is this table still available? And maybe it's time to put this table somewhere in Gramine documentation?
Oh, you are right. Somehow I limited myself to old Ubuntu. The list would be nice, I could also document simply the version of clang/gcc?
common/include/pal_error.h
line 13 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But shouldn't this
NODISCARD
attribute make our build fail on compatible GCC versions? I would assume that this would produce tons of warnings on Ubuntu 24.04? (And those warnings will be turned into hard errors because of our meson flags.)Have you tried it on newer GCCs/Ubuntus? Does anything break? If yes -- and I think it should be yes -- then this PR should go together with the appropriate fixes. And possibly after we merge Ubuntu 24.04 support.
No, it doesn't generate tons of errors. Currently, functions don't return pal_error_t
. In almost all situations they return int
. As mentioned in PR this will be a future work to convert functions to return pal_error_t
. The proposition is that the new code should already start using pal_error_t
as the return code.
I haven't tested it on Ubuntu 24.04. However, I tested it using a newer version of GCC (that does support this flag) on Debian.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow, @oshogbo, and @woju)
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
Not done? You didn't force-push or whatever you wanted to do.
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
Not done? You didn't force-push or whatever you wanted to do.
meson.build
line 21 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Oh, you are right. Somehow I limited myself to old Ubuntu. The list would be nice, I could also document simply the version of clang/gcc?
Let's wait for @woju opinion.
common/include/pal_error.h
line 13 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
No, it doesn't generate tons of errors. Currently, functions don't return
pal_error_t
. In almost all situations they returnint
. As mentioned in PR this will be a future work to convert functions to returnpal_error_t
. The proposition is that the new code should already start usingpal_error_t
as the return code.I haven't tested it on Ubuntu 24.04. However, I tested it using a newer version of GCC (that does support this flag) on Debian.
Wow, you are right. We don't use pal_error_t
at all.
09d74f2
to
ab24fbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow and @woju)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not done? You didn't force-push or whatever you wanted to do.
Force pushed to wrong branch sorry.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not done? You didn't force-push or whatever you wanted to do.
Force pushed to wrong branch sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @mkow and @woju)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @mkow, and @oshogbo)
meson.build
line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Let's wait for @woju opinion.
https://woju.github.io/gramine-deps-table/ . It can't be added to our docs, because those versions change over time, so I have github action that recreates it daily. I can add it to our orga however, if you want.
The table is currently broken for GCC, because some maintainer decided to have something called libiberty
have the same email as gcc (repology, the site I pull data from, has complicated logic for deduplicating packages between different distro families, where package names may differ, and this is a clear fault at this logic). I may need to tweak my script someday.
The real table looks like this:
distro | GCC |
---|---|
*EL 8 | 8 |
Ubuntu 20.04 LTS | 9 |
Debian 11 | 10 |
Ubuntu 22.04 LTS | 11 |
*EL 9 | 11 |
Debian 12 | 12 |
Ubuntu 24.04 LTS | 13 |
IIUC C23 is supported as -std=c2x
since GCC 9, [[nodiscard]]
since GCC 10 and __has_c_attribute
since GCC 11. Looking at the table, we need this if
until we drop support for AlmaLinux (CentOS etc.) 8. Which will happen when EL 10 will be released, but it wasn't even announced yet. RHEL 9 was released 2 years ago and 3 years after RHEL 8, which itself was released 5 years after RHEL 7, so we're looking at at least 1 year, but possibly 3.
I think __has_c_attribute
approach is suboptimal, because nodiscard could be supported by testing for it in meson: https://mesonbuild.com/Compiler-properties.html#does-code-compile. Something like if cc.compiles(<code with unconditional nodiscard>)
and add a global define instead of using __has_c_attribute
, because GCC 10 < GCC 11. This is also slightly easier to follow if you debug stuff through preprocessor, because the result of cc.compiles
is shown in meson log, bug random #ifdef
is silent. Admittedly in nodiscard
this might not be very relevant (am I correct that this just runs some static analyser and has no impact on compiled code?), but for other c23 features this might matter.
Also add_project_arguments('-std=')
is potentially bad idea, because meson has competing builtin option c_std
: https://mesonbuild.com/Builtin-options.html#compiler-options, which is already set above. Apparently since meson 1.3.0 we will be able to have default_options: ['c_std=c23,c2x,c11']
, but no supported distro has it yet (only noble
). I don't know what happens when you set both c_std
and -std=
.
HTH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)
meson.build
line 21 at r1 (raw file):
I can replace the __has_c_attribute
with meson check.
Also
add_project_arguments('-std=')
is potentially bad idea, because meson has competing builtin optionc_std
: https://mesonbuild.com/Builtin-options.html#compiler-options, which is already set above. Apparently since meson 1.3.0 we will be able to havedefault_options: ['c_std=c23,c2x,c11']
, but no supported distro has it yet (onlynoble
). I don't know what happens when you set bothc_std
and-std=
.
Do you suggest to drop default_options
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @mkow, and @oshogbo)
meson.build
line 21 at r1 (raw file):
Do you suggest to drop
default_options
?
I'm not, I just have no idea what will happen. Oh, and I have no idea what will happen when someone does meson setup -Dc_std=<something>
, which we can't prevent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)
meson.build
line 21 at r1 (raw file):
Oh, and I have no idea what will happen when someone does
meson setup -Dc_std=<something>
.
But do we care? If somebody does something really weird, then it's their problem no?
I'm not, I just have no idea what will happen.
The lastadd_project_arguments
takes precedent:
ARGS = -Ipal/src/host/linux/libpal.so.p -Ipal/src/host/linux -I../pal/src/host/linux -Ipal/include -I../pal/include -I../pal/include/pal -I../pal/include/arch/x86_64 -Icommon/include -I../common/include -I../common/include/arch/x86_64 -I../pal/include/arch/x86_64/linux -I../pal/include/host/linux-common -Isubprojects/uthash-2.1.0/src -I../subprojects/uthash-2.1.0/src -Isubprojects/tomlc99-208203af46bdbdb29ba199660ed78d09c220b6c5 -I../subprojects/tomlc99-208203af46bdbdb29ba199660ed78d09c220b6c5 -Icommon/src/ioctls -I../common/src/ioctls -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -std=c11 -O0 -g -std=c2x -Wa,--noexecstack -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes -Wwrite-strings -Wnull-dereference -DDEBUG -fPIC -DIN_PAL -ffreestanding -fPIC -fsanitize=undefined -fno-sanitize-recover=undefined -DUBSAN -fsanitize=address -fno-sanitize-link-runtime -mllvm -asan-mapping-offset=0x18000000000 -DASAN -mllvm -asan-use-after-return=0 -fno-stack-protector -DHOST_TYPE=Linux
Do you want me to check something else?
I think
__has_c_attribute
approach is suboptimal, because nodiscard could be supported by testing for it in meson: https://mesonbuild.com/Compiler-properties.html#does-code-compile. Something likeif cc.compiles(<code with unconditional nodiscard>)
and add a global define instead of using__has_c_attribute
, because GCC 10 < GCC 11. This is also slightly easier to follow if you debug stuff through preprocessor, because the result ofcc.compiles
is shown in meson log, bug random#ifdef
is silent. Admittedly innodiscard
this might not be very relevant (am I correct that this just runs some static analyser and has no impact on compiled code?), but for other c23 features this might matter.
We can't use ``cc.compiles()`, as it ignores detecting the C standard. As:
These compiler checks do not use compiler arguments added with add_*_arguments(), via -Dlang_args on the command-line, or through CFLAGS/LDFLAGS, etc in the environment. Hence, you can trust that the tests will be fully self-contained, and won't fail because of custom flags added by other parts of the build file or by users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)
meson.build
line 21 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Oh, and I have no idea what will happen when someone does
meson setup -Dc_std=<something>
.But do we care? If somebody does something really weird, then it's their problem no?
I'm not, I just have no idea what will happen.
The last
add_project_arguments
takes precedent:ARGS = -Ipal/src/host/linux/libpal.so.p -Ipal/src/host/linux -I../pal/src/host/linux -Ipal/include -I../pal/include -I../pal/include/pal -I../pal/include/arch/x86_64 -Icommon/include -I../common/include -I../common/include/arch/x86_64 -I../pal/include/arch/x86_64/linux -I../pal/include/host/linux-common -Isubprojects/uthash-2.1.0/src -I../subprojects/uthash-2.1.0/src -Isubprojects/tomlc99-208203af46bdbdb29ba199660ed78d09c220b6c5 -I../subprojects/tomlc99-208203af46bdbdb29ba199660ed78d09c220b6c5 -Icommon/src/ioctls -I../common/src/ioctls -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -std=c11 -O0 -g -std=c2x -Wa,--noexecstack -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes -Wwrite-strings -Wnull-dereference -DDEBUG -fPIC -DIN_PAL -ffreestanding -fPIC -fsanitize=undefined -fno-sanitize-recover=undefined -DUBSAN -fsanitize=address -fno-sanitize-link-runtime -mllvm -asan-mapping-offset=0x18000000000 -DASAN -mllvm -asan-use-after-return=0 -fno-stack-protector -DHOST_TYPE=Linux
Do you want me to check something else?
I think
__has_c_attribute
approach is suboptimal, because nodiscard could be supported by testing for it in meson: https://mesonbuild.com/Compiler-properties.html#does-code-compile. Something likeif cc.compiles(<code with unconditional nodiscard>)
and add a global define instead of using__has_c_attribute
, because GCC 10 < GCC 11. This is also slightly easier to follow if you debug stuff through preprocessor, because the result ofcc.compiles
is shown in meson log, bug random#ifdef
is silent. Admittedly innodiscard
this might not be very relevant (am I correct that this just runs some static analyser and has no impact on compiled code?), but for other c23 features this might matter.We can't use ``cc.compiles(
)`, as it ignores detecting the C standard. As:
These compiler checks do not use compiler arguments added with add_*_arguments(), via -Dlang_args on the command-line, or through CFLAGS/LDFLAGS, etc in the environment. Hence, you can trust that the tests will be fully self-contained, and won't fail because of custom flags added by other parts of the build file or by users.
Please, ignore my comment about cc.compiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @mkow, and @oshogbo)
meson.build
line 21 at r1 (raw file):
The last
add_project_arguments
takes precedent:
Good.
We can't use
cc.compiles()
, as it ignores detecting the C standard. As:
Oof, this is inconvenient.
OK, so can we do it like that: we'll define default c_std
to c2x
(not c23
), then still do this detection of cc.compiles(nodiscard)
, and expect people who compile on EL8 (incl. packaging) to manually write meson setup -Dc_std=c11
? I think there are not that many people who build on EL8.
Signed-off-by: Mariusz Zaborski <oshogbo@invisiblethingslab.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
meson.build
line 21 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
The last
add_project_arguments
takes precedent:Good.
We can't use
cc.compiles()
, as it ignores detecting the C standard. As:Oof, this is inconvenient.
OK, so can we do it like that: we'll define default
c_std
toc2x
(notc23
), then still do this detection ofcc.compiles(nodiscard)
, and expect people who compile on EL8 (incl. packaging) to manually writemeson setup -Dc_std=c11
? I think there are not that many people who build on EL8.
I have proposed solution to solve this inconvenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
meson.build
line 21 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
I have proposed solution to solve this inconvenient.
*inconvenience
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @oshogbo)
meson.build
line 21 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
*inconvenience
Ah, it has args:
. So in that case I'd prefer if test for C23 was decoupled from nodiscard, like it were previously. If someone wanted to get new feature from C23 used, then add_project_arguments
still needs to be called only once.
But still, I think the cleanest solution would be to just have c_std=c2x
, test for nodiscard
and have people explicitly set -Dc_std=c11
if they're on very old compilers. @dimakuv @mkow what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
meson.build
line 21 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Ah, it has
args:
. So in that case I'd prefer if test for C23 was decoupled from nodiscard, like it were previously. If someone wanted to get new feature from C23 used, thenadd_project_arguments
still needs to be called only once.But still, I think the cleanest solution would be to just have
c_std=c2x
, test fornodiscard
and have people explicitly set-Dc_std=c11
if they're on very old compilers. @dimakuv @mkow what do you think?
Fine with me, I'm fine with people using old software having some extra hurdles, it would only be bad if using Gramine with new software was harder than with old.
Although I prefer the #ifdef version, as it was simpler and faster (no need to invoke a compiler separately to check for it).
One thing I don't like in the current version is that it gets separate args
, so, it's not exactly the same config as used for Gramine and it may diverge.
So, not blocking, but I don't see what was wrong with #ifdefs - it was simpler, faster and guaranteed to be tested in the same environment as later compiled.
Description of the changes
Change the build system to use C23 or C2x (the experimental version of C23 used before the official release). We plan to convert functions to return pal_error_t instead of int. This will be implemented in separate commits, each making small, incremental changes.
This commit is done also to enforce a new policy on a new code.
Why __has_c_attribute
The
__has_c_attribute
is used to check the availability of new attributes. It was added simultaneously withnodiscard
in the experimental version: https://gcc.gnu.org/gcc-11/changes.html.Documentation for __has_c_attribute can be found at: https://en.cppreference.com/w/c/language/attributes.
How to test this PR?
CI.
This change is