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.

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

FreeBSD clang -std99 -pedantic doesn't like static_assert #3693

Closed
gilles-peskine-arm opened this issue Sep 18, 2020 · 1 comment · Fixed by #7229
Closed

FreeBSD clang -std99 -pedantic doesn't like static_assert #3693

gilles-peskine-arm opened this issue Sep 18, 2020 · 1 comment · Fixed by #7229
Assignees
Labels
bug component-platform Portability layer and build scripts historical-reviewing Currently reviewing (for legacy PR/issues)

Comments

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Sep 18, 2020

On FreeBSD 11.2 with Clang 6.0.0, or on FreeBSD 11.1-RC2 with Clang 4.0.0, Mbed TLS does not build with clang -std=c99 -pedantic -Werror. Observed with mbed TLS 2.24.0, and a few versions before.

$ cd library && clang -I../include -std=c99 -pedantic -Werror -c psa_crypto.c
psa_crypto.c:1675:1: error: _Static_assert is a C11-specific feature
      [-Werror,-Wc11-extensions]
static_assert( ( MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY & MBEDTLS_PSA_KA_MASK...
^
/usr/include/assert.h:71:23: note: expanded from macro 'static_assert'
#define static_assert   _Static_assert
                        ^
psa_crypto.c:1677:1: error: _Static_assert is a C11-specific feature
      [-Werror,-Wc11-extensions]
static_assert( ( PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_US...
^
/usr/include/assert.h:71:23: note: expanded from macro 'static_assert'
#define static_assert   _Static_assert
                        ^
psa_crypto.c:1679:1: error: _Static_assert is a C11-specific feature
      [-Werror,-Wc11-extensions]
static_assert( ( PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_EXTERNA...
^
/usr/include/assert.h:71:23: note: expanded from macro 'static_assert'
#define static_assert   _Static_assert
                        ^
3 errors generated.

The offending code looks like this:

#if defined(static_assert)
static_assert(…, "…");
#endif

so it's valid C99 code (which skips the asserts)

This can be reproduced with tests/scripts/all.sh -k test_clang_opt which we want to run on FreeBSD on our CI.

I cannot reproduce this on Linux, neither on https://godbolt.org/ nor on my machine (I tried several versions of Clang including 6.0.0). So it seems to be a FreeBSD thing.

I'm labelling this as “bug” because we do try to build without warnings on popular platforms, although it's borderline because the problem only occurs with a pedantic flag, not with a basic -std=c99 -Werror -Wall -Wextra. Mostly it's a bug because it fires on our CI and it's inconvenient to work around in a CI script.

@gilles-peskine-arm gilles-peskine-arm added bug component-platform Portability layer and build scripts labels Sep 18, 2020
@tom-cosgrove-arm
Copy link
Contributor

This doesn't seem to be a bug in Mbed TLS - I can reproduce the problem with

#include <assert.h>

#ifdef static_assert
static_assert(1 == 1, "oops!");
#endif

On FreeBSD, assert.h defines static_assert if __ISO_C_VISIBLE >= 2011. __ISO_C_VISIBLE is being set by default (to 2011) in sys/cdefs.h. This still happens on FreeBSD 12.3, as well as on 11.2.

On Linux, assert.h defines static_assert if __USE_ISOC11 is defined, and __USE_ISOC11 is defined by features.h if _ISOC11_SOURCE is defined. It gets defined if nothing specified on the command line, but doesn't if -std=c99 is given on the command line.

I suspect the best thing is to provide our own version of static_assert, to be defined if there isn't already one, or if we're on FreeBSD (defined(__FreeBSD__)) and __STDC_VERSION__ < 201100L (by undef static_assert)

#if defined(__FreeBSD__) && __STDC_VERSION__ < 201100L
#undef static_assert
#endif

#if !defined(static_assert)
#define static_assert ... /* one of the standard ways, e.g. typedef'ing an array with size < 0 */
#endif

Then if we always have static_assert() available, we can drop the #if defined guards, making the code easier to read.

If we prefer, we could call this mbedtls_static_assert().

The problem with the current workaround is that it hides warnings about all misuses of C11 features.

@tom-cosgrove-arm tom-cosgrove-arm added the historical-reviewing Currently reviewing (for legacy PR/issues) label Jan 27, 2023
@tom-cosgrove-arm tom-cosgrove-arm self-assigned this Feb 2, 2023
@gilles-peskine-arm gilles-peskine-arm added this to Technical debt 2023Q1 in EPICs for Mbed TLS Feb 2, 2023
tom-cosgrove-arm added a commit to tom-cosgrove-arm/mbedtls that referenced this issue Mar 8, 2023
Fixes Mbed-TLS#3693

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>
tom-cosgrove-arm added a commit to tom-cosgrove-arm/mbedtls that referenced this issue Mar 20, 2023
Fixes Mbed-TLS#3693

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-platform Portability layer and build scripts historical-reviewing Currently reviewing (for legacy PR/issues)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants