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

Peter Oberparleiter
2009-06-03T11:59:17Z

Andrew Morton wrote:
> On Tue, 02 Jun 2009 13:44:02 +0200
> Peter Oberparleiter <oberpar@linux.vnet.ibm.com> wrote:
>> +/*
>> + * __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 :)

No problem :) -fprofile-arcs cause gcc to insert profiling code which
relies on gcc's constructor mechanism for initialization purposes (so
the two are separate things while the former makes use of the latter).
gcc's constructor mechanism also has uses besides -fprofile-arcs, such
as calling C++ object constructors and the like.

In userland, constructor functions are called by one of the gcc
libraries just before the main() function. For the kernel, the kernel
constructor patch mimics a similar behavior:
* for code which was compiled in, constructor functions are called
in do_basic_setup(), just before processing initcalls
* for modules, init_module() calls them before mod->init().
The context is in both cases that of the calling functions.

Note that any function can be made a constructor function by tagging it
with __attribute__((constructor)), though I'm not sure if that has any
feasible use.

>> +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.

As Michael already pointed out that's already being taken care of by the
#define pr_fmt.

>
>> + }
>> + /*
>> + * 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.

I know, I know.. and I will try to come up with another patch that
consolidates all those within() implementations - but seeing that this
might take more discussions to get right, I'd rather only start with
that after making sure that the gcov code is finalized.

>
>> ...
>>
>> +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?

Right - the sscanf would make sense if kernel parameters could contain
spaces (in that case it catches <number><blanks><garbage> input) which
it can't so strtoul() would indeed make more sense. I'll prepare an
updated patch and send it out later today.

>> +/*
>> + * 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?

Yes :) The expensiveness is more than made up for by the reduced code
complexity of the iterator interface. Also the extra effort is only
spent when actually reading coverage files, i.e. not while performing a
test.

>> ...
>>
>> +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?

Hm, good question. Having a look at my test system, I see coverage data
files of up to 60kb size. With counters making up the largest part of
those, I'd guess the allocation size can be around ~55kb. I assume that
makes it a candidate for vmalloc?

>> + 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.

Thanks :)