Add processing of code coverage for optee core #4192

lenormandfranck · 2020-11-02T08:25:45Z

Code coverage is the notion of knowing which part of the code is executed at runtime during a scenario or normal execution.

Code coverage is a useful tool with different purposes:

Verify which lines of code are executed during testing
Optimize branching prediction by feeding scenario at compilation

The code coverage processing is already available for user code or other projects like the Linux kernel using its implementation in different compiler like GCC or Clang.

It was not available in optee for 3 reasons:

Compilation not configured for it
No gcov library linked to process the structure added by the compiler into coverage data files
No method to store the generated data files to place accessible to the user

This pull request adresses these issues in order to add code coverage analysis to optee.

The code coverage is added using 3 main components:

libgcov: library which handles:
-- keep a list of the structure added by the compiler containing code coverage data
-- reset all coverage data
-- write all coverage data
-- Transform the code coverage structures into data to be written following gcov gcda format
PTA gcov: Allow to interact with the code coverage of the code or write coverage data file
coverage data file writer: Write coverage data file to REE FS using new RPC

Code coverage support should be enabled only when developping/debugging as it adds overhead at runtime.
It also increases the size of the binary generated (gcc 9.2.0):

tee.elf: 18%
libutils: 10%
libutee: 5%
TA aes_perf (from optee_test): 27%
The time to build is also increased by ~25%

Limitations:

Clang suport not tested but should be easy as it follows gcov data files format.
No code coverage of the TA: Current implementation can support it but it needs to call constructor function added by the compiler when loading the TA which is not already done.

Supporting PR:

optee_client: Write of coverage data file in REE FS optee_client#231
optee_test: xtest: Add test of gcov optee_test#457

ta_from_ta.txt.txt
core_from_ca.txt.txt
core_from_ta.txt.txt

This patch adds the flags to configure the use of gcov: - CFG_CORE_GCOV_SUPPORT: enable the coverage for the core - CFG_TA_GCOV_SUPPORT: enable the coverage for TAs If any of them is enabled, CFG_GCOV_SUPPORT is enabled These flags are exported to the devkit. Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

For imx 6 qnd 7 targets, the build fails because TEE_RAM_VA_SIZE is too small. This patch increase it from 512ko to 2 Mo when coverage is enabled. Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

The code coverage is a metric measuring the lines of code executed in a code base during execution. If the code coverage is generated after a test suite is ran, it will give the coverage of code tested by the test suite. It gives the possibility to identify the code which is not tested. The gcov code coverage is supported by different compiler: - GCC (gcc.gnu.org/onlinedocs/gcc/Gcov.html) - clang compiler (clang.llvm.org/docs/SourceBasedCodeCoverage.html) It allows to analyze which part of the code is executed. The interface with a specific implementation of gcov is defined usign the structure gcov_impl_t. The library implements the symbol required for gcov: - __gcov_init: called by each coverage unit - __gcov_merge_add - __gcov_exit The calls to these symbols are redirected to the implementation for the compiler used. The library role is to transform the coverage data in program memory to a valid gcov data file. For this it interacts with: - user/TA: Control (Reset or Dump) the coverage data - Function to store data: Send the data to it for storage in REE FS In order to retrieve the coverage (*.gcda) for a specific UC, it is recommended to do the following actions: - clear the coverage - perform the UC - save the coverage Note: The core and the TAs are linked against different instances of the libgcov. The libgcov of the core can be controlled by the gcov PTA. The TA instances are controlled calling the libgcov API. Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

The compiler GCC implements gcov code coverage and this patch adds the support to handle the structure in memory generated by the compiler. It also handles the transformation of the structure in memory into gcov data files. Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

The coverage data are generated during execution, in order to exploit it, the data can be stored to REE FS in clear text which is done by the writer. The writer receives the data to store and call the dedicated RPC to instruct the TEE client to store it in REE FS. Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

The PTA has two main purposes: - Control the code coverage of the core: - PTA_CMD_GCOV_GET_VERSION - PTA_CMD_GCOV_CORE_RESET - PTA_CMD_GCOV_CORE_DUMP_ALL - Write code coverage data from a TA (called by libgcov) - PTA_CMD_GCOV_DUMP Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

The code coverage is performed at runtime by keeping in memory structures holding the releveant data: In order to generate the code coverage, two steps are required: - Instruct the compiler to generate coverage information structure and functions to initialize them: flag --coverage. - Call the constructors added to initialize the coverage information and register them to the libgcov (modification of boot.c) When an object file (.o) is created with code coverage support, a note (.gcno) file is created with it. The note file contains informations used when creating the code coverage report. The support of code coverage in compiled code increases its size (~15%) and the compilation time (~25%). Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

