Re: [RFC PATCH 5/6] gcov: add gcov profiling infrastructure

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

On Wed, 07 May 2008 12:57:33 +0200 Peter Oberparleiter <peter.oberparleiter@de.ibm.com> wrote:

> >> +unsigned int gcov_version;
> >> +
> >> +void __gcov_init(struct gcov_info *info)
> >> +{
> >> +	mutex_lock(&gcov_lock);
> >> +	/* Check for compatible gcc version. */
> >> +	if (gcov_version == 0) {
> >> +		gcov_version = info->version;
> >> +		printk(KERN_INFO TAG "gcc version %x\n", gcov_version);
> > 
> > hm, what does the output from this look like?  "gcc version 42", which is
> > really gcc version 66 only we didn't tell the user that we printed it in
> > hex?
> > 
> > Might need a bit more thought here?
> 
> Quote from gcc/gcov-io.h:
> 
>    The version number
>    consists of the single character major version number, a two
>    character minor version number (leading zero for versions less than
>    10), and a single character indicating the status of the release.
>    [...]
>    For gcc 3.4 experimental, it would be '304e' (0x33303465).
> 
> This number really is meant for debugging purposes: when a user
> encounters problems, a copy of this line can tell exactly which version
> of gcc's gcov data structures were used during compilation.
> 
> Maybe a modified message text "gcov data version magic: %x" would be
> more descriptive.

"0x%x" would reduce confusion.

> >> +static inline int within(void *addr, void *start, unsigned long size)
> >> +{
> >> +        return (addr >= start && (void *) addr < start + size);
> >> +}
> > 
> > That is at least our fourth implementation of within(), and not all of them
> > have the same semantics.
> 
> I'll see if these can be merged. What would be the right place,
> linux/kernel.h?

Yeah.  Although you'd be forgiven for retaining the private implementation.
"[patch] consolidate all the within() implementations" is a separate work
and it'd be wrong to say this-is-a-prerequisite.

> >> +/* Profiling data types used for gcc 3.4 and above. */
> >> +
> >> +#define GCOV_COUNTERS		5
> >> +#define GCOV_DATA_MAGIC		((unsigned int) 0x67636461)
> >> +#define GCOV_TAG_FUNCTION	((unsigned int) 0x01000000)
> >> +#define GCOV_TAG_COUNTER_BASE	((unsigned int) 0x01a10000)
> >> +#define GCOV_TAG_FOR_COUNTER(count)					\
> >> +	(GCOV_TAG_COUNTER_BASE + ((unsigned int) (count) << 17))
> >> +
> >> +#if BITS_PER_LONG >= 64
> >> +typedef long gcov_type;
> >> +#else
> >> +typedef long long gcov_type;
> >> +#endif
> > 
> > Can we zap gcov_type completely and use u64?
> 
> gcc defines these types so I don't think there's another way to stay
> compatible than to copy their definitions.

hm, sad.  u64 is long on some 64-bit architectures and long long on others.

> >> +
> >> +	list_for_each_entry(node, &parent->children, list) {
> >> +		if (strcmp(node->name, name) == 0)
> >> +			return node;
> >> +	}
> >> +
> >> +	return NULL;
> >> +}
> > 
> > I trust this won't be called very frequently.
> 
> get_child_by_name() will be called multiple times for each gcov data
> structure when it is initialized. I don't see a problem with that
> though..

OK.  It just looks rather slow...

> Anyway, thanks for the extensive comments. I'll get back with a modified
> version of this patch set.

np.

The increase in kenrel size is pretty shocking.  From a sample of 1
(mm/swap.o) it really does increase text and data by about a factor of
three.  I assume that this means that distributors will be unable to enable
this feature, which makes it a kernel-developers-only thing.  Which is OK,
I guess.  But a shame.