Re: [PATCH 3/4] gcov: add gcov profiling infrastructure

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 02 Jun 2009 13:44:02 +0200
Peter Oberparleiter <oberpar@linux.vnet.ibm.com> wrote:

> From: Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
> 
> Enable the use of GCC's coverage testing tool gcov [1] with the Linux
> kernel. gcov may be useful for:
> 
>  * debugging (has this code been reached at all?)
>  * test improvement (how do I change my test to cover these lines?)
>  * minimizing kernel configurations (do I need this option if the
>    associated code is never run?)
> 
> The profiling patch incorporates the following changes:
> 
>  * change kbuild to include profiling flags
>  * provide functions needed by profiling code
>  * present profiling data as files in debugfs
> 
> Note that on some architectures, enabling gcc's profiling option
> "-fprofile-arcs" for the entire kernel may trigger compile/link/
> run-time problems, some of which are caused by toolchain bugs and
> others which require adjustment of architecture code.
> 
> For this reason profiling the entire kernel is initially restricted
> to those architectures for which it is known to work without changes.
> This restriction can be lifted once an architecture has been tested
> and found compatible with gcc's profiling. Profiling of single files
> or directories is still available on all platforms (see config help
> text).
> 
> 
> [1] http://gcc.gnu.org/onlinedocs/gcc/Gcov.html
> 
>
> ...
>
> +/*
> + * __gcov_init is called by gcc-generated constructor code for each object
> + * file compiled with -fprofile-arcs.
> + */

How does this work?  At what time does gcc call the constructors, and
in what context are they called, etc?

IOW, please teach me about -fprofile-arcs :)


> +void __gcov_init(struct gcov_info *info)
> +{
> +	static unsigned int gcov_version;
> +
> +	mutex_lock(&gcov_lock);
> +	if (gcov_version == 0) {
> +		gcov_version = info->version;
> +		/*
> +		 * Printing gcc's version magic may prove useful for debugging
> +		 * incompatibility reports.
> +		 */
> +		pr_info("version magic: 0x%x\n", gcov_version);

Will this be spat out as simply

version magic: 0x1234

?  If so, that'll be rather obscure because people won't know what
subsystem printed it.  Prefixing this (and all other printks) with
"gcov: " would fix that.

> +	}
> +	/*
> +	 * Add new profiling data structure to list and inform event
> +	 * listener.
> +	 */
> +	info->next = gcov_info_head;
> +	gcov_info_head = info;
> +	if (gcov_events_enabled)
> +		gcov_event(GCOV_ADD, info);
> +	mutex_unlock(&gcov_lock);
> +}
> +EXPORT_SYMBOL(__gcov_init);
> +
>
> ...
>
> +#ifdef CONFIG_MODULES
> +static inline int within(void *addr, void *start, unsigned long size)
> +{
> +	return ((addr >= start) && (addr < start + size));
> +}

That's our fourth implementation of within() (at least).  All basically
identical.  Whine.

>
> ...
>
> +static int __init gcov_persist_setup(char *str)
> +{
> +	int val;
> +	char delim;
> +
> +	if (sscanf(str, "%d %c", &val, &delim) != 1) {
> +		pr_warning("invalid gcov_persist parameter '%s'\n", str);
> +		return 0;
> +	}
> +	pr_info("setting gcov_persist to %d\n", val);
> +	gcov_persist = val;
> +
> +	return 1;
> +}
> +__setup("gcov_persist=", gcov_persist_setup);

hm, what's the input format here?  It looks like

	gcov_persist=1 x

and " x" is checked for, but ignored.

Confused.  What's this all for?  Can't we just us plain old strtoul(), etc?

> +/*
> + * seq_file.start() implementation for gcov data files. Note that the
> + * gcov_iterator interface is designed to be more restrictive than seq_file
> + * (no start from arbitrary position, etc.), to simplify the iterator
> + * implementation.
> + */
> +static void *gcov_seq_start(struct seq_file *seq, loff_t *pos)
> +{
> +	loff_t i;
> +
> +	gcov_iter_start(seq->private);
> +	for (i = 0; i < *pos; i++) {
> +		if (gcov_iter_next(seq->private))
> +			return NULL;
> +	}
> +	return seq->private;
> +}

This looks like it could be very expensive if used inappropriately.  I
guess the answer to that is "don't use it inapprpriately", yes?

>
> ...
>
> +struct gcov_info *gcov_info_dup(struct gcov_info *info)
> +{
> +	struct gcov_info *dup;
> +	unsigned int i;
> +	unsigned int active;
> +
> +	/* Duplicate gcov_info. */
> +	active = num_counter_active(info);
> +	dup = kzalloc(sizeof(struct gcov_info) +
> +		      sizeof(struct gcov_ctr_info) * active, GFP_KERNEL);

How large can this allocation be?

> +	if (!dup)
> +		return NULL;
> +	dup->version		= info->version;
> +	dup->stamp		= info->stamp;
> +	dup->n_functions	= info->n_functions;
> +	dup->ctr_mask		= info->ctr_mask;
> +	/* Duplicate filename. */
> +	dup->filename		= kstrdup(info->filename, GFP_KERNEL);
> +	if (!dup->filename)
> +		goto err_free;
> +	/* Duplicate table of functions. */
> +	dup->functions = kmemdup(info->functions, info->n_functions *
> +				 get_fn_size(info), GFP_KERNEL);
> +	if (!dup->functions)
> +		goto err_free;
> +	/* Duplicate counter arrays. */
> +	for (i = 0; i < active ; i++) {
> +		struct gcov_ctr_info *ctr = &info->counts[i];
> +		size_t size = ctr->num * sizeof(gcov_type);
> +
> +		dup->counts[i].num = ctr->num;
> +		dup->counts[i].merge = ctr->merge;
> +		dup->counts[i].values = kmemdup(ctr->values, size, GFP_KERNEL);
> +		if (!dup->counts[i].values)
> +			goto err_free;
> +	}
> +	return dup;
> +
> +err_free:
> +	gcov_info_free(dup);
> +	return NULL;
> +}
> +

It all looks very nice to me.