Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add processing of code coverage for optee core #4192

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
6 changes: 6 additions & 0 deletions core/core.mk
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,12 @@ include mk/lib.mk
base-prefix := $(sm)-
endif

ifeq ($(CFG_GCOV_SUPPORT),y)
libname = gcov
libdir = lib/libgcov
include mk/lib.mk
endif

ifeq ($(firstword $(subst /, ,$(CFG_CRYPTOLIB_DIR))),core)
# If a library can be compiled for both core and user space a base-prefix
# is needed in order to avoid conflicts in the output. However, if the
Expand Down
61 changes: 61 additions & 0 deletions lib/libgcov/core_dump_coverage.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/* SPDX-License-Identifier: BSD-2-Clause */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// SPDX-License-...

/*
* Copyright 2020 NXP
*/

#include <stdio.h>
#include <string.h>
#include <trace.h>

#include <gcov.h>
#include <optee_rpc_cmd.h>
#include <tee/tee_fs_rpc.h>
#include <tee_api_types.h>

#include "int_gcov.h"

/*
* Dump coverage data to REE FS
*
* The description is prepended to the filepath
* Sends the coverage data to the writer
*
* @desc Description for storage, null terminated string
* @filepath Name of the coverage file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @cov_data The coverage data to dump
* @cov_data_size Size of the coverage data
*/
static TEE_Result core_direct_dump_data(const char *desc, const char *filepath,
void *cov_data, uint32_t cov_data_size)
{
const char *core = "core/";
char new_filepath[COVERAGE_PATH_MAX];
uint32_t filepath_size = 0;

if (!desc || !filepath || !cov_data) {
EMSG("Wrong parameters");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

return TEE_ERROR_BAD_PARAMETERS;
}

filepath_size = strlen(core) + strlen(desc) + strlen(filepath) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (filepath_size >= COVERAGE_PATH_MAX) {
EMSG("Not enough space to store new filepath");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DMSG(), if we need this at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

return TEE_ERROR_SHORT_BUFFER;
}

filepath_size = 0;
strcpy(new_filepath + filepath_size, core);
filepath_size += strlen(core);

strcpy(new_filepath + filepath_size, desc);
filepath_size += strlen(desc);

strcpy(new_filepath + filepath_size, filepath);
filepath_size += strlen(filepath);

filepath_size++;

return gcov_dump_coverage_data(new_filepath, cov_data, cov_data_size);
}

gcov_dump_file_fn g_gcov_dump_fn = core_direct_dump_data;
141 changes: 141 additions & 0 deletions lib/libgcov/gcov.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
/* SPDX-License-Identifier: BSD-2-Clause */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// SPDX-...

/*
* Copyright 2020 NXP
*/

#include <string.h>
#include <trace.h>

#include <gcov.h>

#include "int_gcov.h"

static gcov_dump_writer s_writer_fn;

static bool isnullterm(const char *string, size_t maxlen, size_t *len)
{
size_t s_len = strnlen(string, maxlen);

if (s_len == maxlen)
return false;

if (len)
*len = s_len;

return true;
}

void __gcov_init(void *cov_unit_info)
{
if (!g_gcov_impl || !g_gcov_impl->init)
EMSG("No implementation");
else
g_gcov_impl->init(cov_unit_info);
}

void __gcov_merge_add(void *ctrs, uint32_t nb_ctrs)
{
if (!g_gcov_impl || !g_gcov_impl->merge_add)
EMSG("No implementation");
else
g_gcov_impl->merge_add(ctrs, nb_ctrs);
}

void __gcov_exit(void)
{
if (!g_gcov_impl || !g_gcov_impl->exit)
EMSG("No implementation");
else
g_gcov_impl->exit();
}

TEE_Result gcov_get_version(uint32_t *version)
{
/* We test if the implementation has implementeed the function */
if (!g_gcov_impl || !g_gcov_impl->get_version) {
EMSG("No implementation");
return TEE_ERROR_NOT_IMPLEMENTED;
}

if (version)
*version = g_gcov_impl->get_version();

return TEE_SUCCESS;
}

TEE_Result gcov_reset_coverage_data(void)
{
TEE_Result res = TEE_SUCCESS;

/* We test if the implementation has implementeed the function */
if (!g_gcov_impl || !g_gcov_impl->reset_all_coverage_data) {
EMSG("No implementation");
res = TEE_ERROR_NOT_IMPLEMENTED;
} else {
res = g_gcov_impl->reset_all_coverage_data();
}

return res;
}

TEE_Result register_gcov_dump_writer(gcov_dump_writer writer_fn)
{
s_writer_fn = writer_fn;

return TEE_SUCCESS;
}

TEE_Result gcov_dump_coverage_data(const char *filepath, char *cov_data,
uint32_t cov_data_size)
{
if (!filepath || !cov_data || !cov_data_size)
return TEE_ERROR_BAD_PARAMETERS;

if (!s_writer_fn) {
EMSG("Writer function not registered");
return TEE_ERROR_NOT_IMPLEMENTED;
}

if (!isnullterm(filepath, COVERAGE_PATH_MAX, NULL)) {
EMSG("filepath is not null terminated");
return TEE_ERROR_BAD_PARAMETERS;
}

return s_writer_fn(filepath, cov_data, cov_data_size);
}

TEE_Result gcov_dump_all_coverage_data(const char *desc)
{
TEE_Result res = TEE_SUCCESS;
const char *slash = "/";

if (!desc) {
desc = slash;
} else if (!isnullterm(desc, COVERAGE_PATH_MAX, NULL)) {
EMSG("description is not null terminated");
return TEE_ERROR_BAD_PARAMETERS;
}

/* We test if the implementation has implementeed the function */
if (!g_gcov_impl || !g_gcov_impl->dump_all_coverage_data ||
!g_gcov_dump_fn) {
EMSG("No implementation");
return TEE_ERROR_NOT_IMPLEMENTED;
}

if (g_gcov_impl->start_dump_all_coverage_data) {
res = g_gcov_impl->start_dump_all_coverage_data();
if (res) {
EMSG("Failed to start dump of coverage data");
return res;
}
}

res = g_gcov_impl->dump_all_coverage_data(g_gcov_dump_fn, desc);

if (g_gcov_impl->start_dump_all_coverage_data &&
g_gcov_impl->end_dump_all_coverage_data)
g_gcov_impl->end_dump_all_coverage_data();

return res;
}
52 changes: 52 additions & 0 deletions lib/libgcov/include/gcov.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/* SPDX-License-Identifier: BSD-2-Clause */
/*
* Copyright 2020 NXP
*/

#ifndef GCOV_H
#define GCOV_H

#include <tee_api_types.h>

/*
* Print debug information about the gcov support
*
* @version The version of the implementation
*/
TEE_Result gcov_get_version(uint32_t *version);

/*
* Reset all the coverage data
*/
TEE_Result gcov_reset_coverage_data(void);

/*
* Write the coverage data to filesystem in the file filepath
*
* @filepath Path of the file to write
* @cov_data Coverage data to write
* @cov_data_size Coverage data size
*/
typedef TEE_Result (*gcov_dump_writer)(const char *filepath, char *cov_data,
uint32_t cov_data_size);

TEE_Result register_gcov_dump_writer(gcov_dump_writer writer_fn);

/*
* Dump coverage data to REE FS
*
* @filepath Path of the coverage file
* @cov_data The coverage data to dump
* @cov_data_size Size of the coverage data
*/
TEE_Result gcov_dump_coverage_data(const char *filepath, char *cov_data,
uint32_t cov_data_size);

/*
* Dump the current state of the coverage data to filesystem
*
* @desc Description for storage, null terminated string
*/
TEE_Result gcov_dump_all_coverage_data(const char *desc);

#endif /* GCOV_H */
118 changes: 118 additions & 0 deletions lib/libgcov/int_gcov.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/* SPDX-License-Identifier: BSD-2-Clause */
/*
* Copyright 2020 NXP
*/

#ifndef INT_GCOV_H
#define INT_GCOV_H

#include <tee_api_types.h>

/*
* Maximum size of a coverage path
*/
#define COVERAGE_PATH_MAX 255

/*
* Initialize the data from a coverage unit
*
* @cov_unit_info The coverage data to initialize
*/
void __gcov_init(void *cov_unit_info);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does these function need to have a __ prefix?


/*
* Perform the merge_data operation for @p hProfData
*
* @ctrs List of counters to merge
* @nb_ctrs Number of counters to merge
*/
void __gcov_merge_add(void *ctrs, uint32_t nb_ctrs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use unsigned int or size_t instead of uint32_t.


/*
* Coverage processing can be stopped
*/
void __gcov_exit(void);

/*
* Dump coverage data to REE FS
*
* The description is prepended to the filepath
*
* @desc Description for storage, null terminated string
* @filepath Name of the coverage file
* @cov_data The coverage data to dump
* @cov_data_size Size of the coverage data
*/
typedef TEE_Result (*gcov_dump_file_fn)(const char *desc, const char *filepath,
void *cov_data, uint32_t cov_data_size);

/*
* static Implementation of dump function to use
*/
extern gcov_dump_file_fn g_gcov_dump_fn;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please drop the g_ prefix.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the _t suffix for?

/*
* Get the version of the implementation
*/
uint32_t (*get_version)(void);
/*
* Initialize the data of the coverage unit
*
* @cov_unit_info The coverage unit info to initialize
*/
void (*init)(void *cov_unit_info);

/*
* Perform the merge_data operation for @p hProfData
*
* @ctrs List of counters to merge
* @nb_ctrs Number of counters to merge
*/
void (*merge_add)(void *ctrs, uint32_t nb_ctrs);

/*
* Coverage processing can be stopped
*/
void (*exit)(void);

/*
* Reset all the coverage data
*/
TEE_Result (*reset_all_coverage_data)(void);

/*
* Start dump of all the coverage data
*/
TEE_Result (*start_dump_all_coverage_data)(void);

/*
* Dump all the coverage data to filesystem
*
* @dump_fn The function to call to dump data
* @desc Description for storage, null terminated string
*/
TEE_Result (*dump_all_coverage_data)(gcov_dump_file_fn dump_fn,
const char *desc);

/*
* End dump of all the coverage data
*/
void (*end_dump_all_coverage_data)(void);
};

/*
* static Implementation of gcov to use
*/
extern const struct gcov_impl_t *g_gcov_impl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please drop the g_ prefix.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#endif /* INT_GCOV_H */
10 changes: 10 additions & 0 deletions lib/libgcov/sub.mk
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
global-incdirs-y += include
srcs-y += gcov.c

# Select the dumper of coverage to use in the library depending on the
# component being built
ifeq ($(sm-core),y)
srcs-y += core_dump_coverage.c
else
srcs-y += ta_dump_coverage.c
endif