This script ease the generation of code coverage report. A report is based on a codebase, note files (generated at compilation) and data files (generated at runtime). The report is generated using lcov and present the code coverage as a percentage of lines executed compared to the total number of lines in the codebase. The script will copy all the note and data files in a temporary directory allowing to reuse the generated artifact and checks that the note and data file are correctly organised (the files must be in the same directory). The script also allow to generate an html output using genhtml and to view it. Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

jenswi-linaro

Comments for commit "compil: Fix link error TEE_RAM_VA_SIZE is too small"

Perhaps we could make a generic solution instead? This is bound to be needed for other platforms too.

jenswi-linaro · 2020-11-02T11:56:22Z

core/arch/arm/plat-imx/conf.mk

+#                 = BIT(CORE_MMU_PGDIR_SHIFT)
+# with LPAE disabled: #define CORE_MMU_PGDIR_SHIFT	20
+#                 = BIT(20)
+#                 = 0x80000 (512 Ko)


That should be 0x100000 (1MB)

In effect, my bad

jenswi-linaro

Comments for commit "libgcov: Add library adding the gcov support"

It seems that some of the dynamic registration adds quite a bit of code just to check that stuff is properly registered.

jenswi-linaro · 2020-11-02T15:59:05Z

lib/libgcov/int_gcov.h

+/*
+ * Macro to register the implementation of gcov
+ */
+#define REGISTR_GCOV_IMPL(p_impl) const struct gcov_impl_t *g_gcov_impl = p_impl


Do we need dynamic registration? Isn't this know when at compile time?

This regitration of the gcov implementation is static.
The macro is used to set "extern const struct gcov_impl_t *g_gcov_impl;"

Is the dynamic registration you are speaking about is for "s_writer_fn" () using register_gcov_dump_writer (./lib/libgcov/gcov.c)?

Why all these function pointers if everything is static? It makes it harder to follow the code. If there's good reason it's fine.

jenswi-linaro · 2020-11-02T16:07:02Z

lib/libgcov/core_dump_coverage.c

+		return TEE_ERROR_BAD_PARAMETERS;
+	}
+
+	filepath_size = strlen(core) + strlen(desc) + strlen(filepath) + 1;


Would prefer something based on snprintf() for simplicity, perhaps like:

if (snprintf(buf, sizeof(buf), "core/%s/%s", desc, fname) >= sizeof(buf))

I will use snprintf to simplify the code however not this exact line as I want to check I handle the NULL teminating character properly.

jenswi-linaro · 2020-11-02T16:14:12Z

lib/libgcov/core_dump_coverage.c

+ * Sends the coverage data to the writer
+ *
+ * @desc           Description for storage, null terminated string
+ * @filepath       Name of the coverage file


If it's the name of the file wouldn't fname be more appropriate?

The filepath passed to the function is a patch corresponding to the absolute path of the source file used at compilation. I will replace the "Name" by "Path".

What do you mean by absolute path? Is that the dirname() part of the absolute file name?

jenswi-linaro · 2020-11-02T16:17:13Z

lib/libgcov/core_dump_coverage.c

