[PATCH] gcov: Support gcc 4.7

[PATCH] gcov: Support gcc 4.7

[Frediano Ziglio]

gcc 4.7 changed format used internally for coverage data.
This patch address these changes.
The information that gcc generate are mostly the same but to support common
sections.
The only difference in the blob exported by Xen is that functions have 2
different checksums instead of one.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 tools/misc/xencov_split   |   26 ++++++--
 xen/arch/x86/xen.lds.S    |    1 +
 xen/common/gcov/gcov.c    |  161 +++++++++++++++++++++++++++++++++++++--------
 xen/include/public/gcov.h |   16 ++++-
 xen/include/xen/gcov.h    |   68 ++++++++++++++++++-
 5 files changed, 235 insertions(+), 37 deletions(-)

Patch tested with gcc 4.7, x64 with x86 dom0 and lcov 1.10.

diff --git a/tools/misc/xencov_split b/tools/misc/xencov_split
index 2e5aa80..af0f580 100755
--- a/tools/misc/xencov_split
+++ b/tools/misc/xencov_split
@@ -27,7 +27,8 @@ my $magic = 0x67636461;
 my $ctrBase = 0x01a10000;
 
 my $xenMagic = 0x58544346;	# file header
-my $xenTagFunc = 0x58544366;	# functions tag
+my $xenTagFunc  = 0x58544366;	# functions tag
+my $xenTagFunc2 = 0x58544367;	# functions tag2
 my $xenTagCount0 = 0x58544330;	# counter 0 tag
 my $xenTagEnd = 0x5854432e;	# end file
 
@@ -86,9 +87,9 @@ sub getS()
     return $res;
 }
 
-sub parseFunctions($)
+sub parseFunctions($$)
 {
-    my $numCounters = shift;
+    my ($numCounters, $ver) = @_;
     my $num = get32();
 
     my @funcs;
@@ -96,10 +97,12 @@ sub parseFunctions($)
         my @data;
         my $ident = get32();
         my $checksum = get32();
+        my $checksum2 = 0;
+        $checksum2 = get32() if $ver > 1;
         for my $n (1..$numCounters) {
             push @data, get32(); # number of counters for a type
         }
-        push @funcs, [$ident, $checksum, \@data];
+        push @funcs, [$ver, $ident, $checksum, $checksum2, \@data];
     }
     align();
     return @funcs;
@@ -147,7 +150,12 @@ sub parseFile()
         last if ($tag == $xenMagic || $tag == $xenTagEnd);
         if ($tag == $xenTagFunc) {
             die if scalar(@funcs);
-            @funcs = parseFunctions(scalar(@ctrs));
+            @funcs = parseFunctions(scalar(@ctrs), 1);
+            next;
+        }
+        if ($tag == $xenTagFunc2) {
+            die if scalar(@funcs);
+            @funcs = parseFunctions(scalar(@ctrs), 2);
             next;
         }
 
@@ -159,10 +167,14 @@ sub parseFile()
     # print all functions
     for my $f (@funcs) {
         # tag tag_len ident checksum
-        print OUT pack('VVVV', 0x01000000, 2, $f->[0], $f->[1]);
+        if ($f->[0] == 1) {
+            print OUT pack('VVVV',  0x01000000, 2, $f->[1], $f->[2]);
+        } else {
+            print OUT pack('VVVVV', 0x01000000, 3, $f->[1], $f->[2], $f->[3]);
+        }
         # all counts
         my $n = 0;
-        for my $c (@{$f->[2]}) {
+        for my $c (@{$f->[4]}) {
             my ($type, $data) = @{$ctrs[$n]};
             print OUT pack('VV', $ctrBase + 0x20000 * $type, $c*2);
             die "--$c--$type--$data--" if length($data) < $c * 8;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d959941..bdc4c91 100644
--- 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);
diff --git a/xen/common/gcov/gcov.c b/xen/common/gcov/gcov.c
index b5717b9..1d32d8f 100644
--- a/xen/common/gcov/gcov.c
+++ b/xen/common/gcov/gcov.c
@@ -106,17 +106,120 @@ static int write_string(write_iter_t *iter, const char *s)
 
 static inline int next_type(const struct gcov_info *info, int *type)
 {
-    while ( ++*type < XENCOV_COUNTERS && !counter_active(info, *type) )
+    while ( ++*type < XENCOV_COUNTERS_MASK && !counter_active(info, *type) )
         continue;
     return *type;
 }
 
+static inline const struct gcov_fn_info_407 *
+next_func(const struct gcov_info_407 *info, int *n_func)
+{
+    while ( ++*n_func < info->n_functions ) {
+        const struct gcov_fn_info_407 *fn = info->functions[*n_func];
+
+        /* the test for info member handle common data redefinitions
+           in object files */
+        if ( fn && fn->info == info)
+             return fn;
+    }
+
+    return NULL;
+}
+
+static inline const struct gcov_ctr_info_407 *
+next_ctr(const struct gcov_fn_info_407 *fn, int *n_ctr)
+{
+    while ( ++*n_ctr < XENCOV_COUNTERS_407 )
+        if ( fn->info->merge[*n_ctr] )
+            return &fn->ctrs[*n_ctr];
+
+    return NULL;
+}
+
 static inline void align_iter(write_iter_t *iter)
 {
     iter->write_offset =
         (iter->write_offset + sizeof(uint64_t) - 1) & -sizeof(uint64_t);
 }
 
+static int write_info(write_iter_t *iter, const struct gcov_info* info)
+{
+    const struct gcov_ctr_info *ctr;
+    int type, ret;
+    size_t size_fn = sizeof(struct gcov_fn_info);
+
+    /* dump counters */
+    ctr = info->counts;
+    for ( type = -1; next_type(info, &type) < XENCOV_COUNTERS_MASK; ++ctr )
+    {
+        align_iter(iter);
+        chk(write32(iter, XENCOV_TAG_COUNTER(type)));
+        chk(write32(iter, ctr->num));
+        chk(write_raw(iter, ctr->values,
+                          ctr->num * sizeof(ctr->values[0])));
+
+        size_fn += sizeof(unsigned);
+    }
+
+    /* dump all functions together */
+    align_iter(iter);
+    chk(write32(iter, XENCOV_TAG_FUNC));
+    chk(write32(iter, info->n_functions));
+    chk(write_raw(iter, info->functions, info->n_functions * size_fn));
+
+    return 0;
+}
+
+static int write_info_407(write_iter_t *iter, const struct gcov_info_407* info)
+{
+    int ret;
+    const struct gcov_fn_info_407 *fn;
+    const struct gcov_ctr_info_407 *ctr;
+    int n_func, n_ctr;
+    unsigned int num_func = 0;
+    unsigned int ctrs[XENCOV_COUNTERS_407];
+
+    for ( n_ctr = 0; n_ctr < XENCOV_COUNTERS_407; ++n_ctr )
+        ctrs[n_ctr] = 0;
+
+    /* scan to total counters */
+    for ( n_func = -1; (fn = next_func(info, &n_func)) != NULL; )
+    {
+        ++num_func;
+        for ( n_ctr = -1; (ctr = next_ctr(fn, &n_ctr)) != NULL; )
+            ctrs[n_ctr] += ctr->num;
+    }
+
+    /* output counters */
+    for ( n_ctr = 0; n_ctr < XENCOV_COUNTERS_407; ++n_ctr )
+    {
+        if ( !ctrs[n_ctr] ) continue;
+        align_iter(iter);
+        chk(write32(iter, XENCOV_TAG_COUNTER(n_ctr)));
+        chk(write32(iter, ctrs[n_ctr]));
+        for ( n_func = -1; (fn = next_func(info, &n_func)) != NULL; )
+        {
+            ctr = &fn->ctrs[n_ctr];
+            chk(write_raw(iter, ctr->values,
+                          ctr->num * sizeof(ctr->values[0])));
+        }
+    }
+
+    /* dump all functions together */
+    align_iter(iter);
+    chk(write32(iter, XENCOV_TAG_FUNC2));
+    chk(write32(iter, num_func));
+    for ( n_func = -1; (fn = next_func(info, &n_func)) != NULL; )
+    {
+        chk(write32(iter, fn->ident));
+        chk(write32(iter, fn->lineno_checksum));
+        chk(write32(iter, fn->cfg_checksum));
+        for ( n_ctr = -1; (ctr = next_ctr(fn, &n_ctr)) != NULL; )
+            chk(write32(iter, ctr->num));
+    }
+    return 0;
+}
+
 static int write_gcov(write_iter_t *iter)
 {
     struct gcov_info *info;
@@ -128,10 +231,6 @@ static int write_gcov(write_iter_t *iter)
     /* dump all files */
     for ( info = info_list ; info; info = info->next )
     {
-        const struct gcov_ctr_info *ctr;
-        int type;
-        size_t size_fn = sizeof(struct gcov_fn_info);
-
         align_iter(iter);
         chk(write32(iter, XENCOV_TAG_FILE));
         chk(write32(iter, info->version));
@@ -139,23 +238,10 @@ static int write_gcov(write_iter_t *iter)
         chk(write_string(iter, info->filename));
 
         /* dump counters */
-        ctr = info->counts;
-        for ( type = -1; next_type(info, &type) < XENCOV_COUNTERS; ++ctr )
-        {
-            align_iter(iter);
-            chk(write32(iter, XENCOV_TAG_COUNTER(type)));
-            chk(write32(iter, ctr->num));
-            chk(write_raw(iter, ctr->values,
-                          ctr->num * sizeof(ctr->values[0])));
-
-            size_fn += sizeof(unsigned);
-        }
-
-        /* dump all functions together */
-        align_iter(iter);
-        chk(write32(iter, XENCOV_TAG_FUNC));
-        chk(write32(iter, info->n_functions));
-        chk(write_raw(iter, info->functions, info->n_functions * size_fn));
+        if (info->version < XENCOV_VERSION_407)
+            chk(write_info(iter, info));
+        else
+            chk(write_info_407(iter, (struct gcov_info_407 *) info));
     }
 
     /* stop tag */
@@ -164,19 +250,38 @@ static int write_gcov(write_iter_t *iter)
     return 0;
 }
 
+static void reset_info(struct gcov_info *info)
+{
+    const struct gcov_ctr_info *ctr;
+    int type;
+
+    ctr = info->counts;
+    for ( type = -1; next_type(info, &type) < XENCOV_COUNTERS_MASK; ++ctr )
+        memset(ctr->values, 0, ctr->num * sizeof(ctr->values[0]));
+}
+
+static void reset_info_407(struct gcov_info_407 *info)
+{
+    const struct gcov_fn_info_407 *fn;
+    const struct gcov_ctr_info_407 *ctr;
+    int n_func, n_ctr;
+
+    for ( n_func = -1; (fn = next_func(info, &n_func)) != NULL; )
+        for ( n_ctr = -1; (ctr = next_ctr(fn, &n_ctr)) != NULL; )
+            memset(ctr->values, 0, ctr->num * sizeof(ctr->values[0]));
+}
+
 static int reset_counters(void)
 {
     struct gcov_info *info;
 
     for ( info = info_list ; info; info = info->next )
     {
-        const struct gcov_ctr_info *ctr;
-        int type;
-
         /* reset counters */
-        ctr = info->counts;
-        for ( type = -1; next_type(info, &type) < XENCOV_COUNTERS; ++ctr )
-            memset(ctr->values, 0, ctr->num * sizeof(ctr->values[0]));
+        if ( info->version < XENCOV_VERSION_407 )
+            reset_info(info);
+        else
+            reset_info_407((struct gcov_info_407 *) info);
     }
 
     return 0;
diff --git a/xen/include/public/gcov.h b/xen/include/public/gcov.h
index 1b29b48..e7573fb 100644
--- 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
+#define XENCOV_COUNTERS         8
 #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];
+};
+
+/**
  * Information for all functions
  * Aligned to 8 bytes
  */
diff --git a/xen/include/xen/gcov.h b/xen/include/xen/gcov.h
index 27c5c37..f36388a 100644
--- a/xen/include/xen/gcov.h
+++ b/xen/include/xen/gcov.h
@@ -40,6 +40,8 @@ struct gcov_fn_info
     unsigned int n_ctrs[0];
 };
 
+typedef void (*gcov_merge_func)(gcov_type *, unsigned int);
+
 /**
  * struct gcov_ctr_info - profiling data per counter type
  * @num: number of counter values for this type
@@ -53,7 +55,7 @@ struct gcov_ctr_info
 {
     unsigned int num;
     gcov_type *values;
-    void (*merge)(gcov_type *, unsigned int);
+    gcov_merge_func merge;
 };
 
 /**
@@ -82,6 +84,70 @@ struct gcov_info
     struct gcov_ctr_info      counts[0];
 };
 
+struct gcov_info_407;
+
+/**
+ * struct gcov_ctr_info_407 - profiling data per counter type
+ * @num: number of counter values for this type
+ * @values: array of counter values for this type
+ * @merge: merge function for counter values of this type (unused)
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the values array.
+ */
+struct gcov_ctr_info_407
+{
+    unsigned int num;
+    gcov_type *values;
+};
+
+
+/**
+ * struct gcov_fn_info_407 - profiling meta data per function
+ * @ident: object file-unique function identifier
+ * @lineno_checksum: function lineno checksum
+ * @cfg_checksum: function cfg checksum
+ * @ctrs: counters for this function
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time.
+ */
+struct gcov_fn_info_407
+{
+    const struct gcov_info_407 *info;
+    unsigned int ident;
+    unsigned int lineno_checksum;
+    unsigned int cfg_checksum;
+    struct gcov_ctr_info_407 ctrs[0];
+};
+
+#define XENCOV_COUNTERS_407 8
+#define XENCOV_VERSION_407 0x34303700
+
+/**
+ * struct gcov_info_407 - profiling data per object file
+ * @version: gcov version magic indicating the gcc version used for compilation
+ * @next: list head for a singly-linked list
+ * @stamp: time stamp
+ * @filename: name of the associated gcov data file
+ * @merge: merge functions
+ * @n_functions: number of instrumented functions
+ * @functions: function data
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time with the exception of the next pointer.
+ */
+struct gcov_info_407
+{
+    unsigned int              version;
+    struct gcov_info          *next;
+    unsigned int              stamp;
+    const char                *filename;
+    gcov_merge_func           merge[XENCOV_COUNTERS_407];
+    unsigned int              n_functions;
+    const struct gcov_fn_info_407 * const *functions;
+};
+
 
 /**
  * Sysctl operations for coverage
-- 
1.7.10.4

Re: [PATCH] gcov: Support gcc 4.7

[Jan Beulich]

>>> On 17.06.13 at 10:29, Frediano Ziglio <frediano.ziglio@citrix.com> 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

Re: [PATCH] gcov: Support gcc 4.7

[George Dunlap]

On 17/06/13 09:29, Frediano Ziglio wrote:
> gcc 4.7 changed format used internally for coverage data.
> This patch address these changes.
> The information that gcc generate are mostly the same but to support common
> sections.
> The only difference in the blob exported by Xen is that functions have 2
> different checksums instead of one.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>

At this point we'd like to completely freeze the tree except for 
critical bugfixes.  I think this will have to wait for 4.4.

  -George

> ---
>   tools/misc/xencov_split   |   26 ++++++--
>   xen/arch/x86/xen.lds.S    |    1 +
>   xen/common/gcov/gcov.c    |  161 +++++++++++++++++++++++++++++++++++++--------
>   xen/include/public/gcov.h |   16 ++++-
>   xen/include/xen/gcov.h    |   68 ++++++++++++++++++-
>   5 files changed, 235 insertions(+), 37 deletions(-)
>
> Patch tested with gcc 4.7, x64 with x86 dom0 and lcov 1.10.
>
> diff --git a/tools/misc/xencov_split b/tools/misc/xencov_split
> index 2e5aa80..af0f580 100755
> --- a/tools/misc/xencov_split
> +++ b/tools/misc/xencov_split
> @@ -27,7 +27,8 @@ my $magic = 0x67636461;
>   my $ctrBase = 0x01a10000;
>   
>   my $xenMagic = 0x58544346;	# file header
> -my $xenTagFunc = 0x58544366;	# functions tag
> +my $xenTagFunc  = 0x58544366;	# functions tag
> +my $xenTagFunc2 = 0x58544367;	# functions tag2
>   my $xenTagCount0 = 0x58544330;	# counter 0 tag
>   my $xenTagEnd = 0x5854432e;	# end file
>   
> @@ -86,9 +87,9 @@ sub getS()
>       return $res;
>   }
>   
> -sub parseFunctions($)
> +sub parseFunctions($$)
>   {
> -    my $numCounters = shift;
> +    my ($numCounters, $ver) = @_;
>       my $num = get32();
>   
>       my @funcs;
> @@ -96,10 +97,12 @@ sub parseFunctions($)
>           my @data;
>           my $ident = get32();
>           my $checksum = get32();
> +        my $checksum2 = 0;
> +        $checksum2 = get32() if $ver > 1;
>           for my $n (1..$numCounters) {
>               push @data, get32(); # number of counters for a type
>           }
> -        push @funcs, [$ident, $checksum, \@data];
> +        push @funcs, [$ver, $ident, $checksum, $checksum2, \@data];
>       }
>       align();
>       return @funcs;
> @@ -147,7 +150,12 @@ sub parseFile()
>           last if ($tag == $xenMagic || $tag == $xenTagEnd);
>           if ($tag == $xenTagFunc) {
>               die if scalar(@funcs);
> -            @funcs = parseFunctions(scalar(@ctrs));
> +            @funcs = parseFunctions(scalar(@ctrs), 1);
> +            next;
> +        }
> +        if ($tag == $xenTagFunc2) {
> +            die if scalar(@funcs);
> +            @funcs = parseFunctions(scalar(@ctrs), 2);
>               next;
>           }
>   
> @@ -159,10 +167,14 @@ sub parseFile()
>       # print all functions
>       for my $f (@funcs) {
>           # tag tag_len ident checksum
> -        print OUT pack('VVVV', 0x01000000, 2, $f->[0], $f->[1]);
> +        if ($f->[0] == 1) {
> +            print OUT pack('VVVV',  0x01000000, 2, $f->[1], $f->[2]);
> +        } else {
> +            print OUT pack('VVVVV', 0x01000000, 3, $f->[1], $f->[2], $f->[3]);
> +        }
>           # all counts
>           my $n = 0;
> -        for my $c (@{$f->[2]}) {
> +        for my $c (@{$f->[4]}) {
>               my ($type, $data) = @{$ctrs[$n]};
>               print OUT pack('VV', $ctrBase + 0x20000 * $type, $c*2);
>               die "--$c--$type--$data--" if length($data) < $c * 8;
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index d959941..bdc4c91 100644
> --- 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);
> diff --git a/xen/common/gcov/gcov.c b/xen/common/gcov/gcov.c
> index b5717b9..1d32d8f 100644
> --- a/xen/common/gcov/gcov.c
> +++ b/xen/common/gcov/gcov.c
> @@ -106,17 +106,120 @@ static int write_string(write_iter_t *iter, const char *s)
>   
>   static inline int next_type(const struct gcov_info *info, int *type)
>   {
> -    while ( ++*type < XENCOV_COUNTERS && !counter_active(info, *type) )
> +    while ( ++*type < XENCOV_COUNTERS_MASK && !counter_active(info, *type) )
>           continue;
>       return *type;
>   }
>   
> +static inline const struct gcov_fn_info_407 *
> +next_func(const struct gcov_info_407 *info, int *n_func)
> +{
> +    while ( ++*n_func < info->n_functions ) {
> +        const struct gcov_fn_info_407 *fn = info->functions[*n_func];
> +
> +        /* the test for info member handle common data redefinitions
> +           in object files */
> +        if ( fn && fn->info == info)
> +             return fn;
> +    }
> +
> +    return NULL;
> +}
> +
> +static inline const struct gcov_ctr_info_407 *
> +next_ctr(const struct gcov_fn_info_407 *fn, int *n_ctr)
> +{
> +    while ( ++*n_ctr < XENCOV_COUNTERS_407 )
> +        if ( fn->info->merge[*n_ctr] )
> +            return &fn->ctrs[*n_ctr];
> +
> +    return NULL;
> +}
> +
>   static inline void align_iter(write_iter_t *iter)
>   {
>       iter->write_offset =
>           (iter->write_offset + sizeof(uint64_t) - 1) & -sizeof(uint64_t);
>   }
>   
> +static int write_info(write_iter_t *iter, const struct gcov_info* info)
> +{
> +    const struct gcov_ctr_info *ctr;
> +    int type, ret;
> +    size_t size_fn = sizeof(struct gcov_fn_info);
> +
> +    /* dump counters */
> +    ctr = info->counts;
> +    for ( type = -1; next_type(info, &type) < XENCOV_COUNTERS_MASK; ++ctr )
> +    {
> +        align_iter(iter);
> +        chk(write32(iter, XENCOV_TAG_COUNTER(type)));
> +        chk(write32(iter, ctr->num));
> +        chk(write_raw(iter, ctr->values,
> +                          ctr->num * sizeof(ctr->values[0])));
> +
> +        size_fn += sizeof(unsigned);
> +    }
> +
> +    /* dump all functions together */
> +    align_iter(iter);
> +    chk(write32(iter, XENCOV_TAG_FUNC));
> +    chk(write32(iter, info->n_functions));
> +    chk(write_raw(iter, info->functions, info->n_functions * size_fn));
> +
> +    return 0;
> +}
> +
> +static int write_info_407(write_iter_t *iter, const struct gcov_info_407* info)
> +{
> +    int ret;
> +    const struct gcov_fn_info_407 *fn;
> +    const struct gcov_ctr_info_407 *ctr;
> +    int n_func, n_ctr;
> +    unsigned int num_func = 0;
> +    unsigned int ctrs[XENCOV_COUNTERS_407];
> +
> +    for ( n_ctr = 0; n_ctr < XENCOV_COUNTERS_407; ++n_ctr )
> +        ctrs[n_ctr] = 0;
> +
> +    /* scan to total counters */
> +    for ( n_func = -1; (fn = next_func(info, &n_func)) != NULL; )
> +    {
> +        ++num_func;
> +        for ( n_ctr = -1; (ctr = next_ctr(fn, &n_ctr)) != NULL; )
> +            ctrs[n_ctr] += ctr->num;
> +    }
> +
> +    /* output counters */
> +    for ( n_ctr = 0; n_ctr < XENCOV_COUNTERS_407; ++n_ctr )
> +    {
> +        if ( !ctrs[n_ctr] ) continue;
> +        align_iter(iter);
> +        chk(write32(iter, XENCOV_TAG_COUNTER(n_ctr)));
> +        chk(write32(iter, ctrs[n_ctr]));
> +        for ( n_func = -1; (fn = next_func(info, &n_func)) != NULL; )
> +        {
> +            ctr = &fn->ctrs[n_ctr];
> +            chk(write_raw(iter, ctr->values,
> +                          ctr->num * sizeof(ctr->values[0])));
> +        }
> +    }
> +
> +    /* dump all functions together */
> +    align_iter(iter);
> +    chk(write32(iter, XENCOV_TAG_FUNC2));
> +    chk(write32(iter, num_func));
> +    for ( n_func = -1; (fn = next_func(info, &n_func)) != NULL; )
> +    {
> +        chk(write32(iter, fn->ident));
> +        chk(write32(iter, fn->lineno_checksum));
> +        chk(write32(iter, fn->cfg_checksum));
> +        for ( n_ctr = -1; (ctr = next_ctr(fn, &n_ctr)) != NULL; )
> +            chk(write32(iter, ctr->num));
> +    }
> +    return 0;
> +}
> +
>   static int write_gcov(write_iter_t *iter)
>   {
>       struct gcov_info *info;
> @@ -128,10 +231,6 @@ static int write_gcov(write_iter_t *iter)
>       /* dump all files */
>       for ( info = info_list ; info; info = info->next )
>       {
> -        const struct gcov_ctr_info *ctr;
> -        int type;
> -        size_t size_fn = sizeof(struct gcov_fn_info);
> -
>           align_iter(iter);
>           chk(write32(iter, XENCOV_TAG_FILE));
>           chk(write32(iter, info->version));
> @@ -139,23 +238,10 @@ static int write_gcov(write_iter_t *iter)
>           chk(write_string(iter, info->filename));
>   
>           /* dump counters */
> -        ctr = info->counts;
> -        for ( type = -1; next_type(info, &type) < XENCOV_COUNTERS; ++ctr )
> -        {
> -            align_iter(iter);
> -            chk(write32(iter, XENCOV_TAG_COUNTER(type)));
> -            chk(write32(iter, ctr->num));
> -            chk(write_raw(iter, ctr->values,
> -                          ctr->num * sizeof(ctr->values[0])));
> -
> -            size_fn += sizeof(unsigned);
> -        }
> -
> -        /* dump all functions together */
> -        align_iter(iter);
> -        chk(write32(iter, XENCOV_TAG_FUNC));
> -        chk(write32(iter, info->n_functions));
> -        chk(write_raw(iter, info->functions, info->n_functions * size_fn));
> +        if (info->version < XENCOV_VERSION_407)
> +            chk(write_info(iter, info));
> +        else
> +            chk(write_info_407(iter, (struct gcov_info_407 *) info));
>       }
>   
>       /* stop tag */
> @@ -164,19 +250,38 @@ static int write_gcov(write_iter_t *iter)
>       return 0;
>   }
>   
> +static void reset_info(struct gcov_info *info)
> +{
> +    const struct gcov_ctr_info *ctr;
> +    int type;
> +
> +    ctr = info->counts;
> +    for ( type = -1; next_type(info, &type) < XENCOV_COUNTERS_MASK; ++ctr )
> +        memset(ctr->values, 0, ctr->num * sizeof(ctr->values[0]));
> +}
> +
> +static void reset_info_407(struct gcov_info_407 *info)
> +{
> +    const struct gcov_fn_info_407 *fn;
> +    const struct gcov_ctr_info_407 *ctr;
> +    int n_func, n_ctr;
> +
> +    for ( n_func = -1; (fn = next_func(info, &n_func)) != NULL; )
> +        for ( n_ctr = -1; (ctr = next_ctr(fn, &n_ctr)) != NULL; )
> +            memset(ctr->values, 0, ctr->num * sizeof(ctr->values[0]));
> +}
> +
>   static int reset_counters(void)
>   {
>       struct gcov_info *info;
>   
>       for ( info = info_list ; info; info = info->next )
>       {
> -        const struct gcov_ctr_info *ctr;
> -        int type;
> -
>           /* reset counters */
> -        ctr = info->counts;
> -        for ( type = -1; next_type(info, &type) < XENCOV_COUNTERS; ++ctr )
> -            memset(ctr->values, 0, ctr->num * sizeof(ctr->values[0]));
> +        if ( info->version < XENCOV_VERSION_407 )
> +            reset_info(info);
> +        else
> +            reset_info_407((struct gcov_info_407 *) info);
>       }
>   
>       return 0;
> diff --git a/xen/include/public/gcov.h b/xen/include/public/gcov.h
> index 1b29b48..e7573fb 100644
> --- 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
> +#define XENCOV_COUNTERS         8
>   #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];
> +};
> +
> +/**
>    * Information for all functions
>    * Aligned to 8 bytes
>    */
> diff --git a/xen/include/xen/gcov.h b/xen/include/xen/gcov.h
> index 27c5c37..f36388a 100644
> --- a/xen/include/xen/gcov.h
> +++ b/xen/include/xen/gcov.h
> @@ -40,6 +40,8 @@ struct gcov_fn_info
>       unsigned int n_ctrs[0];
>   };
>   
> +typedef void (*gcov_merge_func)(gcov_type *, unsigned int);
> +
>   /**
>    * struct gcov_ctr_info - profiling data per counter type
>    * @num: number of counter values for this type
> @@ -53,7 +55,7 @@ struct gcov_ctr_info
>   {
>       unsigned int num;
>       gcov_type *values;
> -    void (*merge)(gcov_type *, unsigned int);
> +    gcov_merge_func merge;
>   };
>   
>   /**
> @@ -82,6 +84,70 @@ struct gcov_info
>       struct gcov_ctr_info      counts[0];
>   };
>   
> +struct gcov_info_407;
> +
> +/**
> + * struct gcov_ctr_info_407 - profiling data per counter type
> + * @num: number of counter values for this type
> + * @values: array of counter values for this type
> + * @merge: merge function for counter values of this type (unused)
> + *
> + * This data is generated by gcc during compilation and doesn't change
> + * at run-time with the exception of the values array.
> + */
> +struct gcov_ctr_info_407
> +{
> +    unsigned int num;
> +    gcov_type *values;
> +};
> +
> +
> +/**
> + * struct gcov_fn_info_407 - profiling meta data per function
> + * @ident: object file-unique function identifier
> + * @lineno_checksum: function lineno checksum
> + * @cfg_checksum: function cfg checksum
> + * @ctrs: counters for this function
> + *
> + * This data is generated by gcc during compilation and doesn't change
> + * at run-time.
> + */
> +struct gcov_fn_info_407
> +{
> +    const struct gcov_info_407 *info;
> +    unsigned int ident;
> +    unsigned int lineno_checksum;
> +    unsigned int cfg_checksum;
> +    struct gcov_ctr_info_407 ctrs[0];
> +};
> +
> +#define XENCOV_COUNTERS_407 8
> +#define XENCOV_VERSION_407 0x34303700
> +
> +/**
> + * struct gcov_info_407 - profiling data per object file
> + * @version: gcov version magic indicating the gcc version used for compilation
> + * @next: list head for a singly-linked list
> + * @stamp: time stamp
> + * @filename: name of the associated gcov data file
> + * @merge: merge functions
> + * @n_functions: number of instrumented functions
> + * @functions: function data
> + *
> + * This data is generated by gcc during compilation and doesn't change
> + * at run-time with the exception of the next pointer.
> + */
> +struct gcov_info_407
> +{
> +    unsigned int              version;
> +    struct gcov_info          *next;
> +    unsigned int              stamp;
> +    const char                *filename;
> +    gcov_merge_func           merge[XENCOV_COUNTERS_407];
> +    unsigned int              n_functions;
> +    const struct gcov_fn_info_407 * const *functions;
> +};
> +
>   
>   /**
>    * Sysctl operations for coverage

Re: [PATCH] gcov: Support gcc 4.7

[Ian Campbell]

On Mon, 2013-06-17 at 09:29 +0100, Frediano Ziglio wrote:
> gcc 4.7 changed format used internally for coverage data.
> This patch address these changes.
> The information that gcc generate are mostly the same but to support common
> sections.
> The only difference in the blob exported by Xen is that functions have 2
> different checksums instead of one.

Do we really need to reflect that in the Xen public API though? Could we
condense it into the things we want/need and try and keep our API
relatively stable? 

I'm a bit concerned that we are going to end up with dozens of variants
of the API (and the internal code) supporting each new variant of this
internal-to-gcc ABI.

I think you explained before why this couldn't be exported from the
hypervisor as an opaque set of bytes to be interpreted by a userspace
tool, which is a shame. I wonder if it is worth asking upstream if they
have any advice.

Ian.

Re: [PATCH] gcov: Support gcc 4.7

[Frediano Ziglio]

On Mon, 2013-06-17 at 11:42 +0100, Ian Campbell wrote:
> On Mon, 2013-06-17 at 09:29 +0100, Frediano Ziglio wrote:
> > gcc 4.7 changed format used internally for coverage data.
> > This patch address these changes.
> > The information that gcc generate are mostly the same but to support common
> > sections.
> > The only difference in the blob exported by Xen is that functions have 2
> > different checksums instead of one.
> 
> Do we really need to reflect that in the Xen public API though? Could we
> condense it into the things we want/need and try and keep our API
> relatively stable? 
> 
> I'm a bit concerned that we are going to end up with dozens of variants
> of the API (and the internal code) supporting each new variant of this
> internal-to-gcc ABI.
> 
> I think you explained before why this couldn't be exported from the
> hypervisor as an opaque set of bytes to be interpreted by a userspace
> tool, which is a shame. I wonder if it is worth asking upstream if they
> have any advice.
> 
> Ian.
> 
> 

Perhaps a simple easy solution would be to allow privileged domains to
read/write arbitrary memory from Xen (write to reset counters).

Frediano

Re: [PATCH] gcov: Support gcc 4.7

[Egger, Christoph]

On 17.06.13 12:42, Ian Campbell wrote:
> On Mon, 2013-06-17 at 09:29 +0100, Frediano Ziglio wrote:
>> gcc 4.7 changed format used internally for coverage data.
>> This patch address these changes.
>> The information that gcc generate are mostly the same but to support common
>> sections.
>> The only difference in the blob exported by Xen is that functions have 2
>> different checksums instead of one.
> 
> Do we really need to reflect that in the Xen public API though? Could we
> condense it into the things we want/need and try and keep our API
> relatively stable? 
> 
> I'm a bit concerned that we are going to end up with dozens of variants
> of the API (and the internal code) supporting each new variant of this
> internal-to-gcc ABI.
> 
> I think you explained before why this couldn't be exported from the
> hypervisor as an opaque set of bytes to be interpreted by a userspace
> tool, which is a shame.

I think this is possible the same way the hypervisor exports
machine check error telemetry.

Christoph

Re: [PATCH] gcov: Support gcc 4.7

[Ian Campbell]

On Mon, 2013-06-17 at 11:46 +0100, Frediano Ziglio wrote:
> Perhaps a simple easy solution would be to allow privileged domains to
> read/write arbitrary memory from Xen (write to reset counters).

"Arbitrary memory" would be a very hard sell.

If all these counters could be constrained to pages which contains these
counters and nothing else then that might be a possibility to consider.

Ian.

Re: [PATCH] gcov: Support gcc 4.7

[Frediano Ziglio]

On Mon, 2013-06-17 at 11:49 +0100, Ian Campbell wrote:
> On Mon, 2013-06-17 at 11:46 +0100, Frediano Ziglio wrote:
> > Perhaps a simple easy solution would be to allow privileged domains to
> > read/write arbitrary memory from Xen (write to reset counters).
> 
> "Arbitrary memory" would be a very hard sell.
> 
> If all these counters could be constrained to pages which contains these
> counters and nothing else then that might be a possibility to consider.
> 
> Ian.
> 

It would be very good if gcc could provide some options to move all
counter in a given separate section and another for all coverage data
(structures and strings). Actually there are no such options. I tried to
change generated .s file but with no success (I don't know how to
generate a common section in a new section, .comm/.lcomm generate always
in .bss section).

I'll write to gcc ML.

Frediano

Re: [PATCH] gcov: Support gcc 4.7

[Frediano Ziglio]

On Mon, 2013-06-17 at 11:49 +0100, Ian Campbell wrote:
> On Mon, 2013-06-17 at 11:46 +0100, Frediano Ziglio wrote:
> > Perhaps a simple easy solution would be to allow privileged domains to
> > read/write arbitrary memory from Xen (write to reset counters).
> 
> "Arbitrary memory" would be a very hard sell.
> 
> If all these counters could be constrained to pages which contains these
> counters and nothing else then that might be a possibility to consider.
> 
> Ian.
> 

It depends on how this would be implemented. A future option of gcc
could help at least having everything in contiguous memory regions.

The fact that these information are generated by the compiler does not
help that much. Every version could change the format "slightly" but
enough to make Xen core when such information are requested!

Actually they changed format for these version of gcc:
- 3.3
- 3.4
- 4.7

And honestly I still don't understand clearly why they changed for gcc
4.7! Probably not for a C feature but to support C++ templates and
comdat. A "smart" preprocessor sit in the middle of gcc and gas that
process assembly generated, check for structures and put everything in
different sections could really help... but is not that easy project!
Another similar way would be to parse the xen-syms output file and
generate needed blob/code. Quite interesting challenge (I did more
terrible things then this) but I don't thing that Citrix would agree.
Possibly a safe thing to do would be in the code to check that compiler
is really gcc and that we can support the version that is compiling,
just to make sure. But is also true that this feature is a debug one
disabled on production so I don't know how worth would be.

Not speaking of clang support, currently quite hard (the structures are
less documented, the code hooks destructors path which is not safe to
call from Xen code which is not expected to terminate).

Frediano

Re: [PATCH] gcov: Support gcc 4.7

[George Dunlap]

On 06/18/2013 03:32 PM, Frediano Ziglio wrote:
> On Mon, 2013-06-17 at 11:49 +0100, Ian Campbell wrote:
>> On Mon, 2013-06-17 at 11:46 +0100, Frediano Ziglio wrote:
>>> Perhaps a simple easy solution would be to allow privileged domains to
>>> read/write arbitrary memory from Xen (write to reset counters).
>>
>> "Arbitrary memory" would be a very hard sell.
>>
>> If all these counters could be constrained to pages which contains these
>> counters and nothing else then that might be a possibility to consider.
>>
>> Ian.
>>
>
> It depends on how this would be implemented. A future option of gcc
> could help at least having everything in contiguous memory regions.
>
> The fact that these information are generated by the compiler does not
> help that much. Every version could change the format "slightly" but
> enough to make Xen core when such information are requested!
>
> Actually they changed format for these version of gcc:
> - 3.3
> - 3.4
> - 4.7
>
> And honestly I still don't understand clearly why they changed for gcc
> 4.7! Probably not for a C feature but to support C++ templates and
> comdat. A "smart" preprocessor sit in the middle of gcc and gas that
> process assembly generated, check for structures and put everything in
> different sections could really help... but is not that easy project!
> Another similar way would be to parse the xen-syms output file and
> generate needed blob/code. Quite interesting challenge (I did more
> terrible things then this) but I don't thing that Citrix would agree.
> Possibly a safe thing to do would be in the code to check that compiler
> is really gcc and that we can support the version that is compiling,
> just to make sure. But is also true that this feature is a debug one
> disabled on production so I don't know how worth would be.
>
> Not speaking of clang support, currently quite hard (the structures are
> less documented, the code hooks destructors path which is not safe to
> call from Xen code which is not expected to terminate).

Doesn't Linux have gcov support via lcov?  Do they have to deal with 
this stuff as well, or is there a generic way to deal with it?

  -George

Re: [PATCH] gcov: Support gcc 4.7

[Frediano Ziglio]

On Tue, 2013-06-18 at 15:53 +0100, George Dunlap wrote:
> On 06/18/2013 03:32 PM, Frediano Ziglio wrote:
> > On Mon, 2013-06-17 at 11:49 +0100, Ian Campbell wrote:
> >> On Mon, 2013-06-17 at 11:46 +0100, Frediano Ziglio wrote:
> >>> Perhaps a simple easy solution would be to allow privileged domains to
> >>> read/write arbitrary memory from Xen (write to reset counters).
> >>
> >> "Arbitrary memory" would be a very hard sell.
> >>
> >> If all these counters could be constrained to pages which contains these
> >> counters and nothing else then that might be a possibility to consider.
> >>
> >> Ian.
> >>
> >
> > It depends on how this would be implemented. A future option of gcc
> > could help at least having everything in contiguous memory regions.
> >
> > The fact that these information are generated by the compiler does not
> > help that much. Every version could change the format "slightly" but
> > enough to make Xen core when such information are requested!
> >
> > Actually they changed format for these version of gcc:
> > - 3.3
> > - 3.4
> > - 4.7
> >
> > And honestly I still don't understand clearly why they changed for gcc
> > 4.7! Probably not for a C feature but to support C++ templates and
> > comdat. A "smart" preprocessor sit in the middle of gcc and gas that
> > process assembly generated, check for structures and put everything in
> > different sections could really help... but is not that easy project!
> > Another similar way would be to parse the xen-syms output file and
> > generate needed blob/code. Quite interesting challenge (I did more
> > terrible things then this) but I don't thing that Citrix would agree.
> > Possibly a safe thing to do would be in the code to check that compiler
> > is really gcc and that we can support the version that is compiling,
> > just to make sure. But is also true that this feature is a debug one
> > disabled on production so I don't know how worth would be.
> >
> > Not speaking of clang support, currently quite hard (the structures are
> > less documented, the code hooks destructors path which is not safe to
> > call from Xen code which is not expected to terminate).
> 
> Doesn't Linux have gcov support via lcov?  Do they have to deal with 
> this stuff as well, or is there a generic way to deal with it?
> 
>   -George

They only support gcc 3.4 format (the one we currently support without
my new patch which add support for 4.7 format)

Frediano

Re: [PATCH] gcov: Support gcc 4.7

[Frediano Ziglio]

On Mon, 2013-06-17 at 09:58 +0100, Jan Beulich wrote:
> >>> On 17.06.13 at 10:29, Frediano Ziglio <frediano.ziglio@citrix.com> 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

Re: [PATCH] gcov: Support gcc 4.7

[Jan Beulich]

>>> On 28.06.13 at 12:18, Frediano Ziglio <frediano.ziglio@citrix.com> wrote:
> On Mon, 2013-06-17 at 09:58 +0100, Jan Beulich wrote:
>> >>> On 17.06.13 at 10:29, Frediano Ziglio <frediano.ziglio@citrix.com> 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