[PATCH] coverage: fix on ARM

[PATCH] coverage: fix on ARM

[Ian Campbell]

Use a list of pointers to simplify the handling of 32- vs 64-bit.

Also on ARM the section name is ".init_array" and not ".ctors".

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Frediano Ziglio <frediano.ziglio@citrix.com>
Cc: keir@xen.org
---
This applies independently of my 64-bit ARM series.
---
 xen/arch/arm/xen.lds.S |   10 ++++------
 xen/arch/x86/xen.lds.S |    6 ++----
 xen/common/lib.c       |   13 +++++--------
 3 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 9043994..fd755d7 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -91,12 +91,10 @@ SECTIONS
        *(.init.data.rel)
        *(.init.data.rel.*)
 
-       . = ALIGN(4);
-       __CTOR_LIST__ = .;
-       LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
-       *(.ctors)
-       LONG(0)
-       __CTOR_END__ = .;
+       . = ALIGN(8);
+       __ctors_start = .;
+       *(.init_array)
+       __ctors_end = .;
   } :text
   . = ALIGN(32);
   .init.setup : {
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 5570389..d959941 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -110,11 +110,9 @@ SECTIONS
        __trampoline_seg_stop = .;
 
        . = ALIGN(8);
-       __CTOR_LIST__ = .;
-       QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
+       __ctors_start = .;
        *(.ctors)
-       QUAD(0)
-       __CTOR_END__ = .;
+       __ctors_end = .;
   } :text
   . = ALIGN(32);
   .init.setup : {
diff --git a/xen/common/lib.c b/xen/common/lib.c
index e0c65cf..6b80601 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -479,17 +479,14 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
     return ret;
 }
 
-extern const struct
-{
-    unsigned long count;
-    void (*funcs[1])(void);
-} __CTOR_LIST__;
+typedef void (*ctor_func_t)(void);
+extern const ctor_func_t __ctors_start[], __ctors_end[];
 
 void __init init_constructors(void)
 {
-    unsigned long n;
-    for ( n = 0; n < __CTOR_LIST__.count; ++n )
-        __CTOR_LIST__.funcs[n]();
+    const ctor_func_t *f;
+    for (f = __ctors_start; f < __ctors_end; ++f )
+        (*f)();
 }
 
 /*
-- 
1.7.2.5

Re: [PATCH] coverage: fix on ARM

[Jan Beulich]

>>> On 22.02.13 at 11:57, Ian Campbell <ian.campbell@citrix.com> wrote:
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -91,12 +91,10 @@ SECTIONS
>         *(.init.data.rel)
>         *(.init.data.rel.*)
>  
> -       . = ALIGN(4);
> -       __CTOR_LIST__ = .;
> -       LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
> -       *(.ctors)
> -       LONG(0)
> -       __CTOR_END__ = .;
> +       . = ALIGN(8);
> +       __ctors_start = .;
> +       *(.init_array)

Is this renaming from .ctors to .init_array really intended (i.e. was
using .ctors here wrong)?

Jan

Re: [PATCH] coverage: fix on ARM

[Ian Campbell]

On Fri, 2013-02-22 at 11:16 +0000, Jan Beulich wrote:
> >>> On 22.02.13 at 11:57, Ian Campbell <ian.campbell@citrix.com> wrote:
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -91,12 +91,10 @@ SECTIONS
> >         *(.init.data.rel)
> >         *(.init.data.rel.*)
> >  
> > -       . = ALIGN(4);
> > -       __CTOR_LIST__ = .;
> > -       LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
> > -       *(.ctors)
> > -       LONG(0)
> > -       __CTOR_END__ = .;
> > +       . = ALIGN(8);
> > +       __ctors_start = .;
> > +       *(.init_array)
> 
> Is this renaming from .ctors to .init_array really intended (i.e. was
> using .ctors here wrong)?

Yes, I mentioned it in the commit message, it is called .init_array on
ARM for whatever reason. Probably either historical accident or
compatibility eg. with ARM's compilers.

Ian.

Re: [PATCH] coverage: fix on ARM

[Jan Beulich]

>>> On 22.02.13 at 12:24, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2013-02-22 at 11:16 +0000, Jan Beulich wrote:
>> >>> On 22.02.13 at 11:57, Ian Campbell <ian.campbell@citrix.com> wrote:
>> > --- a/xen/arch/arm/xen.lds.S
>> > +++ b/xen/arch/arm/xen.lds.S
>> > @@ -91,12 +91,10 @@ SECTIONS
>> >         *(.init.data.rel)
>> >         *(.init.data.rel.*)
>> >  
>> > -       . = ALIGN(4);
>> > -       __CTOR_LIST__ = .;
>> > -       LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
>> > -       *(.ctors)
>> > -       LONG(0)
>> > -       __CTOR_END__ = .;
>> > +       . = ALIGN(8);
>> > +       __ctors_start = .;
>> > +       *(.init_array)
>> 
>> Is this renaming from .ctors to .init_array really intended (i.e. was
>> using .ctors here wrong)?
> 
> Yes, I mentioned it in the commit message,

Oops, sorry, managed to skip that line somehow.

> it is called .init_array on
> ARM for whatever reason. Probably either historical accident or
> compatibility eg. with ARM's compilers.

Or rather the other way around, because .init_array is a (newer)
ELF concept, whereas .ctors isn't.

Jan

Re: [PATCH] coverage: fix on ARM

[Ian Campbell]

On Fri, 2013-02-22 at 11:41 +0000, Jan Beulich wrote:
> >>> On 22.02.13 at 12:24, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Fri, 2013-02-22 at 11:16 +0000, Jan Beulich wrote:
> >> >>> On 22.02.13 at 11:57, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> > --- a/xen/arch/arm/xen.lds.S
> >> > +++ b/xen/arch/arm/xen.lds.S
> >> > @@ -91,12 +91,10 @@ SECTIONS
> >> >         *(.init.data.rel)
> >> >         *(.init.data.rel.*)
> >> >  
> >> > -       . = ALIGN(4);
> >> > -       __CTOR_LIST__ = .;
> >> > -       LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
> >> > -       *(.ctors)
> >> > -       LONG(0)
> >> > -       __CTOR_END__ = .;
> >> > +       . = ALIGN(8);
> >> > +       __ctors_start = .;
> >> > +       *(.init_array)
> >> 
> >> Is this renaming from .ctors to .init_array really intended (i.e. was
> >> using .ctors here wrong)?
> > 
> > Yes, I mentioned it in the commit message,
> 
> Oops, sorry, managed to skip that line somehow.
> 
> > it is called .init_array on
> > ARM for whatever reason. Probably either historical accident or
> > compatibility eg. with ARM's compilers.
> 
> Or rather the other way around, because .init_array is a (newer)
> ELF concept, whereas .ctors isn't.

Ah, that explains it, thanks!

Ian.

Re: [PATCH] coverage: fix on ARM

[Ian Campbell]

On Fri, 2013-02-22 at 10:57 +0000, Ian Campbell wrote:
> Use a list of pointers to simplify the handling of 32- vs 64-bit.
> 
> Also on ARM the section name is ".init_array" and not ".ctors".

Any comments/ack/nacks?

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Frediano Ziglio <frediano.ziglio@citrix.com>
> Cc: keir@xen.org
> ---
> This applies independently of my 64-bit ARM series.
> ---
>  xen/arch/arm/xen.lds.S |   10 ++++------
>  xen/arch/x86/xen.lds.S |    6 ++----
>  xen/common/lib.c       |   13 +++++--------
>  3 files changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index 9043994..fd755d7 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -91,12 +91,10 @@ SECTIONS
>         *(.init.data.rel)
>         *(.init.data.rel.*)
>  
> -       . = ALIGN(4);
> -       __CTOR_LIST__ = .;
> -       LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
> -       *(.ctors)
> -       LONG(0)
> -       __CTOR_END__ = .;
> +       . = ALIGN(8);
> +       __ctors_start = .;
> +       *(.init_array)
> +       __ctors_end = .;
>    } :text
>    . = ALIGN(32);
>    .init.setup : {
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 5570389..d959941 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -110,11 +110,9 @@ SECTIONS
>         __trampoline_seg_stop = .;
>  
>         . = ALIGN(8);
> -       __CTOR_LIST__ = .;
> -       QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
> +       __ctors_start = .;
>         *(.ctors)
> -       QUAD(0)
> -       __CTOR_END__ = .;
> +       __ctors_end = .;
>    } :text
>    . = ALIGN(32);
>    .init.setup : {
> diff --git a/xen/common/lib.c b/xen/common/lib.c
> index e0c65cf..6b80601 100644
> --- a/xen/common/lib.c
> +++ b/xen/common/lib.c
> @@ -479,17 +479,14 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
>      return ret;
>  }
>  
> -extern const struct
> -{
> -    unsigned long count;
> -    void (*funcs[1])(void);
> -} __CTOR_LIST__;
> +typedef void (*ctor_func_t)(void);
> +extern const ctor_func_t __ctors_start[], __ctors_end[];
>  
>  void __init init_constructors(void)
>  {
> -    unsigned long n;
> -    for ( n = 0; n < __CTOR_LIST__.count; ++n )
> -        __CTOR_LIST__.funcs[n]();
> +    const ctor_func_t *f;
> +    for (f = __ctors_start; f < __ctors_end; ++f )
> +        (*f)();
>  }
>  
>  /*

Re: [PATCH] coverage: fix on ARM

[Keir Fraser]

On 27/02/2013 13:17, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:

> On Fri, 2013-02-22 at 10:57 +0000, Ian Campbell wrote:
>> Use a list of pointers to simplify the handling of 32- vs 64-bit.
>> 
>> Also on ARM the section name is ".init_array" and not ".ctors".
> 
> Any comments/ack/nacks?

If it works, it's good by me ;)

 -- Keir

>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Frediano Ziglio <frediano.ziglio@citrix.com>
>> Cc: keir@xen.org
>> ---
>> This applies independently of my 64-bit ARM series.
>> ---
>>  xen/arch/arm/xen.lds.S |   10 ++++------
>>  xen/arch/x86/xen.lds.S |    6 ++----
>>  xen/common/lib.c       |   13 +++++--------
>>  3 files changed, 11 insertions(+), 18 deletions(-)
>> 
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index 9043994..fd755d7 100644
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -91,12 +91,10 @@ SECTIONS
>>         *(.init.data.rel)
>>         *(.init.data.rel.*)
>>  
>> -       . = ALIGN(4);
>> -       __CTOR_LIST__ = .;
>> -       LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
>> -       *(.ctors)
>> -       LONG(0)
>> -       __CTOR_END__ = .;
>> +       . = ALIGN(8);
>> +       __ctors_start = .;
>> +       *(.init_array)
>> +       __ctors_end = .;
>>    } :text
>>    . = ALIGN(32);
>>    .init.setup : {
>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>> index 5570389..d959941 100644
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -110,11 +110,9 @@ SECTIONS
>>         __trampoline_seg_stop = .;
>>  
>>         . = ALIGN(8);
>> -       __CTOR_LIST__ = .;
>> -       QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
>> +       __ctors_start = .;
>>         *(.ctors)
>> -       QUAD(0)
>> -       __CTOR_END__ = .;
>> +       __ctors_end = .;
>>    } :text
>>    . = ALIGN(32);
>>    .init.setup : {
>> diff --git a/xen/common/lib.c b/xen/common/lib.c
>> index e0c65cf..6b80601 100644
>> --- a/xen/common/lib.c
>> +++ b/xen/common/lib.c
>> @@ -479,17 +479,14 @@ unsigned long long parse_size_and_unit(const char *s,
>> const char **ps)
>>      return ret;
>>  }
>>  
>> -extern const struct
>> -{
>> -    unsigned long count;
>> -    void (*funcs[1])(void);
>> -} __CTOR_LIST__;
>> +typedef void (*ctor_func_t)(void);
>> +extern const ctor_func_t __ctors_start[], __ctors_end[];
>>  
>>  void __init init_constructors(void)
>>  {
>> -    unsigned long n;
>> -    for ( n = 0; n < __CTOR_LIST__.count; ++n )
>> -        __CTOR_LIST__.funcs[n]();
>> +    const ctor_func_t *f;
>> +    for (f = __ctors_start; f < __ctors_end; ++f )
>> +        (*f)();
>>  }
>>  
>>  /*
> 
> 

Re: [PATCH] coverage: fix on ARM

[Frediano Ziglio]

On Wed, 2013-02-27 at 13:17 +0000, Ian Campbell wrote:
> On Fri, 2013-02-22 at 10:57 +0000, Ian Campbell wrote:
> > Use a list of pointers to simplify the handling of 32- vs 64-bit.
> > 
> > Also on ARM the section name is ".init_array" and not ".ctors".
> 
> Any comments/ack/nacks?
> 

Looks fine to me. Did you test it? Actually the way I test is:
- compile with coverage
- run xencov to extract information (more or less for x64 is 500k)
- check file content to see if counters increase while taking multiple
snapshot (xencov read - | hexdump -C | tail -100, counters are after
0CTX string which means "Xen Test Coverage count 0")
- reset counters (but this has nothing to do with constructors)

I actually cannot test for Arm.

Oh.. there is a missing space after a "for("

Frediano

> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Frediano Ziglio <frediano.ziglio@citrix.com>
> > Cc: keir@xen.org
> > ---
> > This applies independently of my 64-bit ARM series.
> > ---
> >  xen/arch/arm/xen.lds.S |   10 ++++------
> >  xen/arch/x86/xen.lds.S |    6 ++----
> >  xen/common/lib.c       |   13 +++++--------
> >  3 files changed, 11 insertions(+), 18 deletions(-)
> > 
> > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > index 9043994..fd755d7 100644
> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -91,12 +91,10 @@ SECTIONS
> >         *(.init.data.rel)
> >         *(.init.data.rel.*)
> >  
> > -       . = ALIGN(4);
> > -       __CTOR_LIST__ = .;
> > -       LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
> > -       *(.ctors)
> > -       LONG(0)
> > -       __CTOR_END__ = .;
> > +       . = ALIGN(8);
> > +       __ctors_start = .;
> > +       *(.init_array)
> > +       __ctors_end = .;
> >    } :text
> >    . = ALIGN(32);
> >    .init.setup : {
> > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > index 5570389..d959941 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -110,11 +110,9 @@ SECTIONS
> >         __trampoline_seg_stop = .;
> >  
> >         . = ALIGN(8);
> > -       __CTOR_LIST__ = .;
> > -       QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
> > +       __ctors_start = .;
> >         *(.ctors)
> > -       QUAD(0)
> > -       __CTOR_END__ = .;
> > +       __ctors_end = .;
> >    } :text
> >    . = ALIGN(32);
> >    .init.setup : {
> > diff --git a/xen/common/lib.c b/xen/common/lib.c
> > index e0c65cf..6b80601 100644
> > --- a/xen/common/lib.c
> > +++ b/xen/common/lib.c
> > @@ -479,17 +479,14 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
> >      return ret;
> >  }
> >  
> > -extern const struct
> > -{
> > -    unsigned long count;
> > -    void (*funcs[1])(void);
> > -} __CTOR_LIST__;
> > +typedef void (*ctor_func_t)(void);
> > +extern const ctor_func_t __ctors_start[], __ctors_end[];
> >  
> >  void __init init_constructors(void)
> >  {
> > -    unsigned long n;
> > -    for ( n = 0; n < __CTOR_LIST__.count; ++n )
> > -        __CTOR_LIST__.funcs[n]();
> > +    const ctor_func_t *f;
> > +    for (f = __ctors_start; f < __ctors_end; ++f )
> > +        (*f)();
> >  }
> >  
> >  /*
> 
> 

Re: [PATCH] coverage: fix on ARM

[Ian Campbell]

On Thu, 2013-02-28 at 10:00 +0000, Frediano Ziglio wrote:
> On Wed, 2013-02-27 at 13:17 +0000, Ian Campbell wrote:
> > On Fri, 2013-02-22 at 10:57 +0000, Ian Campbell wrote:
> > > Use a list of pointers to simplify the handling of 32- vs 64-bit.
> > > 
> > > Also on ARM the section name is ".init_array" and not ".ctors".
> > 
> > Any comments/ack/nacks?
> > 
> 
> Looks fine to me. Did you test it?

I tested on x86_64 and arm32/64 by annotating the init function to print
the function names as it called them and observing that a) a bunch of
functions were called (on x86 it looked similar before and after this
change) and b) it didn't crash.

>  Actually the way I test is:
> - compile with coverage
> - run xencov to extract information (more or less for x64 is 500k)
> - check file content to see if counters increase while taking multiple
> snapshot (xencov read - | hexdump -C | tail -100, counters are after
> 0CTX string which means "Xen Test Coverage count 0")
> - reset counters (but this has nothing to do with constructors)
> 
> I actually cannot test for Arm.
> 
> Oh.. there is a missing space after a "for("

So there is, thanks for spotting. A resend will have to wait until after
my flight to Hong Kong or else whoever applies, which may end up being
me, can fix it as they go.

Cheers,
Ian.

> 
> Frediano
> 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Frediano Ziglio <frediano.ziglio@citrix.com>
> > > Cc: keir@xen.org
> > > ---
> > > This applies independently of my 64-bit ARM series.
> > > ---
> > >  xen/arch/arm/xen.lds.S |   10 ++++------
> > >  xen/arch/x86/xen.lds.S |    6 ++----
> > >  xen/common/lib.c       |   13 +++++--------
> > >  3 files changed, 11 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> > > index 9043994..fd755d7 100644
> > > --- a/xen/arch/arm/xen.lds.S
> > > +++ b/xen/arch/arm/xen.lds.S
> > > @@ -91,12 +91,10 @@ SECTIONS
> > >         *(.init.data.rel)
> > >         *(.init.data.rel.*)
> > >  
> > > -       . = ALIGN(4);
> > > -       __CTOR_LIST__ = .;
> > > -       LONG((__CTOR_END__ - __CTOR_LIST__) / 4 - 2)
> > > -       *(.ctors)
> > > -       LONG(0)
> > > -       __CTOR_END__ = .;
> > > +       . = ALIGN(8);
> > > +       __ctors_start = .;
> > > +       *(.init_array)
> > > +       __ctors_end = .;
> > >    } :text
> > >    . = ALIGN(32);
> > >    .init.setup : {
> > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > index 5570389..d959941 100644
> > > --- a/xen/arch/x86/xen.lds.S
> > > +++ b/xen/arch/x86/xen.lds.S
> > > @@ -110,11 +110,9 @@ SECTIONS
> > >         __trampoline_seg_stop = .;
> > >  
> > >         . = ALIGN(8);
> > > -       __CTOR_LIST__ = .;
> > > -       QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
> > > +       __ctors_start = .;
> > >         *(.ctors)
> > > -       QUAD(0)
> > > -       __CTOR_END__ = .;
> > > +       __ctors_end = .;
> > >    } :text
> > >    . = ALIGN(32);
> > >    .init.setup : {
> > > diff --git a/xen/common/lib.c b/xen/common/lib.c
> > > index e0c65cf..6b80601 100644
> > > --- a/xen/common/lib.c
> > > +++ b/xen/common/lib.c
> > > @@ -479,17 +479,14 @@ unsigned long long parse_size_and_unit(const char *s, const char **ps)
> > >      return ret;
> > >  }
> > >  
> > > -extern const struct
> > > -{
> > > -    unsigned long count;
> > > -    void (*funcs[1])(void);
> > > -} __CTOR_LIST__;
> > > +typedef void (*ctor_func_t)(void);
> > > +extern const ctor_func_t __ctors_start[], __ctors_end[];
> > >  
> > >  void __init init_constructors(void)
> > >  {
> > > -    unsigned long n;
> > > -    for ( n = 0; n < __CTOR_LIST__.count; ++n )
> > > -        __CTOR_LIST__.funcs[n]();
> > > +    const ctor_func_t *f;
> > > +    for (f = __ctors_start; f < __ctors_end; ++f )
> > > +        (*f)();
> > >  }
> > >  
> > >  /*
> > 
> > 
>