+	uint32_t filepath_size = 0;
+
+	if (!desc || !filepath || !cov_data) {
+		EMSG("Wrong parameters");


DMSG(), if we need this at all.
Perhaps a plain assert(desc && fname && cov_data); instead of the if and everything would be more useful.

jenswi-linaro · 2020-11-02T16:17:25Z

lib/libgcov/core_dump_coverage.c

+
+	filepath_size = strlen(core) + strlen(desc) + strlen(filepath) + 1;
+	if (filepath_size >= COVERAGE_PATH_MAX) {
+		EMSG("Not enough space to store new filepath");


DMSG(), if we need this at all.

jenswi-linaro

Some comments for "libgcov: Add implementation of gcov for GCC"

jenswi-linaro · 2020-11-02T16:44:42Z

lib/libgcov/gcov_gcc.c

+#include "int_gcov.h"
+
+#define MAGIC_VALUE(c1, c2, c3, c4) \
+	(((uint32_t)c1 << 24) + ((uint32_t)c2 << 16) + ((uint32_t)c3 << 8) + \


We have a the SHIFT_U32() for this, SHIFT_U32(c1, 24)...

thank you for the pointer

jenswi-linaro · 2020-11-02T16:46:40Z

lib/libgcov/gcov_gcc.c

+};
+
+static const uint32_t ctr_tags[LAST_CTR_SUPPORTED] = {
+	[CTR_ARCS] = 0x01a10000,


What does 0x01a10000 mean?

It is the ID for the counter arcs used in gcov implementation in GCC. I will document the array.

jenswi-linaro · 2020-11-02T16:48:58Z

lib/libgcov/gcov_gcc.c

+};
+
+/*
+ * Structure create by gcc to hold code coverade data of an object


s/create/created/
s/coverade/coverage/

jenswi-linaro · 2020-11-02T17:02:56Z

lib/libgcov/gcov_gcc.c

+	/* Pointer to object containing the function */
+	const struct object_data *object;
+	/* Identifier */
+	uint32_t id;


uint32_t or unsigned int?

"unsigned int" is not defined strictly as a 32 bit value by the C spec. GCC is using a 32 bit value (ref get_gcov_type in gcc/coverage.c).

jenswi-linaro · 2020-11-02T17:06:38Z

lib/libgcov/gcov_gcc.c

+		/* Number of counters of the type */
+		uint32_t nb_ctrs;
+		/* Array of counters value */
+		uint64_t *ctrs;


uint64_t or unsigned long?

I rechecked looking at gcc code for gcov, for arm, it is uint64_t however for other architectures, there could be some issues, I will look into it.

jenswi-linaro · 2020-11-02T17:36:52Z

lib/libgcov/gcov_gcc.c

+	struct object_data *od = s_int_data.linked_list_objects;
+	const struct func_data *fd = NULL;
+	const struct ctr_data *cd = NULL;
+	uint32_t cur_fn = 0;


I'd prefer unsigned int, same at next line below.

What is the advantage of "unsigned int"? I like uint32_t because it gives precisely the range of the variable and is shorter to write.

I'm open to define u_int in <types_ext.h> if it's too tedious to type unsigned int.
When you use a fixed width integer you signal that this specific width is important, often because it's supposed to fit in a hardware defined register or some ABI. I'm not sure why exactly 32 bits should be important here. When using an unsigned int it's often not the exact range of the type that's important, but we know that the capacity has to be large enough for what's needed though. If you suspect that a unsigned int isn't large enough the next choice is often size_t, or perhaps unsigned long depending on the case.

jenswi-linaro · 2020-11-02T17:39:28Z

lib/libgcov/gcov_gcc.c

+					       sizeof(*cd->ctrs) * cd->nb_ctrs);
+			}
+		}
+	} while ((od = od->next));


We try to avoid assignments inside expressions. Consider:

for (od = s_int_data.linked_list_objects; od; od = od->next) { ... }

Sure, it is cleaner

jenswi-linaro · 2020-11-02T18:02:32Z

lib/libgcov/gcov_gcc.c

+ *
+ * From gcc/gcc/gcov-dump.c
+ *
+ * A FUNCTION block is generally foloowed by blocks for counters, 1 bloc per


s/foloowed/followed/
s/bloc /block /

jenswi-linaro · 2020-11-02T18:08:14Z

lib/libgcov/gcov_gcc.c

