Re: [Xen-devel] [PATCH] gcov: Support gcc 4.7

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.06.13 at 10:29, Frediano Ziglio <frediano.ziglio@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -112,6 +112,7 @@ SECTIONS
>         . = ALIGN(8);
>         __ctors_start = .;
>         *(.ctors)
> +       *(.init_array)
>         __ctors_end = .;
>    } :text
>    . = ALIGN(32);

First of all - again an x86 change without a matching ARM one?

And then this is different from how binutils deals with these in a
number of aspects:
- .init_array.* not handled (raises the question why .ctors.* didn't
  get handled already)
- .init_array following .ctors here, while binutils has it the other
  way around
- final section being .init_array instead of .ctors
- enclosing symbols prefixed with __init_array_ instead of __ctors_

Some or all of these may not matter, but I think any difference to
how binutils deals with these sections needs to be explained in the
patch description.

> +static inline const struct gcov_fn_info_407 *
> +next_func(const struct gcov_info_407 *info, int *n_func)

What are these "_407" suffixes intended to mean? They look rather
arbitrary, and to me aren't connected to the gcc version talked
about here.

> +{
> +    while ( ++*n_func < info->n_functions ) {

Coding style.

> +        const struct gcov_fn_info_407 *fn = info->functions[*n_func];
> +
> +        /* the test for info member handle common data redefinitions
> +           in object files */

Again. Stopping here; there are more.

> --- a/xen/include/public/gcov.h
> +++ b/xen/include/public/gcov.h
> @@ -28,10 +28,12 @@
>  #ifndef __XEN_PUBLIC_GCOV_H__
>  #define __XEN_PUBLIC_GCOV_H__ __XEN_PUBLIC_GCOV_H__
>  
> -#define XENCOV_COUNTERS         5
> +#define XENCOV_COUNTERS_MASK    5

Misnamed manifest constant? It's never being used as a mask and
also doesn't look like one.

> +#define XENCOV_COUNTERS         8

And this one doesn't appear to get used anywhere, so why are
these changes being done in the first place?

>  #define XENCOV_TAG_BASE         0x58544300u
>  #define XENCOV_TAG_FILE         (XENCOV_TAG_BASE+0x46u)
>  #define XENCOV_TAG_FUNC         (XENCOV_TAG_BASE+0x66u)
> +#define XENCOV_TAG_FUNC2        (XENCOV_TAG_BASE+0x67u)
>  #define XENCOV_TAG_COUNTER(n)   (XENCOV_TAG_BASE+0x30u+((n)&0xfu))
>  #define XENCOV_TAG_END          (XENCOV_TAG_BASE+0x2eu)
>  #define XENCOV_IS_TAG_COUNTER(n) \
> @@ -93,6 +95,18 @@ struct xencov_function
>  };
>  
>  /**
> + * Information for each function
> + * Number of counter is equal to the number of counter structures got before
> + */
> +struct xencov_function2
> +{
> +    uint32_t ident;
> +    uint32_t lineno_checksum;
> +    uint32_t cfg_checksum;
> +    uint32_t num_counters[1];
> +};

Nor can I seem to be able to spot a use of this structure.

Jan


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