Pass c99 to compiler #3641

okhowang · 2020-09-03T08:22:37Z

Fixes #3631

Signed-off-by: okhowang(王沛文) okhowang@tencent.com

Notes:

Pull requests cannot be accepted until the PR follows the contributing guidelines. In particular, each commit must have at least one Signed-off-by: line from the committer to certify that the contribution is made under the terms of the Developer Certificate of Origin.
This is just a template, so feel free to use/remove the unnecessary things

Description

Pass standard c99 to compiler

Status

READY

Requires Backporting

No

Migrations

NO

Additional comments

Todos

Tests
Documentation
Changelog updated
Backported

Steps to test or reproduce

ronald-cron-arm

I have a few questions.

ronald-cron-arm · 2020-09-25T12:50:05Z

CMakeLists.txt

@@ -154,6 +154,10 @@ string(REGEX MATCH "Clang" CMAKE_COMPILER_IS_CLANG "${CMAKE_C_COMPILER_ID}")

 include(CheckCCompilerFlag)

+if (NOT CMAKE_VERSION VERSION_LESS 3.1.0)


Why do we need that?

CMAKE_C_STANDARD is available from cmake 3.1
ref: https://cmake.org/cmake/help/v3.1/release/3.1.0.html

ronald-cron-arm · 2020-09-25T12:51:46Z

CMakeLists.txt

@@ -175,6 +179,9 @@ if(CMAKE_COMPILER_IS_GNU)
            set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wformat-signedness")
        endif()
    endif()
+    if (NOT CMAKE_C_STANDARD AND GCC_VERSION VERSION_LESS 5.0)


What about to always set flag, whatever the compiler version. For later versions as it is their default it shouldn't matter and if at some point their default move to c11 we are still good.
Otherwise why not doing the same for the arm compiler?

For later versions as it is their default it shouldn't matter and if at some point their default move to c11 we are still good.

agree.

Otherwise why not doing the same for the arm compiler?

if cmake version < 3.1, there is no portable approach to set -std=c99 for all compiler, we should do it by different compiler.
I add flags for gcc only currently.
BTY, what compiler should we support?

why not doing the same for the arm compiler?

I don't think Cmake has any support for Arm compilers. And it shouldn't try to invoke an Arm Compiler unless explicitly requested, because that could cause an unexpected network access to check the license. People who use a “specialist” compiler like Arm Compiler generally have their build environment set up to pass target-specific flags anyway, so Cmake defaults aren't particularly useful for them. The Cmake defaults are mostly useful for non-cross builds where the user just wants to build without fuss.

ronald-cron-arm · 2020-09-25T12:53:37Z

Makefile

@@ -1,5 +1,8 @@
 DESTDIR=/usr/local
 PREFIX=mbedtls_
+override CFLAGS+=-std=gnu99
+MAKEOVERRIDES += CFLAGS+=-std=gnu99


Is this ok with ARM compiler?

No (CI confirms), and it's not ok with many other compilers either. Our makefile should be compiler-agnostic, at least if you pass a different WARNING_CFLAGS argument. I can't think of a case where an override in the makefile would be acceptable: when it comes to compiler flags, the makefile suggests, but the user decides.

If _GNU_SOURCE or some such is needed to support a library feature, the #define needs to go in the appropriate .c or .h file. If the code is using a GNU extension to the C language, it shouldn't, or if the feature is optional (e.g. extra safety or optimization), it should be guarded by conditional compilation.

-std=c99 and -std=gnu99 are made by C_EXTENSIONS in cmake.
and CMAKE_C_EXTENSIONS is ON default.
so we need set it to OFF when cmake?

we should set different flag for different compiler also.
but I have no idea about how do it in Makefile

Passing -std=c99 when you know that the compiler supports it (GCC, Clang, maybe other compilers with a gcc-compatible command line) is ok. But don't pass it if you don't know. And don't pass -std=gnu99: our code shouldn't require any GNU extension.

The makefiles are designed to be portable as long as you have GNU make. At least make lib should work on any OS with any compiler that accepts the options -o and -c. They don't even assume Unix utilities, and targets like make clean that assume Unix utilities (such as rm) have a Windows alternative (del). I don't think it's realistic to check the compiler in the makefiles. Note that cross-compilation is very common.

It's ok if some compilers require passing different flags when compiling, for example passing make WARNING_CFLAGS="-Wno-something" or make CFLAGS="-mfoo -fno-weird-optimization". We expect the default to work on the platforms that people use the most (at least when not cross-compiling), but we know we can't cover them all. It's not ok if the makefile passes -std=c99 to the compiler whether the user wanted it or not.

