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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

On Mon, 2013-06-17 at 09:58 +0100, Jan Beulich wrote:
> >>> 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.
> 

I don't know why gcc changed from .ctors to .init_array, I found some
reference for some architectural coherency (it seems other architectures
use init_array, for instance arm already used init_array, the structure
is the same).

I'm looking at binutils scripts:

...
  .init_array     :
  {
    PROVIDE_HIDDEN (__init_array_start = .);
    KEEP (*(SORT_BY_INIT_PRIORITY(.init_array.*) 
SORT_BY_INIT_PRIORITY(.ctors.*)))
    KEEP (*(.init_array))
    KEEP (*(EXCLUDE_FILE (*crtbegin.o *crtbegin?.o *crtend.o *crtend?.o ) 
.ctors))
    PROVIDE_HIDDEN (__init_array_end = .);
  }
...
  .ctors          :
  {
    /* gcc uses crtbegin.o to find the start of
       the constructors, so we make sure it is
       first.  Because this is a wildcard, it
       doesn't matter if the user does not
       actually link against crtbegin.o; the
       linker won't look for a file to match a
       wildcard.  The wildcard also means that it
       doesn't matter which directory crtbegin.o
       is in.  */
    KEEP (*crtbegin.o(.ctors))
    KEEP (*crtbegin?.o(.ctors))
    /* We don't want to include the .ctor section from
       the crtend.o file until after the sorted ctors.
       The .ctor section from the crtend file contains the
       end of ctors marker and it must be last */
    KEEP (*(EXCLUDE_FILE (*crtend.o *crtend?.o ) .ctors))
    KEEP (*(SORT(.ctors.*)))
    KEEP (*(.ctors))
  }
..

quite complicated. Can you read it ?

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

Do you prefer 4_7, perhaps is more readable.

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

Fixed, found some others.

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

Possibly is better to use XENCOV_COUNTERS_3_4 then. I used _MASK cause
3.4 used a strange mask field.

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

It's used in the macro some lines below.

> >  #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
> 

Yes, cause actually there is no C program that use these information
(perl is used)

Frediano



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