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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <faa5a621-1f21-3136-58e7-9215c4f4d079@linux.ibm.com>
Date:   Tue, 5 Mar 2019 14:57:55 +0100
From:   Peter Oberparleiter <oberpar@...ux.ibm.com>
To:     Tri Vo <trong@...roid.com>
Cc:     ghackmann@...roid.com, ndesaulniers@...gle.com,
        linux-kernel@...r.kernel.org, kernel-team@...roid.com,
        yamada.masahiro@...ionext.com, Trilok Soni <tsoni@...cinc.com>,
        Prasad Sodagudi <psodagud@...cinc.com>,
        Daniel Mentz <danielmentz@...gle.com>,
        Petri Gynther <pgynther@...gle.com>
Subject: Re: [PATCH v3 2/3] gcov: Clang support

On 23.01.2019 00:37, Tri Vo wrote:
> From: Greg Hackmann <ghackmann@...roid.com>
> 
> 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@...gle.com>
> Co-authored-by: Tri Vo <trong@...roid.com>
> Signed-off-by: Greg Hackmann <ghackmann@...roid.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@...gle.com>
> Signed-off-by: Tri Vo <trong@...roid.com>
> Tested-by: Trilok Soni <tsoni@...cinc.com>
> Tested-by: Prasad Sodagudi <psodagud@...cinc.com>
> Tested-by: Tri Vo <trong@...roid.com>
> Tested-by: Daniel Mentz <danielmentz@...gle.com>
> Tested-by: Petri Gynther <pgynther@...gle.com>
> ---
>  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@...ux.ibm.com>

-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany