Re: [Xen-devel] [PATCH v2 5/9] gcov: add new interface and 3.4 and 4.7 format support

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Oct 10, 2016 at 05:56:35AM -0600, Jan Beulich wrote:
[...]
> Who is 9 (more similar ones below)?
> 
> > +static int counter_active(const struct gcov_info *info, unsigned int type)
> > +{
> > +    return info->merge[type] ? 1 : 0;
> 
> Return type bool and preferably with !! instead of conditional
> expression.
> 

Andrew beat me to it: the code and data structures above are mostly
imported from Linux.

> > +size_t gcov_store_u32(void *buffer, size_t off, u32 v)
> > +{
> > +    u32 *data;
> 
> Please be consistent wrt u32 vs ...
> 
> > +static int gcov_info_dump_payload(const struct gcov_info *info,
> > +                                  XEN_GUEST_HANDLE_PARAM(char) buffer,
> > +                                  uint32_t *off)
> > +{
> > +    char *buf;
> > +    uint32_t buf_size;
> 
> ... uint32_t (and alike; I'd suggest using only the latter, and I think
> we should get rid of u32 / __u32 and their siblings eventually).
> 

Right. I will switch to using uint32_t.

> > +static uint32_t gcov_get_size(void)
> > +{
> > +    uint32_t total_size;
> > +    struct gcov_info *info = NULL;
> > +
> > +    /* Magic number XCOV */
> > +    total_size = sizeof(uint32_t);
> 
> Again - can't this be the initializer?
> 

Sure. I will move it up.

> > +int sysctl_gcov_op(xen_sysctl_gcov_op_t *op)
> > +{
> > +    int ret;
> > +
> > +    switch ( op->cmd )
> > +    {
> > +    case XEN_SYSCTL_GCOV_get_size:
> > +        op->size = gcov_get_size();
> > +        ret = 0;
> > +        break;
> > +    case XEN_SYSCTL_GCOV_read:
> 
> Blank lines between non-fall-through case statements please.
> 

OK.

> > --- /dev/null
> > +++ b/xen/common/gcov/gcov.h
> > @@ -0,0 +1,36 @@
> > +#ifndef _GCOV_H_
> > +#define _GCOV_H_
> > +
> > +#include <xen/guest_access.h>
> > +#include <xen/types.h>
> > +
> > +/*
> > + * Profiling data types used for gcc 3.4 and above - these are defined by
> > + * gcc and need to be kept as close to the original definition as possible 
> > to
> > + * remain compatible.
> > + */
> > +#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)                                \
> > +   _TAG_COUNTER_BASE + ((unsigned int) (count) << 17))
> 
> Stray blanks after casts.
> 

Will fix.

> > --- /dev/null
> > +++ b/xen/common/gcov/gcov_base.c
> > @@ -0,0 +1,64 @@
> > +/*
> > + * Common code across gcov implementations
> > + *
> > + * Copyright Citrix Systems R&D UK
> > + * Author(s): Wei Liu <wei.liu2@xxxxxxxxxx>
> > + *
> > + *    Uses gcc-internal data definitions.
> > + *    Based on the gcov-kernel patch by:
> > + *       Hubertus Franke <frankeh@xxxxxxxxxx>
> > + *       Nigel Hinds <nhinds@xxxxxxxxxx>
> > + *       Rajan Ravindran <rajancr@xxxxxxxxxx>
> > + *       Peter Oberparleiter <oberpar@xxxxxxxxxxxxxxxxxx>
> > + *       Paul Larson
> > + */
> > +
> > +#include "gcov.h"
> > +
> > +/*
> > + * __gcov_init is called by gcc-generated constructor code for each object
> > + * file compiled with -fprofile-arcs.
> > + *
> > + * Although this function is called only during initialization is called 
> > from
> > + * a .text section which is still present after initialization so not 
> > declare
> > + * as __init.
> 
> I don't follow - we do such references elsewhere, so I can't see a
> reason not to follow that model here too as long the call here
> happens before .init.* get discarded.
> 

This is residual from previous implementation. I haven't checked if the
statement is true or not. If this statement is not true I'm happy to
make corresponding adjustments.

> Having reached the end here - what if the user gets the Kconfig setting
> wrong? Wouldn't it be helpful if the gcov_<major>_<minor>.c files
> #error-ed upon an out of range gcc version?
> 

That's a good idea.

Wei.

> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel