Use earliest latest compilers #7968

gowthamsk-arm · 2023-07-20T22:38:34Z

Needs Mbed-TLS/mbedtls-test#118 to be merged, otherwise the CI won't pass.

This PR has adds following changes:

The Ubuntu 16.04 and 22.04 docker images in mbedtls-test have been updated with earliest and latest versions of gcc and clang respectively. This PR adds the components and support functions required for the CI to run these compilers. For FreeBSD, we invoke the function by name so a condition is added to disable the existing test_clang_opt function for Linux.
Running clang-16 on mbedtls reports warnings of type "-Wstrict-prototypes". This patch fixes these warnings by adding void to functions with no arguments.

PR checklist

changelog not required
backport Backport 2.28: Use earliest latest compilers 2.28 #7969
tests not required

gowthamsk-arm · 2023-07-21T08:13:51Z

This PR has a dependency on Mbed-TLS/mbedtls-test#118
Internal CI run: https://jenkins-mbedtls.oss.arm.com/job/mbedtls-release-ci-testing/509/

gowthamsk-arm · 2023-07-25T13:15:24Z

OpenCI run: https://mbedtls.trustedfirmware.org/job/mbedtls-release-ci-testing/15/

tests/scripts/all.sh

gilles-peskine-arm · 2023-07-25T18:52:56Z

tests/suites/test_suite_aes.function

@@ -418,7 +418,7 @@ exit:
 /* END_CASE */

 /* BEGIN_CASE */
-void aes_invalid_mode()
+void aes_invalid_mode(void)


I'd prefer to keep allowing () in test functions. It's a more logical syntax (which C lacks only due to backward compatibility constraint), and if we stop allowing it, we'll likely forget in new work and then get surprised when the CI fails with clang-latest. If we do, then we don't need the massive (in terms of changed lines) changes to *.function.

This only requires a small change in tests/scripts/generate_test_code.py: in parse_function_code, if args is blank (ideally also if args only contains comments, but that's not critical), add void to it.

Makes sense. Will drop the *.function changes.

There's already code in the script to accept void as meaning no arguments, since 47e2e88. Looking at it now (I'd forgotten how it was done), it seems that we're stripping void if it was present in the source, so we should unconditionally add it when generating. So yes, adding it in gen_from_test_data does sound good.

By the way, there are unit tests in tests/scripts/test_generate_test_code.py. If inserting void breaks a unit test, please update the expected result from that test. If inserting void doesn't break an existing unit test, please add one.

The Ubuntu 16.04 and 22.04 docker images have been updated with earliest and latest versions of gcc and clang respectively. This patch adds the necessary component and support functions required for the CI to run these compilers. For FreeBSD we invoke the function by name so a condition is added to disable the existing test_clang_opt function for linux. Signed-off-by: Gowtham Suresh Kumar <[email protected]>

Running clang-16 on mbedtls reports warnings of type "-Wstrict-prototypes". This patch fixes these warnings by adding void to functions with no arguments. The generate_test_code.py is modified to insert void into test functions with no arguments in *.function files. Signed-off-by: Gowtham Suresh Kumar <[email protected]>

gilles-peskine-arm

Just a minor thing

gilles-peskine-arm · 2023-07-31T10:28:07Z

tests/scripts/generate_test_code.py

@@ -667,6 +667,11 @@ def parse_function_code(funcs_f, dependencies, suite_dependencies):
    code = code.replace(name, 'test_' + name, 1)
    name = 'test_' + name

+    # If a test function has no arguments then add 'void' argument to
+    # avoid "-Wstrict-prototypes" warnings from clang-16


Minor: not specifically clang-16. That option has been around at least as far back as Clang 3.5.

gilles-peskine-arm · 2023-07-31T10:30:15Z

tests/scripts/all.sh

+    : ${CLANG_LATEST:="clang-16"}
+    : ${CLANG_EARLIEST:="clang-3.5"}
+    : ${GCC_LATEST:="gcc-12"}
+    : ${GCC_EARLIEST:="gcc-4.7"}


