xtest: Add test of gcov #457

lenormandfranck · 2020-11-02T07:57:36Z

Adds test for gcov in the range 31000

This patch adds a set of test of the generation of code coverage:
(At the font of the test suite)

31001: Check the code coverage is enabled for the core:
31002: Check the code coverage is enabled for the TA:
31003: Dump the current code coverage of the core
31004: Reset the code coverage of the core
(Normal execution in order, should be at the end)
31005: Dump different code coverage:
- 31005.1: Core code coverage from xtest
- 31005.2: Core code coverage from TA
- 31005.3: TA code coverage from TA

The test are added using ADBG_CASE_DEFINE_FRONT to the test suite
so the code coverage of the core is resetted before executing xtest
suite and dump at the end.
It allows to capture the code coverage of the core when running
xtest.

Adds test for gcov in the range 31000 This patch adds a set of test of the generation of code coverage: (At the font of the test suite) - 31001: Check the code coverage is enabled for the core: - 31002: Check the code coverage is enabled for the TA: - 31003: Dump the current code coverage of the core - 31004: Reset the code coverage of the core (Normal execution in order, should be at the end) - 31005: Dump different code coverage: - 31005.1: Core code coverage from xtest - 31005.2: Core code coverage from TA - 31005.3: TA code coverage from TA The test are added using ADBG_CASE_DEFINE_FRONT to the test suite so the code coverage of the core is resetted before executing xtest suite and dump at the end. It allows to capture the code coverage of the core when running xtest. Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

etienne-lms · 2020-11-06T14:40:01Z

host/xtest/gcov.c

