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 2/4] 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,
        Greg Hackmann <ghackmann@google.com>,
        Trilok Soni <tsoni@quicinc.com>,
        Prasad Sodagudi <psodagud@quicinc.com>
References: <20190114210426.177543-1-trong@android.com>
 <20190114210426.177543-3-trong@android.com>
From:   Peter Oberparleiter <oberpar@linux.ibm.com>
Date:   Wed, 16 Jan 2019 17:06:36 +0100
MIME-Version: 1.0
In-Reply-To: <20190114210426.177543-3-trong@android.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <6c97cdb9-7b7d-e95b-567e-1938ca7f4ac2@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 14.01.2019 22:04, Tri Vo wrote:
> From: Greg Hackmann <ghackmann@google.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().
> 
> Signed-off-by: Greg Hackmann <ghackmann@google.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>
> ---
>  kernel/gcov/Kconfig  |   5 +
>  kernel/gcov/Makefile |   1 +
>  kernel/gcov/clang.c  | 531 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 537 insertions(+)
>  create mode 100644 kernel/gcov/clang.c

Please consider adding a short section detailing usage differences
between GCC and Clang coverage to the GCOV documentation at

  Documentation/dev-tools/gcov.rst

> diff --git a/kernel/gcov/clang.c b/kernel/gcov/clang.c
> new file mode 100644
> index 000000000000..b00795d28137
> --- /dev/null
> +++ b/kernel/gcov/clang.c

[...]

> +/**
> + * gcov_info_add - add up profiling data
> + * @dest: profiling data set to which data is added
> + * @source: profiling data set which is added
> + *
> + * Adds profiling counts of @source to @dest.
> + */
> +void gcov_info_add(struct gcov_info *dst, struct gcov_info *src)
> +{
> +	struct gcov_fn_info *dfn_ptr;
> +	struct gcov_fn_info *sfn_ptr = list_first_entry_or_null(&src->functions,
> +			struct gcov_fn_info, head);
> +
> +	list_for_each_entry(dfn_ptr, &dst->functions, head) {
> +		u32 i;
> +
> +		for (i = 0; i < sfn_ptr->num_counters; i++)
> +			dfn_ptr->counters[i] += sfn_ptr->counters[i];
> +
> +		if (!list_is_last(&sfn_ptr->head, &src->functions))

This check seems wrong - both dst and src should contain the same amount
of functions and counters per function (the GCC support is based on the
same assumption).

Even if not, the next iteration would try to add counters for different
functions which would be incorrect and could cause access violations due
to differing values for num_counters.

> +			sfn_ptr = list_next_entry(sfn_ptr, head);
> +	}
> +}
> +
> +static struct gcov_fn_info *gcov_fn_info_dup(struct gcov_fn_info *fn)
> +{
> +	struct gcov_fn_info *fn_dup = kmemdup(fn, sizeof(*fn),
> +			GFP_KERNEL);
> +	if (!fn_dup)
> +		return NULL;
> +	INIT_LIST_HEAD(&fn_dup->head);
> +
> +	fn_dup->name = kstrdup(fn->name, GFP_KERNEL);

Since struct gcov_fn_info does not define a member named 'name', this
should fail during compilation. I saw that this is fixed with the next
patch, but this really needs to be merged into this one.

> +	if (!fn_dup->name)
> +		goto err_name;
> +
> +	fn_dup->counters = kmemdup(fn->counters,
> +			fn->num_counters * sizeof(fn->counters[0]),
> +			GFP_KERNEL);

Please consider using vmalloc() here (like the GCC support does) because
the number of counters can be quite large and there might be situations
where the required amount of physically contiguous memory is not
available at run-time.

> +	if (!fn_dup->counters)
> +		goto err_counters;
> +
> +	return fn_dup;
> +
> +err_counters:
> +	kfree(fn_dup->name);
> +err_name:
> +	kfree(fn_dup);
> +	return NULL;
> +}
> +
> +/**
> + * gcov_info_dup - duplicate profiling data set
> + * @info: profiling data set to duplicate
> + *
> + * Return newly allocated duplicate on success, %NULL on error.
> + */
> +struct gcov_info *gcov_info_dup(struct gcov_info *info)
> +{
> +	struct gcov_info *dup;
> +	struct gcov_fn_info *fn, *tmp;
> +
> +	dup = kmemdup(info, sizeof(*dup), GFP_KERNEL);
> +	if (!dup)
> +		return NULL;
> +	INIT_LIST_HEAD(&dup->head);
> +	INIT_LIST_HEAD(&dup->functions);
> +	dup->filename = kstrdup(info->filename, GFP_KERNEL);

To be consistent, please also check for a failed allocation here.

> +
> +	list_for_each_entry_safe(fn, tmp, &info->functions, head) {

It should not be necessary to use the _safe variant here as
info->functions is not modified during traversal.


-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany