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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.06.13 at 12:18, Frediano Ziglio <frediano.ziglio@xxxxxxxxxx> wrote:
> 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 ?

It's not _that_ difficult, but yes, this is what I looked at when
putting together the earlier response.

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

Unless 4.7 is the gcov version number, please call it gcc_4_7 or
gcc4_7, so that it's clear the version number of what you're
referring to.

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

Same here - if this is a gcc version, include gcc in the name.

>> > + * 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.
> 
> Yes, cause actually there is no C program that use these information
> (perl is used)

Okay, so it's here then just for documentation purposes. That should
be said in the preceding comment, or else you risk it getting removed
by some cleanup patch. Even better would be to end the comment
only after the structure declaration, to avoid cluttering the name
space.

Jan

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