@@ -0,0 +1,160 @@
+/*


license tag? (bsd2? gpl2?)

I will add the GPL tags to all the files

etienne-lms · 2020-11-06T14:42:33Z

host/xtest/gcov.c

+
+#include <tee_client_api.h>
+#include <ta_gcov.h>
+#include <pta_gcov.h>


sort these group:

#include <pta_gcov.h> #include <ta_gcov.h> #include <tee_client_api.h>

~~If not i unique alpha~~ Maybe a unique list of #include <> (with those from line 4 to 12) sorted alphabetically ~~, at your will.~~
(edited)

etienne-lms · 2020-11-06T14:42:59Z

host/xtest/gcov.c

+{
+	TEEC_Session session = { };
+	TEEC_Operation op = TEEC_OPERATION_INITIALIZER;
+	uint32_t ret_orig;


prefer with an init value. 0 is fine.

etienne-lms · 2020-11-06T14:55:36Z

host/xtest/gcov.c

+		Do_ADBG_EndSubCase(c, "Dump %s", conf->desc);
+	}
+}
+ADBG_CASE_DEFINE(regression, 31005, xtest_tee_test_31005,


regression tests are currently all from regression_.c source files.
Either use a new suite gcov (for this gcov.c)?
or more simply rename gcov.c to regression_3000.c? where we maybe start putting debug tools tests in regression 3xxx.

There is a few constraints I want to have for the gcov test:

Dump the coverage of the core at boot

Clear the core code coverage

execute xtest

Dump the code coverage of the core

This way, running gcov allowed to dump the relevent code coverage without the need for another app called by the user.
this is why the tests for gcov are added last as object file so I can place the test 1 to 4 at the start of the list to run (ADBG_CASE_DEFINE_FRONT) and the 5th at the end.

I could make an implementation more flexible by having the following:

current tests in regression_3000.c

test suite gcov with copy of test 3 and 5
So the user would have to do:

xtest gcov 0 (dump core code coverage and clean)

xtest

xtest gcov 1 (dump core code coverage corresponding to the execution of xtest)

I think having extra xtest arguments would be flexible, i.e.:
xtest --gcov-version to get the 2 gcov version info
xtest --gcov-core to collect gcov data for the core
xtest --gcov-clean to clean gcov data
xtest --gcov-collect to collect gcov data

etienne-lms · 2020-11-06T14:56:15Z

ta/CMakeLists.txt

@@ -20,4 +20,5 @@ target_include_directories(${PROJECT_NAME}
 	INTERFACE socket/include
 	INTERFACE storage_benchmark/include
 	INTERFACE tpm_log_test/include
+	INTERFACE gcov/include


move to line 13, in alpha ordering

etienne-lms · 2020-11-06T15:15:22Z

ta/gcov/ta_gcov.c

+	if (res != TEE_SUCCESS) {
+		EMSG("TEE_OpenTASession failed with code 0x%x origin 0x%x",
+		     res, err_origin);
+		goto exit;


shoudl return, not close an invalid sess handle.

I have fixed the fail path

ok as-is but it think it would be easier to return res; straight here.

etienne-lms · 2020-11-06T15:15:53Z

ta/gcov/ta_gcov.c

+	TEE_TASessionHandle sess;
+	uint32_t err_origin;
+	uint32_t int_ptypes = 0;
+	TEE_Param int_params[TEE_NUM_PARAMS] = {0};


s/{0}/{ 0 }/ (with space chars)

etienne-lms · 2020-11-06T15:16:12Z

ta/gcov/ta_gcov.c

+						   TEE_PARAM_TYPE_NONE);
+	uint32_t cRT = TEE_TIMEOUT_INFINITE;
+	TEE_UUID uuid = (TEE_UUID)PTA_GCOV_UUID;
+	TEE_TASessionHandle sess;


prefer always an init vlaue. (ret above also, and those below)

etienne-lms · 2020-11-06T15:17:56Z

ta/gcov/include/user_ta_header_defines.h

+
+#define TA_FLAGS                (TA_FLAG_USER_MODE | TA_FLAG_EXEC_DDR | \
+				TA_FLAG_SECURE_DATA_PATH | \
+				TA_FLAG_CACHE_MAINTENANCE)


These flags are not needed. #define TA_FLAGS 0 should be fine here.

yes, thanks

etienne-lms · 2020-11-06T15:23:06Z

ta/gcov/ta_gcov.c

@@ -0,0 +1,143 @@
+/*


Please add an SPDX license tag.

Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

github-actions · 2020-12-10T01:13:06Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note that you can always re-open a closed pull request at any time.

lenormandfranck · 2020-12-14T08:33:44Z

I have pushed a patch to addresse etienne comment.
I plan to keep the test part of gcov calls in xtest however I will move the calls which cannot really be tested (generation of data coverage) in its own test suite to avoid the workaround I am doing which will not be stable on the long term.

github-actions · 2021-01-14T02:07:03Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note that you can always re-open a closed pull request at any time.

jenswi-linaro · 2021-01-14T14:29:22Z

@etienne-lms ping?

etienne-lms · 2021-01-15T06:52:59Z

host/xtest/gcov.c

+	(void)ADBG_EXPECT_TEEC_SUCCESS(c,
+		TEEC_InvokeCommand(&session, cmd, &op, &ret_orig));
+
+	ADBG_EXPECT_TRUE(c, (op.params[0].value.a != 0));


can drop inner parentheses.

etienne-lms · 2021-01-15T06:53:50Z

host/xtest/gcov.c

+	op.paramTypes = TEEC_PARAM_TYPES(TEEC_VALUE_OUTPUT, TEEC_NONE,
+					 TEEC_NONE, TEEC_NONE);
+
+	(void)ADBG_EXPECT_TEEC_SUCCESS(c,


can remove (void).

etienne-lms · 2021-01-15T07:04:49Z

host/xtest/gcov.c

+		Do_ADBG_EndSubCase(c, "Dump %s", conf->desc);
+	}
+}
+ADBG_CASE_DEFINE(regression, 31005, xtest_tee_test_31005,


I think having extra xtest arguments would be flexible, i.e.:
xtest --gcov-version to get the 2 gcov version info
xtest --gcov-core to collect gcov data for the core
xtest --gcov-clean to clean gcov data
xtest --gcov-collect to collect gcov data

etienne-lms · 2021-01-15T07:05:15Z

ta/gcov/ta_gcov.c

+						   TEE_PARAM_TYPE_NONE,
+						   TEE_PARAM_TYPE_NONE,
+						   TEE_PARAM_TYPE_NONE);
+	if (param_types != exp_param_types) {


add an empty line above

etienne-lms · 2021-01-15T07:05:49Z

ta/gcov/ta_gcov.c

+}
+
+TEE_Result TA_OpenSessionEntryPoint(uint32_t param_types,
+		TEE_Param  params[4], void **sess_ctx)


checkpatch would require indentation to parenthesis.

etienne-lms · 2021-01-15T07:12:34Z

ta/gcov/ta_gcov.c

+	res = TEE_InvokeTACommand(sess, cRT, PTA_CMD_GCOV_CORE_DUMP_ALL,
+				  int_ptypes, int_params, &err_origin);
+	if (res != TEE_SUCCESS)
+		EMSG("TEE_InvokeTACommand failed with code 0x%x origin 0x%x",


EMSG("... code %#"PRIx32" origin %#"PRIx32",
even if more than 80 char in this line

etienne-lms · 2021-01-15T07:13:27Z

ta/gcov/ta_gcov.c

+	if (res != TEE_SUCCESS) {
+		EMSG("TEE_OpenTASession failed with code 0x%x origin 0x%x",
+		     res, err_origin);
+		goto exit;


ok as-is but it think it would be easier to return res; straight here.

etienne-lms · 2021-01-15T07:16:02Z

ta/gcov/ta_gcov.c

+	case TA_GCOV_CMD_DUMP_TA:
+		return dump_ta(param_types, params);
+	default:
+		EMSG("Command %d not supported", cmd_id);


s/%d/%"PRIu32"/

etienne-lms · 2021-01-15T07:26:26Z

ta/gcov/ta_gcov.c

+						   TEE_PARAM_TYPE_NONE,
+						   TEE_PARAM_TYPE_NONE,
+						   TEE_PARAM_TYPE_NONE);
+	uint32_t cRT = TEE_TIMEOUT_INFINITE;


s/cRT/c_rt/ prefer snake case in OP-TEE C source files.

etienne-lms · 2021-01-15T07:29:33Z

ta/gcov/ta_gcov.c

+						   TEE_PARAM_TYPE_NONE,
+						   TEE_PARAM_TYPE_NONE);
+	if (param_types != exp_param_types) {
+		EMSG("Wrong param_types, exp %x, got %x", exp_param_types,


EMSG("Wrong param_types, exp %#"PRIx32", got #"PRIx32, exp_param_types, param_types);

Must use PRIx32 and PRIu32 with uint32_t typed arguments.

github-actions · 2021-02-15T01:09:26Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note that you can always re-open a closed pull request at any time.

lenormandfranck mentioned this pull request Nov 2, 2020

Add processing of code coverage for optee core OP-TEE/optee_os#4192

Draft

etienne-lms reviewed Nov 6, 2020

View reviewed changes

Review etienne 1

550fe60

Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

github-actions bot added the Stale label Dec 10, 2020

github-actions bot removed the Stale label Dec 15, 2020

github-actions bot added the Stale label Jan 14, 2021

jenswi-linaro removed the Stale label Jan 14, 2021

etienne-lms reviewed Jan 15, 2021

View reviewed changes

github-actions bot added the Stale label Feb 15, 2021

github-actions bot closed this Feb 21, 2021

xtest: Add test of gcov #457

xtest: Add test of gcov #457

Conversation

lenormandfranck commented Nov 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms Nov 6, 2020 • edited Loading

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

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

github-actions bot commented Dec 10, 2020

lenormandfranck commented Dec 14, 2020

github-actions bot commented Jan 14, 2021

jenswi-linaro commented Jan 14, 2021

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

github-actions bot commented Feb 15, 2021

etienne-lms Nov 6, 2020 •

edited

Loading