so we shouldn't add c99 flag in Makefile?

so we shouldn't add c99 flag in Makefile?

Right. Unless you find a way to do so that does the right thing for compilers that don't support -std=, is OS-agnostic, and doesn't make people who aren't GNU make experts want to gouge their eyes out (GNU make is a difficult language once you get past the basics).

I have revert changes in Makefile

okhowang · 2020-09-25T18:23:36Z

I current add c99 flag for gcc/clang in Makefile/CMake.
IAR and MSVC has no necessary to add c99 flag.
ref: https://github.com/Kitware/CMake/blob/master/Modules/Compiler/MSVC-C.cmake and https://github.com/Kitware/CMake/blob/master/Modules/Compiler/IAR-C.cmake

is there any other compiler that we need support?

gilles-peskine-arm · 2020-09-25T18:27:17Z

is there any other compiler that we need support?

In CMake: no, it's fine. Other compilers should still be usable, but they don't need to be usable out of the box: it's ok if they require explicit CFLAGS.

ronald-cron-arm · 2020-09-30T07:46:18Z

CMakeLists.txt

@@ -154,6 +154,11 @@ string(REGEX MATCH "Clang" CMAKE_COMPILER_IS_CLANG "${CMAKE_C_COMPILER_ID}")

 include(CheckCCompilerFlag)

+if (NOT CMAKE_VERSION VERSION_LESS 3.1.0)
+    set(CMAKE_C_STANDARD 99)


Is it a problem to not use this at all (remove those 4 lines) and to do "set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=c99")" unconditionally for GNU and ARM compiler?

add -std=c99 to CMAKE_C_FLAGS for GNU and ARM compiler is simple.
but CMAKE_C_STANDARD and CMAKE_C_EXTENSIONS is standard variables in CMake.
although they are exists after CMake 3.1.0
adding -std=c99 and some other compiler-specific script should be removed eventually after we drop support for old CMake version

I haven't followed all the conversation so I'm not sure if this is relevant. For information, we try to support maintained versions of Ubuntu and CentOS, which currently means CMake back to 2.8.12.2. And armcc doesn't support -std.

ok, I am convinced with your arguments, let's keep the setting of CMAKE_C_STANDARD. Rather than:
if (NOT CMAKE_VERSION VERSION_LESS 3.1.0), I would prefer though:
if (CMAKE_VERSION VERSION_GREATER ... ).

we need a expression cmake >= 3.1.0.
but VERSION_GREATER_EQUAL cmake introduced in cmake 3.7.
so we have to use not NOT CMAKE_VERSION VERSION_LESS 3.1.0

ronald-cron-arm · 2020-09-30T12:20:02Z

Otherwise in the CI the all.sh test component "test_everest: build: Everest ECDH context (ASan build)" is falling. Here is the part of the CI logs reporting the error:

[ 14%] Building C object library/CMakeFiles/mbedcrypto.dir/sha1.c.o
[ 15%] Building C object library/CMakeFiles/mbedcrypto.dir/__/3rdparty/everest/library/Hacl_Curve25519_joined.c.o
[ 15%] Built target mbedtls_test
In file included from /var/lib/build/3rdparty/everest/library/Hacl_Curve25519_joined.c:31:
/var/lib/build/3rdparty/everest/library/Hacl_Curve25519.c:417:17: error: implicit declaration of function 'le64toh' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
uint64_t i0 = load64_le(input);
^
/var/lib/build/3rdparty/everest/include/everest/kremlin/c_endianness.h:199:23: note: expanded from macro 'load64_le'
#define load64_le(b) (le64toh(load64(b)))
^
In file included from /var/lib/build/3rdparty/everest/library/Hacl_Curve25519_joined.c:31:
/var/lib/build/3rdparty/everest/library/Hacl_Curve25519.c:544:3: error: implicit declaration of function 'htole64' is invalid in C99 -Werror,-Wimplicit-function-declaration]
store64_le(b0, o0);
^
/var/lib/build/3rdparty/everest/include/everest/kremlin/c_endianness.h:200:38: note: expanded from macro 'store64_le'
#define store64_le(b, i) (store64(b, htole64(i)))
^
/var/lib/build/3rdparty/everest/library/Hacl_Curve25519.c:544:3: note: did you mean 'store64'?
/var/lib/build/3rdparty/everest/include/everest/kremlin/c_endianness.h:200:38: note: expanded from macro 'store64_le'
#define store64_le(b, i) (store64(b, htole64(i)))
^
/var/lib/build/3rdparty/everest/include/everest/kremlin/c_endianness.h:185:20: note: 'store64' declared here
inline static void store64(uint8_t *b, uint64_t i) {
^
2 errors generated.

library/CMakeFiles/mbedcrypto.dir/build.make:1670: recipe for target 'library/CMakeFiles/mbedcrypto.dir//3rdparty/everest/library/Hacl_Curve25519_joined.c.o' failed
make[2]: *** [library/CMakeFiles/mbedcrypto.dir//3rdparty/everest/library/Hacl_Curve25519_joined.c.o] Error 1
make[2]: *** Waiting for unfinished jobs....
CMakeFiles/Makefile2:418: recipe for target 'library/CMakeFiles/mbedcrypto.dir/all' failed
make[1]: *** [library/CMakeFiles/mbedcrypto.dir/all] Error 2
make: *** [all] Error 2
^^^^test_everest: build: Everest ECDH context (ASan build): command make -> 2^^^^
Makefile:138: recipe for target 'all' failed

okhowang · 2020-10-01T02:07:16Z

htole64 and le64toh was defined in <endian.h> with _BSD_SOURCE macro.
I couldn't found it in everest.
and Hacl_Curve25519.c looks like a generated code.
How we fix this problem?

gilles-peskine-arm · 2020-10-01T08:28:53Z

The Everest code is less portable than the rest of the library. If it needs #define _BSD_SOURCE to compile under a strict mode, we can add that somewhere. But preferably in a header that was not automatically generated.

okhowang · 2020-10-09T03:11:38Z

I added _BSD_SOURCE in Hacl_Curve25519_joined.c.
and _DEFAULT_SOURCE was added additionly for some compiler's warning #warning "_BSD_SOURCE and _SVID_SOURCE are deprecated, use _DEFAULT_SOURCE".

jk-arm · 2020-10-20T07:04:36Z

may i know when this fix expected to be part of the master?

tom-cosgrove-arm · 2022-05-11T14:15:26Z

3rdparty/everest/library/Hacl_Curve25519_joined.c

@@ -18,6 +18,12 @@
 *
 *  This file is part of mbed TLS (https://tls.mbed.org)
 */
+#ifndef _BSD_SOURCE


For unusual things like this it is useful to have a comment that explains why they are needed

err, yes, but why? what happens if you don't have it?

Fixes Mbed-TLS#3631 Signed-off-by: okhowang(王沛文) <okhowang@tencent.com>

Disable compiler-specific extensions (e.g. use --std=c11 instead of --std=gnu11). Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

tom-cosgrove-arm · 2022-05-11T20:37:32Z

programs/test/udp_proxy.c

@@ -73,9 +73,10 @@ int main( void )
 #endif
 #endif /* _MSC_VER */
 #else /* ( _WIN32 || _WIN32_WCE ) && !EFIX64 && !EFI32 */
-#if defined(MBEDTLS_HAVE_TIME)
+#if defined(MBEDTLS_HAVE_TIME) || (defined(MBEDTLS_TIMING_C) && !defined(MBEDTLS_TIMING_ALT))


@AndrzejKurek LOL :)

(@daverodgman don't worry, just flash-backs to a previous PR!)

tom-cosgrove-arm · 2022-05-11T20:51:33Z

3rdparty/everest/library/Hacl_Curve25519_joined.c

@@ -18,6 +18,12 @@
 *
 *  This file is part of mbed TLS (https://tls.mbed.org)
 */
+#ifndef _BSD_SOURCE
+#define _BSD_SOURCE /* Required to enable compilation under --std=c99 */


So actually I think this should be

Suggested change

#define _BSD_SOURCE /* Required to enable compilation under --std=c99 */

/* Required to get htole64() from gcc/glibc's endian.h (older systems)

* when we compile with -std=c99 */

#define _BSD_SOURCE

tom-cosgrove-arm · 2022-05-11T20:52:10Z

3rdparty/everest/library/Hacl_Curve25519_joined.c

+#define _BSD_SOURCE /* Required to enable compilation under --std=c99 */
+#endif
+#ifndef _DEFAULT_SOURCE
+#define _DEFAULT_SOURCE


Suggested change

#define _DEFAULT_SOURCE

/* (modern version of _BSD_SOURCE) */

#define _DEFAULT_SOURCE

See https://stackoverflow.com/a/29201732/7761803 (not clear that _BSD_SOURCE is still required, but let's continue to support older systems)

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

tom-cosgrove-arm

LGTM

tom-cosgrove-arm · 2022-05-12T08:55:59Z

@davidhorstmann-arm can you re-review please?

davidhorstmann-arm

LGTM - thanks!

daverodgman · 2022-05-12T13:00:48Z

CI failure is spurious

daverodgman added the needs-review Every commit must be reviewed by at least two team members, label Sep 3, 2020

ronald-cron-arm requested changes Sep 25, 2020

View reviewed changes

okhowang force-pushed the c99 branch from bbe8689 to 4fddcb7 Compare September 25, 2020 18:20

okhowang force-pushed the c99 branch from 4fddcb7 to e24e27a Compare September 25, 2020 18:30

ronald-cron-arm requested changes Sep 30, 2020

View reviewed changes

daverodgman added the Community label Oct 2, 2020

gilles-peskine-arm mentioned this pull request Oct 8, 2020

Build error (compiler not in C99 mode) #3771

Closed

daverodgman linked an issue Oct 8, 2020 that may be closed by this pull request

Build error (compiler not in C99 mode) #3771

Closed

okhowang force-pushed the c99 branch from e24e27a to cb40a4d Compare October 9, 2020 03:09

ronald-cron-arm self-requested a review November 20, 2020 16:00

mpg mentioned this pull request Nov 23, 2020

ssl_msg.c:1168:5: error: ‘for’ loop initial declarations are only allowed in C99 mode #3901

Closed

okhowang force-pushed the c99 branch from cb40a4d to 29af689 Compare December 30, 2020 11:24

gilles-peskine-arm added the needs-reviewer This PR needs someone to pick it up for review label Apr 6, 2021

daverodgman added needs-reviewer This PR needs someone to pick it up for review and removed needs-reviewer This PR needs someone to pick it up for review labels Apr 6, 2021

daverodgman assigned ronald-cron-arm Apr 6, 2021

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Apr 27, 2021

tom-cosgrove-arm reviewed May 11, 2022

View reviewed changes

daverodgman dismissed davidhorstmann-arm’s stale review via 75450fc May 11, 2022 16:09

daverodgman force-pushed the c99 branch from 824caf6 to 75450fc Compare May 11, 2022 16:09

daverodgman force-pushed the c99 branch from 75450fc to 2de554d Compare May 11, 2022 16:23

okhowang and others added 2 commits May 11, 2022 16:26

Pass c99 to compiler in CMakeLists.txt

df865c3

Fixes Mbed-TLS#3631 Signed-off-by: okhowang(王沛文) <okhowang@tencent.com>

Additional CMake flag for controlling C standard

f5f1cfc

Disable compiler-specific extensions (e.g. use --std=c11 instead of --std=gnu11). Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman force-pushed the c99 branch from 2de554d to dcd7b80 Compare May 11, 2022 16:26

daverodgman added size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits labels May 11, 2022

daverodgman added 2 commits May 11, 2022 19:55

Fix Ubuntu compile error in udp_proxy.c

faef649

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

Add comment

85fba90

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman force-pushed the c99 branch from dcd7b80 to 85fba90 Compare May 11, 2022 18:57

tom-cosgrove-arm reviewed May 11, 2022

View reviewed changes

Improve comments

c9c6e8d

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests labels May 12, 2022

tom-cosgrove-arm approved these changes May 12, 2022

View reviewed changes

davidhorstmann-arm approved these changes May 12, 2022

View reviewed changes

daverodgman added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels May 12, 2022

daverodgman merged commit d87e46f into Mbed-TLS:development May 12, 2022

		@@ -154,6 +154,10 @@ string(REGEX MATCH "Clang" CMAKE_COMPILER_IS_CLANG "${CMAKE_C_COMPILER_ID}")

		include(CheckCCompilerFlag)

		if (NOT CMAKE_VERSION VERSION_LESS 3.1.0)

	#define _DEFAULT_SOURCE
	/* (modern version of _BSD_SOURCE) */
	#define _DEFAULT_SOURCE

Pass c99 to compiler #3641

Pass c99 to compiler #3641

Conversation

okhowang commented Sep 3, 2020

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Steps to test or reproduce

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okhowang commented Sep 25, 2020

gilles-peskine-arm commented Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm commented Sep 30, 2020

okhowang commented Oct 1, 2020

gilles-peskine-arm commented Oct 1, 2020

okhowang commented Oct 9, 2020

jk-arm commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented May 12, 2022

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

daverodgman commented May 12, 2022

gilles-peskine-arm commented Sep 25, 2020 •

edited

Loading

ronald-cron-arm Sep 30, 2020 •

edited

Loading

tom-cosgrove-arm May 11, 2022 •

edited

Loading