Add a test with a cpp executable including all mbed TLS headers #1454

AndrzejKurek · 2018-03-14T21:28:25Z

Related to #1426 - in order to avoid such mistakes in the future, a test with all the mbed TLS headers included during a c++ executable building is added.

Internal ref: IOTSSL-2172

AndrzejKurek · 2018-03-14T21:38:29Z

And we've got a winner:

/home/travis/build/ARMmbed/mbedtls/programs/test/header_test.cpp:117:2: error: expected '}'
}
^
/home/travis/build/ARMmbed/mbedtls/include/mbedtls/rsa_internal.h:63:12: note: to match this '{'
extern "C" {

gilles-peskine-arm

I don't think we should build the C++ program by default. Other than that, looks good.

gilles-peskine-arm · 2018-03-14T23:05:03Z

programs/Makefile

@@ -67,6 +69,7 @@ APPS =	aes/aescrypt2$(EXEXT)		aes/crypt_and_hash$(EXEXT)	\
 	random/gen_random_ctr_drbg$(EXEXT)				\
 	test/ssl_cert_test$(EXEXT)	test/benchmark$(EXEXT)		\
 	test/selftest$(EXEXT)		test/udp_proxy$(EXEXT)		\
+	test/header_test$(EXEXT)								\


I don't think we should build this program by default. People who only have a C compiler and no C++ compiler, or who've set CC but not CXX, should be able to run make.

This applies to other build systems as well.

gilles-peskine-arm · 2018-03-14T23:28:12Z

programs/test/header_test.cpp

+#else
+#include <stdio.h>
+#include <stdlib.h>
+#define mbedtls_printf     printf


Why bother with these definitions here?

Just wanted to keep the same include format as in the other mbed TLS files, but I guess it's not necessary here for now. Removed it.

AndrzejKurek · 2018-03-15T09:19:38Z

This feature can be enabled by either:

Defining TEST_CPP using makefiles - make TEST_CPP=1
Defining TEST_CPP using cmake - (running cmake from a separate folder in mbed TLS directory, just to be tidy) cmake -D CMAKE_BUILD_TYPE:String="Check" -D TEST_CPP="yes" ../

mazimkhan · 2018-03-15T10:14:52Z

programs/test/header_test.cpp

+int main( int argc, char *argv[] )
+{
+    (void) argc;
+    (void) argv;


Missing return in a non void function.

In C++, that's allowed in main, by special dispensation. main has an implicit return 0.

k-stachowiak

I have no objections, just one minor remark.

k-stachowiak · 2018-03-15T10:12:08Z

programs/test/header_test.cpp

+
+int main( int argc, char *argv[] )
+{
+    (void) argc;


C standard allows main to take no arguments, which seems to be exactly what is needed here.

Good tip, done.

mazimkhan · 2018-03-15T10:25:02Z

programs/test/header_test.cpp

+#include "mbedtls/timing.h"
+#include "mbedtls/x509.h"
+
+#if defined(MBEDTLS_PLATFORM_C)


Checking MEBDTLS_x_C should be done for all modules. Or if we always build this test with full config then why check here. For any reason if implementation for a header is missing, stubs can be provided in this test completeness sake.

You can include any header file in any configuration, it's up to each header file to exclude things that shouldn't be included in a given configuration. MBEDTLS_PLATFORM_C is special in that regard.

This file was intended for our CI and mainly for the cmake build, but I left a possibility for people to test their configuration with the most common exclusions - custom config.h and custom platform macros. Do you think that I should change this code, if so - how?

And - are you concerned about Including all headers despite having a smaller configuration? There are no guards in the headers anyway, .c files have ifdef guards.

I known that the guards are in C files. My question was why guard platform.h unlike others.

mazimkhan · 2018-03-15T10:31:51Z

programs/Makefile

@@ -241,6 +247,10 @@ test/benchmark$(EXEXT): test/benchmark.c $(DEP)
 	echo "  CC    test/benchmark.c"
 	$(CC) $(LOCAL_CFLAGS) $(CFLAGS) test/benchmark.c   $(LOCAL_LDFLAGS) $(LDFLAGS) -o $@

+test/header_test$(EXEXT): test/header_test.cpp $(DEP)


To make this test self sufficient perhaps execute config.pl with full config.

Good idea, but that's up to test scripts such as all.sh, not the job of the build scripts.

What do you mean by self sufficient?

My idea was to have a make target that indicates whether the headers are compatible with C++. But general consensus here is do the config outside Makefile. Hence ignore this.

mazimkhan · 2018-03-15T10:35:30Z

programs/Makefile

@@ -76,6 +78,10 @@ ifdef PTHREAD
 APPS +=	ssl/ssl_pthread_server$(EXEXT)
 endif

+ifdef TEST_CPP
+APPS += test/header_test$(EXEXT)


Since this test tests C++ compatibility of the headers with full config. It is different from other tests/programs that all build with one valid config. Please check if a full config is valid for other programs.
If not I would rather make it mutually exclusive with all other programs.

This tests headers with any config - there are usually no ifdef guards in headers, hence nothing really should change.

In all.sh we should enable the full config and build this. The actual Makefile should just take the configs it's given.

simonbutcher · 2018-03-15T15:41:04Z

Note this fix relates to existing bug #477 (IOTSSL-761), which should be closed when we merge this.

simonbutcher

Thanks for the efforts @AndrzejKurek. If you could respond to my review feedback before I approve I'd be grateful.

simonbutcher · 2018-03-15T15:24:38Z

programs/.gitignore

@@ -45,6 +45,7 @@ ssl/mini_client
 test/benchmark
 test/ecp-bench
 test/selftest
+test/header_test


I don't think header_test really conveys what we want to do here. Can we call it something like cpp_dummy_build? Or at least something that says it's a C++ test.

Done - renamed to cpp_dummy_build.

simonbutcher · 2018-03-15T15:42:34Z

programs/Makefile

@@ -76,6 +78,10 @@ ifdef PTHREAD
 APPS +=	ssl/ssl_pthread_server$(EXEXT)
 endif

+ifdef TEST_CPP
+APPS += test/header_test$(EXEXT)


In all.sh we should enable the full config and build this. The actual Makefile should just take the configs it's given.

simonbutcher · 2018-03-15T15:44:48Z

programs/test/header_test.cpp

+/*
+ *  A C++ program that includes all of the mbed TLS header files, in order to
+ *  test if no errors are raised in the process.
+ *


Rather than say we want to check we build without errors...

/* * A C++ program that includes all of the mbed TLS header files, in order to * test if no errors are raised in the process.

I'd like to state the purpose more clearly, so I'd propose:

/* * This program is a dummy C++ program to ensure Mbed TLS library header files * can be included and built with a C++ compiler.

Done, updated as suggested.

simonbutcher · 2018-03-15T15:49:48Z

programs/test/header_test.cpp

+#include "mbedtls/memory_buffer_alloc.h"
+#endif
+
+int main( int argc, char *argv[] )


Ok - this is probably a naive question, but is this adequate? Shouldn't we want to check it links with a token function?

Mbed TLS works with C++ by compiling in C (not C++) as a C library, which the C++ linker can then resolve without the usual name mangling. As a build test, we're only confirming the preprocessor and compiler work - not the linker.

Can I propose we should call mbedlts_platform_setup() and then mbedtls_printf("CPP Build test\n") as a simple sanity test? (And the mbedtls_platform_teardown() for completeness).

If we want to check this properly, we need to link with every function in the library. This would detect a missing extern "C" somewhere.

This means the .cpp source file would have to be automatically generated, which requires a rather different approach.

Automatically generating the source file would have the advantage that the list of headers would also be automatically generated. With a static file, we have to remember to add new headers. Fortunately this doesn't happen often. Still, we should add a check script (or code in an existing script) to verify that all the headers are included.

I think trying to link every API function and/or a script to confirm every module is included is too much work for the moment, and a distraction from other issues. Feel free to raise those as issues if you want for the future.

However, I think we can link to a couple of files as I suggest above at little extra cost, and at least that would confirm the linker is working for something.

Added a basic set of calls as suggested.

mazimkhan

All my comments are answered. Approving.

simonbutcher · 2018-03-19T11:55:07Z

@gilles-peskine-arm - can you please approve, and then mark as 'ready for merge' as you see appropriate?

AndrzejKurek · 2018-03-19T11:57:10Z

Sorry for marking as ready for merge - I thought that 2 approvals are enough for that.

gilles-peskine-arm

What's there is good. But there's no Visual Studio project. Please add one or justify why it isn't needed.

AndrzejKurek · 2018-03-19T14:32:37Z

@gilles-peskine-arm
Do you think that there should be a C++ Visual Studio test? The C++ header check is mostly a sanity check, with a possibility for users to perform a check using their own set of headers (included via config.h). Current Visual Studio builds (as we're using it) create a C project, building our library as a C library, so creating a C++ Visual Studio project would be a new feature, with different guards to be added (for people that only want C Visual Studio builds).

mpg · 2018-04-17T09:25:07Z

Re-adding "needs review" + "needs work" as there is one "change requested" review.

simonbutcher · 2018-06-21T11:10:47Z

@AndrzejKurek - Looks like there's a small amount of rework and then we're good. Could you please look at this please? If you don't have access to Visual Studio, please let me know and we'll do something else.

k-stachowiak

LGTM, however I'll hold the approval until the PR is rebased (and re-reviewed), or until it's decided the rebase is not necessary.

k-stachowiak · 2018-06-27T09:00:32Z

tests/scripts/all.sh

+if [ $TEST_CXX -ne 0 ]; then
+    msg "build: Unix make, gcc and g++ test" # ~ 30s
+    cleanup
+    make TEST_CPP=1


Is there a reason for this being CPP rather than CXX like elsewhere? It's far from being a blocker, but it is possible to make it consistent, I'd take the opportunity.

Agreed, CPP is the C preprocessor.

Sure, I can change it, I just used it as an abbreviation of C++ (as in cppreference, not cxxreference).

Update - Removed the flag alltogether.

gilles-peskine-arm · 2018-06-27T09:09:19Z

tests/scripts/all.sh

+
+    msg "build: cmake, gcc and g++ test" # ~ 30s
+    cleanup
+    CC=gcc cmake -D TEST_CPP=YES .


Why test with both make and cmake? We run some tests with each to validate our build system. But here we're just building a program which has no purpose other than validating that it can be built. Any build system would do.

I opted for a larger coverage of build systems, just in case someone forces the CXX compiler for cmake to something not working.

Update - Removed the cmake test.

gilles-peskine-arm · 2018-06-27T09:09:53Z

tests/scripts/all.sh

+
+    msg "build: cmake, gcc and g++ test" # ~ 30s
+    cleanup
+    CC=gcc cmake -D TEST_CPP=YES .


Does CC=gcc influence the C++ compiler? Seems strange to me, I'd expect the C++ compiler to be ${CXX}.

It does not, It's just the usual syntax used for other tests in all.sh, and I thought it's a prerequisite to compile the usual sources (which are getting compiled during this test too).

mpg · 2018-06-28T07:19:52Z

Just a general note on this PR: when it is merge, there should be an announcement on the relevant channels so that all developers know that from now on, every time you create a new module, you also new to add it's header to the C++ testing file.

In case of any problems with the 'extern "C"' directives, building the executable will fail

Remove unnecessary defines from the test. Test by defining TEST_CPP using makefiles or cmake.

Change the name of header_test to cpp_dumy_build Update the test description to better reflect its contents

Remove the cmake test

AndrzejKurek · 2018-06-28T09:20:47Z

I have rebased this PR, incorporated all of the comments, thus making the cpp test mandatory in all.sh (with no flag for removal), removed the cmake test in all.sh.
I have also updated the header list according to head state, and I've reordered them in an alphabetical order, as it is easier to spot any inconsistencies this way.
@k-stachowiak , @sbutcher-arm , @mazimkhan , @gilles-peskine-arm - please re-review.

gilles-peskine-arm · 2018-06-29T14:39:17Z

tests/scripts/all.sh

@@ -580,6 +581,10 @@ msg "test/build: key-exchanges (gcc)" # ~ 1 min
 cleanup
 record_status tests/scripts/key-exchanges.pl

+msg "build: Unix make, gcc and g++ test" # ~ 30s
+cleanup
+make TEST_CPP=1


Should we merge this with an existing build?

Advantage: faster.

Downside: slightly less good reports if it fails since we'd have to look closer at the logs to see if it's the fault of the C++. But this is all.sh anyway, we don't.

I'm not suggesting to add TEST_CPP=1 to an existing command line, that would make it harder to backport this script. Rather, add this after an existing make, before a subsequent cleanup.

So you'd like to have the following code below:

msg "build: Unix make, -Os (gcc)" # ~ 30s cleanup make CC=gcc CFLAGS='-Werror -Wall -Wextra -Os' +make TEST_CPP=1

?

Yes, that's fine with me.

Updated, with additional logs to make things clear.

gilles-peskine-arm

This is a faithful rebase of my previous approval, plus almost-sorting the list of headers, plus an entry in all.sh. I have some reservations, but I'm willing to approve in a pinch.

gilles-peskine-arm · 2018-06-29T14:46:38Z

programs/test/cpp_dummy_build.cpp

+#include "mbedtls/md2.h"
+#include "mbedtls/md4.h"
+#include "mbedtls/md5.h"
+#include "mbedtls/md.h"


Thanks for sorting this list, but I'd prefer if it was sorted in binary order, rather than in some locale where punctuation is not sorted or sorted in a second pass and in a non-ASCII order (md.h should come before md2.h, ssl.h should come before ssl_anything.h).

It's the order reported by ls when using en_US.UTF-8. Could you suggest any other locale that would yield results that you've mentioned and could be considered more "default"? I'm curious, because that's what it would boil down to - checking it by hand, if not running the lengthy all-sh.

Please use the C locale for universally reproducible results.

gilles-peskine-arm · 2018-06-29T14:55:09Z

tests/scripts/all.sh

+msg "build: Unix make, gcc and g++ test" # ~ 30s
+cleanup
+make TEST_CPP=1
+


I would like to add a check that cpp_dummy_build.cpp lists all the headers except for whitelisted ones.

check_headers_in_cpp () { ls include/mbedtls >headers.txt <programs/test/cpp_dummy_build.cpp sed -n 's/"$//; s!^#include "mbedtls/!!p' | sort | diff headers.txt - rm headers.txt } record_status check_headers_in_cpp

Will there be any whitelisted ones? Currently, this fails only because check-config.h is not included there (there's no need to test it since it does not have any extern directives), but there's no problem in adding it there.

check_config.h is the only whitelisted one.

Added a check, included the check-config.h in the dummy build file.

change the g++ test to be incremental, to save time reorganize header order in cpp_dummy_build.cpp according to c locale

AndrzejKurek · 2018-07-11T10:39:24Z

@gilles-peskine-arm, @k-stachowiak, @mazimkhan - Could I kindly ask for your re-review?

gilles-peskine-arm · 2018-07-13T09:19:35Z

programs/Makefile

@@ -77,6 +79,10 @@ ifdef PTHREAD
 APPS +=	ssl/ssl_pthread_server$(EXEXT)
 endif

+ifdef TEST_CPP
+APPS += test/cpp_dummy_build$(EXEXT)


If you run make TEST_CPP=1 and later make clean, then test/cpp_dummy_build doesn't get cleaned. There's a similar preexisting defect with ssl/ssl_pthread_server. The defect with test/cpp_dummy_build is worse because we include it in our test scripts.

I won't block the PR for that, but please, in order of preference, either:

arrange for make clean to remove test/cpp_dummy_build (and ssl/ssl_pthread_server) regardless of the value of TEST_CPP;

or file an issue.

@AndrzejKurek is out for a couple of weeks, so I've raised this as issue #1862, so we can at least merge what we have, and fix this later, as it's a minor issue,

simonbutcher · 2018-07-16T17:13:04Z

CircleCI is failing, but that can be disregarded.

AndrzejKurek added enhancement mbed TLS team needs-review Every commit must be reviewed by at least two team members, labels Mar 14, 2018

AndrzejKurek requested review from k-stachowiak, mazimkhan, gilles-peskine-arm and dgreen-arm March 14, 2018 21:28

gilles-peskine-arm requested changes Mar 14, 2018

View reviewed changes

mazimkhan reviewed Mar 15, 2018

View reviewed changes

k-stachowiak reviewed Mar 15, 2018

View reviewed changes

mazimkhan suggested changes Mar 15, 2018

View reviewed changes

mazimkhan reviewed Mar 15, 2018

View reviewed changes

simonbutcher suggested changes Mar 15, 2018

View reviewed changes

simonbutcher previously approved these changes Mar 16, 2018

View reviewed changes

mazimkhan previously approved these changes Mar 16, 2018

View reviewed changes

AndrzejKurek 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 Mar 16, 2018

simonbutcher removed the approved Design and code approved - may be waiting for CI or backports label Mar 19, 2018

gilles-peskine-arm requested changes Mar 19, 2018

View reviewed changes

mpg added needs-review Every commit must be reviewed by at least two team members, needs-work labels Apr 17, 2018

simonbutcher removed the request for review from dgreen-arm June 21, 2018 11:10

simonbutcher removed the needs-review Every commit must be reviewed by at least two team members, label Jun 27, 2018

k-stachowiak reviewed Jun 27, 2018

View reviewed changes

gilles-peskine-arm reviewed Jun 27, 2018

View reviewed changes

Andrzej Kurek added 6 commits June 28, 2018 05:05

Add a test with a cpp executable including all mbed TLS headers

40741f8

In case of any problems with the 'extern "C"' directives, building the executable will fail

Change the cpp test to be optional

0211c32

Remove unnecessary defines from the test. Test by defining TEST_CPP using makefiles or cmake.

Tests: add a test for cpp linking

89c048c

Change the name of header_test to cpp_dumy_build Update the test description to better reflect its contents

Replace tabs with spaces

037ec4b

Add a CXX build to all.sh to execute the C++ dummy test by default

45a6719

Make the C++ test mandatory by removing the --no-cxx flag from all.sh

05be06c

Remove the cmake test

AndrzejKurek force-pushed the iotssl-2172-cpp-including-headers-test branch from 2a4a106 to 05be06c Compare June 28, 2018 09:16

AndrzejKurek added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 28, 2018

gilles-peskine-arm reviewed Jun 29, 2018

View reviewed changes

RonEld mentioned this pull request Jul 2, 2018

Key wrapping as defined in NIST SP800-38F #1658

Merged

4 tasks

all_sh: add a check for header inclusion in cpp_dummy_build.cpp

991f9fe

change the g++ test to be incremental, to save time reorganize header order in cpp_dummy_build.cpp according to c locale

gilles-peskine-arm approved these changes Jul 11, 2018

View reviewed changes

k-stachowiak approved these changes Jul 13, 2018

View reviewed changes

gilles-peskine-arm reviewed Jul 13, 2018

View reviewed changes

simonbutcher mentioned this pull request Jul 16, 2018

C++ header test doesn't clean up #1862

Closed

simonbutcher 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 Jul 16, 2018

simonbutcher merged commit 991f9fe into development Jul 20, 2018

simonbutcher deleted the iotssl-2172-cpp-including-headers-test branch July 20, 2018 19:48

simonbutcher mentioned this pull request Sep 11, 2018

Failing to compile with C++ compilers #477

Closed

Add a test with a cpp executable including all mbed TLS headers #1454

Add a test with a cpp executable including all mbed TLS headers #1454

Conversation

AndrzejKurek commented Mar 14, 2018

AndrzejKurek commented Mar 14, 2018

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

AndrzejKurek commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-stachowiak 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

Choose a reason for hiding this comment

simonbutcher commented Mar 15, 2018 • edited Loading

simonbutcher 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

mazimkhan left a comment

Choose a reason for hiding this comment

simonbutcher commented Mar 19, 2018

AndrzejKurek commented Mar 19, 2018

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

AndrzejKurek commented Mar 19, 2018 • edited Loading

mpg commented Apr 17, 2018

simonbutcher commented Jun 21, 2018

k-stachowiak 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

mpg commented Jun 28, 2018

AndrzejKurek commented Jun 28, 2018

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

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

AndrzejKurek commented Jul 11, 2018

Choose a reason for hiding this comment

simonbutcher commented Mar 15, 2018 •

edited

Loading

AndrzejKurek commented Mar 19, 2018 •

edited

Loading