Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v3 2/3] gcov: Clang support
To:     Tri Vo <trong@android.com>
Cc:     ghackmann@android.com, ndesaulniers@google.com,
        linux-kernel@vger.kernel.org, kernel-team@android.com,
        yamada.masahiro@socionext.com, Trilok Soni <tsoni@quicinc.com>,
        Prasad Sodagudi <psodagud@quicinc.com>,
        Daniel Mentz <danielmentz@google.com>,
        Petri Gynther <pgynther@google.com>
References: <20190122233749.42220-1-trong@android.com>
 <20190122233749.42220-3-trong@android.com>
From:   Peter Oberparleiter <oberpar@linux.ibm.com>
Date:   Tue, 5 Mar 2019 14:57:55 +0100
MIME-Version: 1.0
In-Reply-To: <20190122233749.42220-3-trong@android.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <faa5a621-1f21-3136-58e7-9215c4f4d079@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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

-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany