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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.16 at 11:40, <wei.liu2@xxxxxxxxxx> wrote:
> +struct type_info {
> +    int ctr_type;

Can this be negative?

> +    unsigned int offset;
> +};
> +
> +/**
> + * struct gcov_iterator - specifies current file position in logical records
> + * @info: associated profiling data
> + * @record: record type
> + * @function: function number
> + * @type: counter type
> + * @count: index into values array
> + * @num_types: number of counter types
> + * @type_info: helper array to get values-array offset for current function
> + */
> +struct gcov_iterator {
> +    const struct gcov_info *info;
> +
> +    int record;

This one indeed looks like it can be.

> +    unsigned int function;
> +    unsigned int type;
> +    unsigned int count;
> +
> +    int num_types;

But judging by its name, this surely can't be.

> +    struct type_info type_info[GCOV_COUNTERS];
> +};
> +
> +/* Mapping of logical record number to actual file content. */
> +#define RECORD_FILE_MAGIC       0
> +#define RECORD_GCOV_VERSION     1
> +#define RECORD_TIME_STAMP       2
> +#define RECORD_FUNCTION_TAG     3
> +#define RECORD_FUNCTON_TAG_LEN  4
> +#define RECORD_FUNCTION_IDENT   5
> +#define RECORD_FUNCTION_CHECK   6
> +#define RECORD_COUNT_TAG        7
> +#define RECORD_COUNT_LEN        8
> +#define RECORD_COUNT            9
> +
> +static int counter_active(const struct gcov_info *info, unsigned int type)
> +{
> +    return (1 << type) & info->ctr_mask;

If the function return type is really meant to be a bitmask, then it
should be unsigned int (and 1u). However, I suspect this really
wants to be bool.

> +static size_t get_fn_size(const struct gcov_info *info)
> +{
> +    size_t size;
> +
> +    size = sizeof(struct gcov_fn_info) + num_counter_active(info) *
> +        sizeof(unsigned int);

Any reason this is not the variable's initializer? Also please use
sizeof(*info) and alignof(*info) (below here).

> +static struct gcov_fn_info *get_fn_info(const struct gcov_info *info,
> +                                        unsigned int fn)
> +{
> +    return (struct gcov_fn_info *)
> +        ((char *) info->functions + fn * get_fn_size(info));

Stray blank after (char *) and sub-optimal indentation.

> +static int gcov_iter_next(struct gcov_iterator *iter)
> +{
> +    switch ( iter->record )
> +    {
> +    case RECORD_FILE_MAGIC:
> +    case RECORD_GCOV_VERSION:
> +    case RECORD_FUNCTION_TAG:
> +    case RECORD_FUNCTON_TAG_LEN:
> +    case RECORD_FUNCTION_IDENT:
> +    case RECORD_COUNT_TAG:
> +        /* Advance to next record */
> +        iter->record++;
> +        break;
> +    case RECORD_COUNT:
> +        /* Advance to next count */
> +        iter->count++;
> +        /* fall through */
> +    case RECORD_COUNT_LEN:
> +        if ( iter->count < get_func(iter)->n_ctrs[iter->type] )
> +        {
> +            iter->record = 9;

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.

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

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

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

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

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

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?

Jan

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