Re: [PATCH v3 2/3] gcov: Clang support

[Peter Oberparleiter]

On 23.01.2019 00:37, Tri Vo wrote:
> From: Greg Hackmann <ghackmann@xxxxxxxxxxx>
> 
> LLVM uses profiling data that's deliberately similar to GCC, but has a very
> different way of exporting that data.  LLVM calls llvm_gcov_init() once per
> module, and provides a couple of callbacks that we can use to ask for more
> data.
> 
> We care about the "writeout" callback, which in turn calls back into
> compiler-rt/this module to dump all the gathered coverage data to disk:
> 
>    llvm_gcda_start_file()
>      llvm_gcda_emit_function()
>      llvm_gcda_emit_arcs()
>      llvm_gcda_emit_function()
>      llvm_gcda_emit_arcs()
>      [... repeats for each function ...]
>    llvm_gcda_summary_info()
>    llvm_gcda_end_file()
> 
> This design is much more stateless and unstructured than gcc's, and is
> intended to run at process exit.  This forces us to keep some local state
> about which module we're dealing with at the moment.  On the other hand, it
> also means we don't depend as much on how LLVM represents profiling data
> internally.
> 
> See LLVM's lib/Transforms/Instrumentation/GCOVProfiling.cpp for more
> details on how this works, particularly GCOVProfiler::emitProfileArcs(),
> GCOVProfiler::insertCounterWriteout(), and GCOVProfiler::insertFlush().
> 
> Co-authored-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> Co-authored-by: Tri Vo <trong@xxxxxxxxxxx>
> Signed-off-by: Greg Hackmann <ghackmann@xxxxxxxxxxx>
> Signed-off-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> Signed-off-by: Tri Vo <trong@xxxxxxxxxxx>
> Tested-by: Trilok Soni <tsoni@xxxxxxxxxxx>
> Tested-by: Prasad Sodagudi <psodagud@xxxxxxxxxxx>
> Tested-by: Tri Vo <trong@xxxxxxxxxxx>
> Tested-by: Daniel Mentz <danielmentz@xxxxxxxxxx>
> Tested-by: Petri Gynther <pgynther@xxxxxxxxxx>
> ---
>  kernel/gcov/Kconfig  |   3 +-
>  kernel/gcov/Makefile |   1 +
>  kernel/gcov/clang.c  | 555 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 558 insertions(+), 1 deletion(-)
>  create mode 100644 kernel/gcov/clang.c
> 

[...]

> diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c
> new file mode 100644
> index 000000000000..f9acdee3d578
> --- /dev/null
> +++ b/kernel/gcov/clang.c
> @@ -0,0 +1,555 @@

[...]

> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/printk.h>
> +#include <linux/ratelimit.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include "gcov.h"
> +
> +#define GCOV_TAG_FUNCTION_LENGTH	3

This define seems to be unused and should be removed.

[...]

> +/**
> + * gcov_info_unlink - unlink/remove profiling data set from the list
> + * @prev: previous profiling data set
> + * @info: profiling data set
> + */
> +void gcov_info_unlink(struct gcov_info *prev, struct gcov_info *info)
> +{
> +	if (info)
> +		list_del(&info->head);

The if-clause can be removed since @info is always non-NULL.

[...]

> +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn)
> +{
> +	size_t cv_size; /* counter values size */
> +

This empty line should be removed.

[...]

Rest looks good! With these minor changes applied:

Reviewed-by: Peter Oberparleiter <oberpar@xxxxxxxxxxxxx>

-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany