search

seL4 / seL4_projects_libs

Other
19 stars 36 forks source link

libsel4vm: Adhere to gnu99 static assertion #96

Closed elmankku closed 1 year ago

elmankku commented 1 year ago

static_assert was standardized in C11 as a macro prior C23. While _Static_assert was introduced to C11 standard too, it's been part of GNU extensions and is available for GNU flavored standards prior C11.

Fixes compilation error on some versions of gcc.

elmankku commented 1 year ago

static_assert() comes from our https://github.com/seL4/musllibc/blob/sel4/include/assert.h

In https://github.com/seL4/util_libs/blob/master/libutils/include/utils/compile_time.h we also define a compile_time_assert() as alias for static_assert(), which is used in several files then. Might be worth to consider changing this also.

We are using _Static_assert() already in https://github.com/seL4/sel4runtime/blob/d935dd05da0cf959e9fd0140af913dc6fdaa0221/include/sel4runtime.h#L114.

Yes, pondered changing utils/compile_time.h. I can change that to be gnu99 compatible.

kent-mcleod commented 1 year ago

Fixes compilation error on some versions of gcc.

Which versions of GCC and with which flags are affected by this?

elmankku commented 1 year ago

Fixes compilation error on some versions of gcc.

Which versions of GCC and with which flags are affected by this?

We've only seen this while running builds on GH Actions directly on ubuntu-latest VM: amd64 gcc-aarch64-linux-gnu amd64 4:11.2.0-1ubuntu1

The error can be seen here and compile command is right above: https://github.com/tiiuae/tii_sel4_build/actions/runs/4720127137/jobs/8371810845#step:4:5197

Actually it seems that the compiler is passed both --std=gnu11 and --std=gnu99. A comparison to a successful build, it turns out that order of flags is reversed, so the effective standard has been gnu11 and that is the reason why this hasn't been caught.

elmankku commented 1 year ago

Actually it seems that the compiler is passed both --std=gnu11 and --std=gnu99. A comparison to a successful build, it turns out that order of flags is reversed, so the effective standard has been gnu11 and that is the reason why this hasn't been caught.

The successful build mentioned above was done on debian:bullseye container with following commands:

repo init -u https://github.com/seL4/camkes-vm-examples-manifest.git
repo sync
mkdir build
cd build
../init-build.sh -DCAMKES_VM_APP=vm_minimal -DPLATFORM=tx1 -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
ninja

You can check compile_commands.json to verify that both standard flags are passed.

elmankku commented 1 year ago

In https://github.com/seL4/util_libs/blob/master/libutils/include/utils/compile_time.h we also define a compile_time_assert() as alias for static_assert(), which is used in several files then. Might be worth to consider changing this also.

Done in: https://github.com/seL4/util_libs/pull/152 and PR updated accordingly.

elmankku commented 1 year ago

Ping @kent-mcleod , what do you think?

The reason why this haven't been caught is because CMAKE_C_STANDARD is set to 11 here: https://github.com/seL4/seL4_tools/blob/b2e54f0c478b7393b7c28a6f385575af1fab4b7b/cmake-tool/helpers/environment_flags.cmake#LL39C8-L39C8

CMAKE_C_STANDARD initializes C_STANDARD variable. That together with C_EXTENSION (ON by default) adds -std=gnu11 to all projects.