I would prefer not to hard-code versions in all.sh, just like we don't hard-code versions of OpenSSL. The CI setup (docker files on Linux) defines the versions we use.

Suggested change

: ${CLANG_LATEST:="clang-16"}

: ${CLANG_EARLIEST:="clang-3.5"}

: ${GCC_LATEST:="gcc-12"}

: ${GCC_EARLIEST:="gcc-4.7"}

: ${CLANG_LATEST:="clang-latest"}

: ${CLANG_EARLIEST:="clang-earlist"}

: ${GCC_LATEST:="gcc-latest"}

: ${GCC_EARLIEST:="gcc-earliest"}

gilles-peskine-arm

LGTM except for a typo that prevented the CI from running

gilles-peskine-arm · 2023-07-31T18:07:06Z

tests/scripts/all.sh

@@ -176,7 +176,10 @@ pre_initialize_variables () {
    : ${ARMC6_BIN_DIR:=/usr/bin}
    : ${ARM_NONE_EABI_GCC_PREFIX:=arm-none-eabi-}
    : ${ARM_LINUX_GNUEABI_GCC_PREFIX:=arm-linux-gnueabi-}
-
+    : ${CLANG_LATEST:="clang-latest"}
+    : ${CLANG_EARLIEST:="clang-earlist"}


Suggested change

: ${CLANG_EARLIEST:="clang-earlist"}

: ${CLANG_EARLIEST:="clang-earliest"}

and likewise for gcc below.

Signed-off-by: Gowtham Suresh Kumar <[email protected]>

gilles-peskine-arm

LGTM

gilles-peskine-arm · 2023-08-01T09:17:21Z

tests/scripts/all.sh

+    : ${CLANG_LATEST:="clang-latest"}
+    : ${CLANG_EARLIEST:="clang-earliest"}
+    : ${GCC_LATEST:="gcc-latest"}
+    : ${GCC_EARLIEST:="gcc-earliest"}


Very minor: there used to be a blank line here, it's still present in the 2.28 branch but not here. Not worth changing if there's nothing else to change.

gilles-peskine-arm · 2024-06-19T11:56:47Z

tests/scripts/all.sh

    scripts/config.py full
-    test_build_opt 'full config' clang -O0 -Os -O2
+    test_build_opt 'full config' "$CLANG_EARLIEST" -O0


Why -O0? That's the fastest build, but -O or -O2is faster when doing build and test. Also,-O0` omits some warnings.

My bad, I looked into the changes thoroughly and this was supposed to be -O. :(
Its also mentioned in #7891.

For the earliest compilers, just do a -O build.

Shall I create a new PR for this change?

I'll fix it in #9286 which is where I noticed it.

gowthamsk-arm self-assigned this Jul 20, 2023

gowthamsk-arm added enhancement component-platform Portability layer and build scripts size-s Estimated task size: small (~2d) priority-medium Medium priority - this can be reviewed as time permits needs-ci Needs to pass CI tests labels Jul 20, 2023

gowthamsk-arm mentioned this pull request Jul 20, 2023

Backport 2.28: Use earliest latest compilers 2.28 #7969

Merged

3 tasks

gowthamsk-arm added 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 needs-ci Needs to pass CI tests and removed needs-ci Needs to pass CI tests labels Jul 25, 2023

gilles-peskine-arm mentioned this pull request Jul 25, 2023

Add earliest and latest compilers Mbed-TLS/mbedtls-test#118

Merged

gilles-peskine-arm reviewed Jul 25, 2023

View reviewed changes

tests/scripts/all.sh Show resolved Hide resolved

gilles-peskine-arm reviewed Jul 25, 2023

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review labels Jul 25, 2023

gowthamsk-arm force-pushed the use_earliest_latest_compilers branch from d5a84ff to 0a08d33 Compare July 26, 2023 15:50

gowthamsk-arm force-pushed the use_earliest_latest_compilers branch from 0a08d33 to 2ccdc6d Compare July 26, 2023 15:52

gowthamsk-arm force-pushed the use_earliest_latest_compilers branch from 2ccdc6d to 186731b Compare July 26, 2023 16:12

gilles-peskine-arm added needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels Jul 27, 2023

gowthamsk-arm added 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 and removed needs-work labels Jul 27, 2023

gowthamsk-arm force-pushed the use_earliest_latest_compilers branch from ec4988b to 6f1977b Compare July 28, 2023 16:09

gowthamsk-arm removed needs-work needs-ci Needs to pass CI tests labels Jul 31, 2023

tgonzalezorlandoarm previously approved these changes Jul 31, 2023

View reviewed changes

gilles-peskine-arm requested changes Jul 31, 2023

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 31, 2023

gowthamsk-arm dismissed tgonzalezorlandoarm’s stale review via f88b48b July 31, 2023 15:39

gowthamsk-arm force-pushed the use_earliest_latest_compilers branch from f88b48b to 57713bc Compare July 31, 2023 15:42

gilles-peskine-arm requested changes Jul 31, 2023

View reviewed changes

gilles-peskine-arm added the needs-ci Needs to pass CI tests label Jul 31, 2023

Update default variable values for compilers

9da40b8

Signed-off-by: Gowtham Suresh Kumar <[email protected]>

gowthamsk-arm force-pushed the use_earliest_latest_compilers branch from 57713bc to 9da40b8 Compare July 31, 2023 22:19

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work needs-ci Needs to pass CI tests labels Aug 1, 2023

gilles-peskine-arm approved these changes Aug 1, 2023

View reviewed changes

tgonzalezorlandoarm approved these changes Aug 1, 2023

View reviewed changes

gowthamsk-arm 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 Aug 1, 2023

gilles-peskine-arm reviewed Aug 1, 2023

View reviewed changes

gilles-peskine-arm added this pull request to the merge queue Aug 1, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 1, 2023

bensze01 added this pull request to the merge queue Aug 1, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 2, 2023

bensze01 added this pull request to the merge queue Aug 2, 2023

Merged via the queue into Mbed-TLS:development with commit 9661f8a Aug 2, 2023

gilles-peskine-arm mentioned this pull request Aug 3, 2023

Empty paragraph passed to '\retval' command [-Werror,-Wdocumentation] #7675

Closed

gilles-peskine-arm reviewed Jun 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use earliest latest compilers #7968

Use earliest latest compilers #7968

gowthamsk-arm commented Jul 20, 2023 •

edited

Loading

gowthamsk-arm commented Jul 21, 2023

gowthamsk-arm commented Jul 25, 2023

gilles-peskine-arm Jul 25, 2023

gowthamsk-arm Jul 25, 2023

gilles-peskine-arm Jul 26, 2023

gilles-peskine-arm Jul 26, 2023

gilles-peskine-arm left a comment

gilles-peskine-arm Jul 31, 2023

gilles-peskine-arm Jul 31, 2023

gilles-peskine-arm left a comment

gilles-peskine-arm Jul 31, 2023

gilles-peskine-arm left a comment

gilles-peskine-arm Aug 1, 2023

gilles-peskine-arm Jun 19, 2024

gowthamsk-arm Jun 19, 2024

gilles-peskine-arm Jun 19, 2024

gowthamsk-arm Jun 19, 2024

	: ${CLANG_EARLIEST:="clang-earlist"}
	: ${CLANG_EARLIEST:="clang-earliest"}

Use earliest latest compilers #7968

Use earliest latest compilers #7968

Conversation

gowthamsk-arm commented Jul 20, 2023 • edited Loading

PR checklist

gowthamsk-arm commented Jul 21, 2023

gowthamsk-arm commented Jul 25, 2023

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

gilles-peskine-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

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-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

gowthamsk-arm commented Jul 20, 2023 •

edited

Loading