+ * counter
+ *
+ * struct gcda_file {
+ *     uint32_t magic;


gcov-dump.c seems to generally not use fixed width integers. unsigned int is used instead of uint32_t

The use of these values is to map as close to possible to the binary representation of the file.

jenswi-linaro · 2020-11-02T18:23:10Z

lib/libgcov/gcov_gcc.c

+ *                 uint32_t cfg_checksum;
+ *             } gcda_functions;
+ *             {
+ *                 uint64_t values;


I guess that means implicit padding after values, but I can't see anything of that below.

No padding, The block can either contains a "gcda_functions" or "gcda_counters" structure.
Is there some confusion?

sizeof(*gcda_block) is the same regardless if gcda_block->gcda_functions or gcda_block->gcda_counters is used.

jenswi-linaro · 2020-12-02T06:45:35Z

lib/libgcov/int_gcov.h

+/*
+ * static Implementation of gcov to use
+ */
+extern const struct gcov_impl_t *g_gcov_impl;


Please drop the g_ prefix.

jenswi-linaro · 2020-12-02T06:45:53Z

lib/libgcov/int_gcov.h

+ * Define the functions an implementation shall provide to process the coverage
+ * data
+ */
+struct gcov_impl_t {


What is the _t suffix for?

jenswi-linaro · 2020-12-02T06:46:10Z

lib/libgcov/int_gcov.h

+/*
+ * static Implementation of dump function to use
+ */
+extern gcov_dump_file_fn g_gcov_dump_fn;


Please drop the g_ prefix.

jenswi-linaro · 2020-12-02T06:46:48Z

lib/libgcov/int_gcov.h

+ *
+ * @cov_unit_info  The coverage data to initialize
+ */
+void __gcov_init(void *cov_unit_info);


Does these function need to have a __ prefix?

jenswi-linaro · 2020-12-02T06:47:15Z

lib/libgcov/int_gcov.h

+ * @ctrs     List of counters to merge
+ * @nb_ctrs  Number of counters to merge
+ */
+void __gcov_merge_add(void *ctrs, uint32_t nb_ctrs);


Please use unsigned int or size_t instead of uint32_t.

github-actions · 2021-01-02T01:36:11Z

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 issue at any time.

etienne-lms

I believe this is still in progress.
Can we add an enhancement label to this P-R so that it does not audo close?

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

core/pta/gcov.c

+ *          [in]     memref[0]	    description
+ *          [none]
+ *          [none]
+ *          [none]


Prefer have this ABI description in lib/libutee/include/pta_gcov.h.

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

core/pta/gcov.c

+ * @psess   The Session
+ * @cmd     The command to execute
+ * @ptypes  The types of the parameters
+ * @params  The parameters


argument description not needed, implicit since PTA format (pseudo_ta_register() below)

etienne-lms · 2020-11-10T08:14:07Z

core/arch/arm/kernel/boot.c

+ * These functions are created automatically by the compiler.
+ * Each constructor function will call __gcov_int() for their function.
+ */
+#if defined(CFG_CORE_SANITIZE_KADDRESS) || defined(CFG_CORE_GCOV_SUPPORT)


could remove this #if, add __aybe_unused attribute to the function
and in init_asan() below:

static void init_asan(void) { if (IS_ENABLED(CFG_CORE_GCOV_SUPPORT)) init_run_constructors(); }

etienne-lms · 2020-11-10T08:15:39Z

core/include/optee_rpc_cmd.h

+ * [in]     memref[1]	    filepath
+ * [in]     memref[2]	    code coverage data
+ */
+#define OPTEE_RPC_CMD_GCOV 12


indent with 2 tabs to align with above OPTEE_RPC_CMD_FTRACE.

etienne-lms · 2020-11-10T08:15:54Z

core/pta/gcov.c

+
+/*
+ * Name of the TA
+ */


can remove this inline comment

etienne-lms · 2020-11-10T08:18:40Z

core/pta/gcov.c

+		return pta_gcov_core_dump_all(ptypes, params);
+	default:
+		EMSG("Command %d not supported", cmd);
+		break;


suggestion: return TEE_ERROR_BAD_PARAMETERS; straight here?

etienne-lms · 2020-11-10T08:19:00Z

core/tee/tee_gcov_writer.c

+				  uint32_t cov_data_size)
+{
+	TEE_Result res = TEE_SUCCESS;
+	struct thread_param params[2] = {};


etienne-lms · 2020-11-10T08:19:59Z

lib/libgcov/core_dump_coverage.c

@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: BSD-2-Clause */


// SPDX-License-...

etienne-lms · 2020-11-10T08:20:21Z

core/tee/tee_gcov_writer.c

@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: BSD-2-Clause */


// SPDX-...

etienne-lms · 2020-11-10T08:20:48Z

lib/libgcov/gcov.c

@@ -0,0 +1,141 @@
+/* SPDX-License-Identifier: BSD-2-Clause */


// SPDX-...

jforissier · 2021-01-07T09:02:10Z

I believe this is still in progress.
Can we add an enhancement label to this P-R so that it does not audo close?

I have removed the stale label and added enhancement.

lenormandfranck added 8 commits November 2, 2020 09:34

compil: Fix link error TEE_RAM_VA_SIZE is too small

89e521c

For imx 6 qnd 7 targets, the build fails because TEE_RAM_VA_SIZE is too small. This patch increase it from 512ko to 2 Mo when coverage is enabled. Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>

lenormandfranck force-pushed the coverage branch from 0b0e93e to 08d97f1 Compare November 2, 2020 08:35

jenswi-linaro reviewed Nov 2, 2020

View reviewed changes

jenswi-linaro reviewed Dec 2, 2020

View reviewed changes

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

github-actions bot closed this Jan 7, 2021

etienne-lms reviewed Jan 7, 2021

View reviewed changes

jenswi-linaro reopened this Jan 7, 2021

jforissier added enhancement and removed Stale labels Jan 7, 2021

		@@ -0,0 +1,141 @@
		/* SPDX-License-Identifier: BSD-2-Clause */

Add processing of code coverage for optee core #4192

Are you sure you want to change the base?

Add processing of code coverage for optee core #4192

Conversation

lenormandfranck commented Nov 2, 2020

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro 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

jenswi-linaro 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

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 Jan 2, 2021

etienne-lms 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

jforissier commented Jan 7, 2021