[RFC PATCH] Coverage support

[RFC PATCH] Coverage support

[Frediano Ziglio]

Updated set of patches for coverage.
Changes:
- implemented way of getting information
- use only 2 parameter in hypercall (still not sure it's better to use
  sysctl)
- use "coverage" for configuration option as suggested by Ian Campbell
- split header in include/public/gcov.h and include/xen/gcov.h to allow
  tools to include public header
- use weak to avoid defining compat_coverage_op

I manage to write 2 tools, one that call the new hypercall and dump the
blob in a file, one that split this file into required .gcda files. 
Both very hack ones and dirty but works and prove the hypervisor code is 
fine. Should I just send them anyway?

The forth patch allow to consume just 0 bytes if coverage is disabled.

[PATCH 1/4] Reserve hypercall number for handling coverage informations.

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 xen/include/public/xen.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 5593066..feec70e 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -101,6 +101,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 #define __HYPERVISOR_kexec_op             37
 #define __HYPERVISOR_tmem_op              38
 #define __HYPERVISOR_xc_reserved_op       39 /* reserved for XenClient */
+#define __HYPERVISOR_coverage             40 /* reserved to get coverage information */
 
 /* Architecture-specific hypercall definitions. */
 #define __HYPERVISOR_arch_0               48
-- 
1.7.9.5

[PATCH 2/4] Adding support for coverage information

[Frediano Ziglio]

This patch introduce coverage support to Xen.
Currently it allows to compile Xen with coverage support but there is no
way to extract them.

The declarations came from Linux source files (as you can see from file
headers).

The idea is to have some operations mainly
- get coverage information size
- read coverage information
- reset coverage counters

The op parameter in the hypercall will be the operation.
The uarg parameter is a pointer (used to read size or data).

Linux use a file system to export these information. The information will
be a blob to handle with some tools (as usually tools require a bunch of
files but Xen does not handle files at all). I'll pack them to make things
simpler as possible.

These information cannot be put in a specific section (allowing a safe
mapping) as gcc use .rodata, .data, .text and .ctors sections.

I added code to handle constructors used in this case to initialize a
linked list of files.

I excluded %.init.o files as they are used before Xen start and should
not have section like .text or .data.

I used a "coverage" configuration option to mimic the "debug" one.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 .gitignore                         |    2 +
 Config.mk                          |    4 ++
 xen/Rules.mk                       |    5 ++
 xen/arch/x86/setup.c               |    3 ++
 xen/arch/x86/x86_64/compat/entry.S |    4 ++
 xen/arch/x86/x86_64/entry.S        |    4 ++
 xen/arch/x86/xen.lds.S             |    7 +++
 xen/common/Makefile                |    2 +
 xen/common/gcov/Makefile           |    5 ++
 xen/common/gcov/gcov.c             |  105 ++++++++++++++++++++++++++++++++++++
 xen/common/gcov/nogcov.c           |   22 ++++++++
 xen/include/public/gcov.h          |   93 ++++++++++++++++++++++++++++++++
 xen/include/xen/gcov.h             |   32 +++++++++++
 xen/include/xen/hypercall.h        |    6 +++
 14 files changed, 294 insertions(+)
 create mode 100644 xen/common/gcov/Makefile
 create mode 100644 xen/common/gcov/gcov.c
 create mode 100644 xen/common/gcov/nogcov.c
 create mode 100644 xen/include/public/gcov.h
 create mode 100644 xen/include/xen/gcov.h

diff --git a/.gitignore b/.gitignore
index 462e291..af48492 100644
--- a/.gitignore
+++ b/.gitignore
@@ -13,6 +13,8 @@
 *.tmp
 *.spot
 *.spit
+*.gcno
+*.gcda
 TAGS
 GTAGS
 GRTAGS
diff --git a/Config.mk b/Config.mk
index 64541c8..a49b7c9 100644
--- a/Config.mk
+++ b/Config.mk
@@ -228,3 +228,7 @@ QEMU_TAG ?= 2a1354d655d816feaad7dbdb8364f40a208439c1
 # doing and are prepared for some pain.
 
 CONFIG_TESTS       ?= y
+
+# Test coverage support
+coverage ?= n
+
diff --git a/xen/Rules.mk b/xen/Rules.mk
index c2db449..c9044f5 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -103,6 +103,11 @@ subdir-all := $(subdir-y) $(subdir-n)
 
 $(filter %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -DINIT_SECTIONS_ONLY
 
+
+ifeq ($(coverage),y)
+$(filter-out %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -fprofile-arcs -ftest-coverage -DTEST_COVERAGE
+endif
+
 ifeq ($(lto),y)
 # Would like to handle all object files as bitcode, but objects made from
 # pure asm are in a different format and have to be collected separately.
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f4d3788..ddf5c4d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -46,6 +46,7 @@
 #include <asm/setup.h>
 #include <xen/cpu.h>
 #include <asm/nmi.h>
+#include <xen/gcov.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool_t __initdata opt_nosmp;
@@ -1313,6 +1314,8 @@ void __init __start_xen(unsigned long mbi_p)
 
     init_trace_bufs();
 
+    init_coverage();
+
     console_endboot();
 
     /* Hide UART from DOM0 if we're using it */
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 7051f90..ebbb6b7 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -413,6 +413,8 @@ ENTRY(compat_hypercall_table)
         .quad do_domctl
         .quad compat_kexec_op
         .quad do_tmem_op
+        .quad compat_ni_hypercall
+        .quad compat_coverage_op
         .rept __HYPERVISOR_arch_0-((.-compat_hypercall_table)/8)
         .quad compat_ni_hypercall
         .endr
@@ -461,6 +463,8 @@ ENTRY(compat_hypercall_args_table)
         .byte 1 /* do_domctl                */
         .byte 2 /* compat_kexec_op          */
         .byte 1 /* do_tmem_op               */
+        .byte 0 /* compat_ni_hypercall      */
+        .byte 2 /* compat_coverage_op       */
         .rept __HYPERVISOR_arch_0-(.-compat_hypercall_args_table)
         .byte 0 /* compat_ni_hypercall      */
         .endr
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 408c348..670a2e6 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -738,6 +738,8 @@ ENTRY(hypercall_table)
         .quad do_domctl
         .quad do_kexec_op
         .quad do_tmem_op
+        .quad do_ni_hypercall
+        .quad do_coverage_op
         .rept __HYPERVISOR_arch_0-((.-hypercall_table)/8)
         .quad do_ni_hypercall
         .endr
@@ -786,6 +788,8 @@ ENTRY(hypercall_args_table)
         .byte 1 /* do_domctl            */
         .byte 2 /* do_kexec             */
         .byte 1 /* do_tmem_op           */
+        .byte 0 /* do_ni_hypercall      */
+        .byte 2 /* do_coverage_op       */
         .rept __HYPERVISOR_arch_0-(.-hypercall_args_table)
         .byte 0 /* do_ni_hypercall      */
         .endr
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d324afd..5570389 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -108,6 +108,13 @@ SECTIONS
        __trampoline_seg_start = .;
        *(.trampoline_seg)
        __trampoline_seg_stop = .;
+
+       . = ALIGN(8);
+       __CTOR_LIST__ = .;
+       QUAD((__CTOR_END__ - __CTOR_LIST__) / 8 - 2)
+       *(.ctors)
+       QUAD(0)
+       __CTOR_END__ = .;
   } :text
   . = ALIGN(32);
   .init.setup : {
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 1677342..51191d6 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -59,5 +59,7 @@ subdir-$(CONFIG_COMPAT) += compat
 
 subdir-$(x86_64) += hvm
 
+subdir-y += gcov
+
 subdir-y += libelf
 subdir-$(HAS_DEVICE_TREE) += libfdt
diff --git a/xen/common/gcov/Makefile b/xen/common/gcov/Makefile
new file mode 100644
index 0000000..43a692c
--- /dev/null
+++ b/xen/common/gcov/Makefile
@@ -0,0 +1,5 @@
+ifneq ($(coverage),y)
+obj-y += nogcov.o
+endif
+obj-$(coverage) += gcov.o
+
diff --git a/xen/common/gcov/gcov.c b/xen/common/gcov/gcov.c
new file mode 100644
index 0000000..e208596
--- /dev/null
+++ b/xen/common/gcov/gcov.c
@@ -0,0 +1,105 @@
+/*
+ *  This code maintains a list of active profiling data structures.
+ *
+ *    Copyright IBM Corp. 2009
+ *    Author(s): Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
+ *
+ *    Uses gcc-internal data definitions.
+ *    Based on the gcov-kernel patch by:
+ *       Hubertus Franke <frankeh@us.ibm.com>
+ *       Nigel Hinds <nhinds@us.ibm.com>
+ *       Rajan Ravindran <rajancr@us.ibm.com>
+ *       Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
+ *       Paul Larson
+ */
+
+#include <xen/config.h>
+#include <xen/init.h>
+#include <xen/lib.h>
+#include <xen/hypercall.h>
+#include <xen/gcov.h>
+#include <xen/errno.h>
+#include <public/xen.h>
+
+static struct gcov_info *info_list;
+static unsigned num_info = 0;
+
+/*
+ * __gcov_init is called by gcc-generated constructor code for each object
+ * file compiled with -fprofile-arcs.
+ *
+ * Although this function is called only during initialization is called from
+ * a .text section which is still present after initialization so not declare
+ * as __init.
+ */
+void __gcov_init(struct gcov_info *info)
+{
+    /* add new profiling data structure to list */
+    info->next = info_list;
+    info_list = info;
+    ++num_info;
+}
+
+/*
+ * These functions may be referenced by gcc-generated profiling code but serve
+ * no function for Xen.
+ */
+void __gcov_flush(void)
+{
+    /* Unused. */
+}
+
+void __gcov_merge_add(gcov_type *counters, unsigned int n_counters)
+{
+    /* Unused. */
+}
+
+void __gcov_merge_single(gcov_type *counters, unsigned int n_counters)
+{
+    /* Unused. */
+}
+
+void __gcov_merge_delta(gcov_type *counters, unsigned int n_counters)
+{
+    /* Unused. */
+}
+
+typedef void (*ctor_func_t)(void);
+extern struct
+{
+    unsigned long count;
+    ctor_func_t funcs[1];
+} __CTOR_LIST__;
+
+void init_coverage(void)
+{
+    unsigned long n;
+    for ( n = 0; n < __CTOR_LIST__.count; ++n )
+        __CTOR_LIST__.funcs[n]();
+
+#ifndef NDEBUG
+    printk(XENLOG_INFO "Initialized %u coverage strucures\n", num_info);
+    if ( info_list )
+        printk(XENLOG_INFO "First coverage file %s\n", info_list->filename);
+#endif
+}
+
+long do_coverage_op(int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
+{
+    return -EINVAL;
+}
+
+#ifdef CONFIG_COMPAT
+int compat_coverage_op(int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
+    __attribute__ ((alias ("do_coverage_op")));
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-set-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/gcov/nogcov.c b/xen/common/gcov/nogcov.c
new file mode 100644
index 0000000..9642c96
--- /dev/null
+++ b/xen/common/gcov/nogcov.c
@@ -0,0 +1,22 @@
+/*
+ * nogov.c
+ *
+ * Compiled if coverage information are not enabled
+ */
+
+#include <xen/config.h>
+#include <xen/lib.h>
+#include <xen/errno.h>
+#include <public/xen.h>
+
+long do_coverage_op(int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
+{
+    return -ENOSYS;
+}
+
+#ifdef CONFIG_COMPAT
+int compat_coverage_op(int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
+    __attribute__ ((alias ("do_coverage_op")));
+#endif
+
+
diff --git a/xen/include/public/gcov.h b/xen/include/public/gcov.h
new file mode 100644
index 0000000..67aedf6
--- /dev/null
+++ b/xen/include/public/gcov.h
@@ -0,0 +1,93 @@
+/*
+ *  Profiling infrastructure declarations.
+ *
+ *  This file is based on gcc-internal definitions. Data structures are
+ *  defined to be compatible with gcc counterparts. For a better
+ *  understanding, refer to gcc source: gcc/gcov-io.h.
+ *
+ *    Copyright IBM Corp. 2009
+ *    Author(s): Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
+ *
+ *    Uses gcc-internal data definitions.
+ */
+
+#ifndef XEN_PUBLIC_GCOV_H
+#define XEN_PUBLIC_GCOV_H XEN_PUBLIC_GCOV_H
+
+/*
+ * Profiling data types used for gcc 3.4 and above - these are defined by
+ * gcc and need to be kept as close to the original definition as possible to
+ * remain compatible.
+ */
+#define GCOV_COUNTERS         5
+#define GCOV_DATA_MAGIC       ((unsigned int) 0x67636461)
+#define GCOV_TAG_FUNCTION     ((unsigned int) 0x01000000)
+#define GCOV_TAG_COUNTER_BASE ((unsigned int) 0x01a10000)
+#define GCOV_TAG_FOR_COUNTER(count) \
+    (GCOV_TAG_COUNTER_BASE + ((unsigned int) (count) << 17))
+
+#if BITS_PER_LONG >= 64
+typedef long gcov_type;
+#else
+typedef long long gcov_type;
+#endif
+
+/**
+ * struct gcov_fn_info - profiling meta data per function
+ * @ident: object file-unique function identifier
+ * @checksum: function checksum
+ * @n_ctrs: number of values per counter type belonging to this function
+ *
+ * This data is generated by gcc during compilation and doesn't change
+ * at run-time.
+ */
+struct gcov_fn_info
+{
+    unsigned int ident;
+    unsigned int checksum;
+    unsigned int n_ctrs[0];
+};
+
+/**
+ * struct gcov_ctr_info - 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
+{
+    unsigned int num;
+    gcov_type *values;
+    void (*merge)(gcov_type *, unsigned int);
+};
+
+/**
+ * struct gcov_info - 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
+ * @n_functions: number of instrumented functions
+ * @functions: function data
+ * @ctr_mask: mask specifying which counter types are active
+ * @counts: counter data per counter type
+ *
+ * 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
+{
+    unsigned int              version;
+    struct gcov_info          *next;
+    unsigned int              stamp;
+    const char                *filename;
+    unsigned int              n_functions;
+    const struct gcov_fn_info *functions;
+    unsigned int              ctr_mask;
+    struct gcov_ctr_info      counts[0];
+};
+
+#endif /* XEN_PUBLIC_GCOV_H */
diff --git a/xen/include/xen/gcov.h b/xen/include/xen/gcov.h
new file mode 100644
index 0000000..517ed75
--- /dev/null
+++ b/xen/include/xen/gcov.h
@@ -0,0 +1,32 @@
+/*
+ *  Profiling infrastructure declarations.
+ *
+ *  This file is based on gcc-internal definitions. Data structures are
+ *  defined to be compatible with gcc counterparts. For a better
+ *  understanding, refer to gcc source: gcc/gcov-io.h.
+ *
+ *    Copyright IBM Corp. 2009
+ *    Author(s): Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
+ *
+ *    Uses gcc-internal data definitions.
+ */
+
+#ifndef XEN_GCOV_H
+#define XEN_GCOV_H XEN_GCOV_H
+
+#include <public/gcov.h>
+
+/**
+ * Initialize coverage informations
+ * Currently create a linked list of all files compiled in with
+ * coverage informations.
+ */
+#ifdef TEST_COVERAGE
+void __init init_coverage(void);
+#else
+static inline void init_coverage(void)
+{
+}
+#endif
+
+#endif /* GCOV_H */
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index 7c3d719..e62525a 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -140,6 +140,9 @@ do_tmem_op(
 extern long
 do_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
 
+extern long
+do_coverage_op(int op, XEN_GUEST_HANDLE_PARAM(void) uarg);
+
 #ifdef CONFIG_COMPAT
 
 extern int
@@ -163,6 +166,9 @@ extern int
 compat_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
 
 extern int
+compat_coverage_op(int op, XEN_GUEST_HANDLE_PARAM(void) uarg);
+
+extern int
 compat_xen_version(
     int cmd,
     XEN_GUEST_HANDLE_PARAM(void) arg);
-- 
1.7.9.5

Re: [PATCH 2/4] Adding support for coverage information

[Jan Beulich]

>>> On 01.02.13 at 15:48, Frediano Ziglio <frediano.ziglio@citrix.com> wrote:
> --- a/Config.mk
> +++ b/Config.mk
> @@ -228,3 +228,7 @@ QEMU_TAG ?= 2a1354d655d816feaad7dbdb8364f40a208439c1
>  # doing and are prepared for some pain.
>  
>  CONFIG_TESTS       ?= y
> +
> +# Test coverage support
> +coverage ?= n
> +

Alongside debug and debug_symbols please.

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -103,6 +103,11 @@ subdir-all := $(subdir-y) $(subdir-n)
>  
>  $(filter %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -DINIT_SECTIONS_ONLY
>  
> +

Stray blank line.

> +ifeq ($(coverage),y)
> +$(filter-out %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -fprofile-arcs -ftest-coverage -DTEST_COVERAGE
> +endif

For one - isn't simply using $(obj-y) here sufficient (i.e. without the
$(filter-out ...))?

And second, I would thing this then ought to become a single line:

$(obj-$(coverage)): CFLAGS += -fprofile-arcs -ftest-coverage -DTEST_COVERAGE

> --- /dev/null
> +++ b/xen/common/gcov/Makefile
> @@ -0,0 +1,5 @@
> +ifneq ($(coverage),y)
> +obj-y += nogcov.o
> +endif
> +obj-$(coverage) += gcov.o
> +

How about

obj-y := nogcov.o
obj-$(coverage) := gcov.o


> +typedef void (*ctor_func_t)(void);
> +extern struct
> +{
> +    unsigned long count;
> +    ctor_func_t funcs[1];
> +} __CTOR_LIST__;

const?

> +
> +void init_coverage(void)

__init

> --- /dev/null
> +++ b/xen/include/public/gcov.h
> @@ -0,0 +1,93 @@
> +/*
> + *  Profiling infrastructure declarations.
> + *
> + *  This file is based on gcc-internal definitions. Data structures are
> + *  defined to be compatible with gcc counterparts. For a better
> + *  understanding, refer to gcc source: gcc/gcov-io.h.
> + *
> + *    Copyright IBM Corp. 2009
> + *    Author(s): Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
> + *
> + *    Uses gcc-internal data definitions.
> + */
> +
> +#ifndef XEN_PUBLIC_GCOV_H
> +#define XEN_PUBLIC_GCOV_H XEN_PUBLIC_GCOV_H
> +
> +/*
> + * Profiling data types used for gcc 3.4 and above - these are defined by
> + * gcc and need to be kept as close to the original definition as possible to
> + * remain compatible.
> + */
> +#define GCOV_COUNTERS         5
> +#define GCOV_DATA_MAGIC       ((unsigned int) 0x67636461)
> +#define GCOV_TAG_FUNCTION     ((unsigned int) 0x01000000)
> +#define GCOV_TAG_COUNTER_BASE ((unsigned int) 0x01a10000)
> +#define GCOV_TAG_FOR_COUNTER(count) \
> +    (GCOV_TAG_COUNTER_BASE + ((unsigned int) (count) << 17))
> +
> +#if BITS_PER_LONG >= 64
> +typedef long gcov_type;
> +#else
> +typedef long long gcov_type;
> +#endif

What's this???

> +/**
> + * struct gcov_ctr_info - 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
> +{
> +    unsigned int num;
> +    gcov_type *values;
> +    void (*merge)(gcov_type *, unsigned int);
> +};

Pointers, and even more so function ones, can't be part of the
public interface.

Jan

Re: [PATCH 2/4] Adding support for coverage information

[Frediano Ziglio]

On Mon, 2013-02-04 at 09:39 +0000, Jan Beulich wrote:
> >>> On 01.02.13 at 15:48, Frediano Ziglio <frediano.ziglio@citrix.com> wrote:
> > --- a/Config.mk
> > +++ b/Config.mk
> > @@ -228,3 +228,7 @@ QEMU_TAG ?= 2a1354d655d816feaad7dbdb8364f40a208439c1
> >  # doing and are prepared for some pain.
> >  
> >  CONFIG_TESTS       ?= y
> > +
> > +# Test coverage support
> > +coverage ?= n
> > +
> 
> Alongside debug and debug_symbols please.

Ok.

> > --- a/xen/Rules.mk
> > +++ b/xen/Rules.mk
> > @@ -103,6 +103,11 @@ subdir-all := $(subdir-y) $(subdir-n)
> >  
> >  $(filter %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -DINIT_SECTIONS_ONLY
> >  
> > +
> 
> Stray blank line.
> 
> > +ifeq ($(coverage),y)
> > +$(filter-out %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -fprofile-arcs -ftest-coverage -DTEST_COVERAGE
> > +endif
> 
> For one - isn't simply using $(obj-y) here sufficient (i.e. without the
> $(filter-out ...))?
> 
> And second, I would thing this then ought to become a single line:
> 
> $(obj-$(coverage)): CFLAGS += -fprofile-arcs -ftest-coverage -DTEST_COVERAGE
> 

Yes, it works fine and seems to compile all proper files but I don't
understand why.
What's the difference between $(obj-bin-y) and $(obj-y) ??

> > --- /dev/null
> > +++ b/xen/common/gcov/Makefile
> > @@ -0,0 +1,5 @@
> > +ifneq ($(coverage),y)
> > +obj-y += nogcov.o
> > +endif
> > +obj-$(coverage) += gcov.o
> > +
> 
> How about
> 
> obj-y := nogcov.o
> obj-$(coverage) := gcov.o
> 

Using sysctl is even simpler, just 

subdir-$(coverage) += gcov

in common/Makefile and

obj-y += gcov.o

in common/gcov/Makefile

> 
> > +typedef void (*ctor_func_t)(void);
> > +extern struct
> > +{
> > +    unsigned long count;
> > +    ctor_func_t funcs[1];
> > +} __CTOR_LIST__;
> 
> const?
> 

Fine.

> > +
> > +void init_coverage(void)
> 
> __init
> 

I removed from previous and put in the header as suggestion.

> > --- /dev/null
> > +++ b/xen/include/public/gcov.h
> > @@ -0,0 +1,93 @@
> > +/*
> > + *  Profiling infrastructure declarations.
> > + *
> > + *  This file is based on gcc-internal definitions. Data structures are
> > + *  defined to be compatible with gcc counterparts. For a better
> > + *  understanding, refer to gcc source: gcc/gcov-io.h.
> > + *
> > + *    Copyright IBM Corp. 2009
> > + *    Author(s): Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
> > + *
> > + *    Uses gcc-internal data definitions.
> > + */
> > +
> > +#ifndef XEN_PUBLIC_GCOV_H
> > +#define XEN_PUBLIC_GCOV_H XEN_PUBLIC_GCOV_H
> > +
> > +/*
> > + * Profiling data types used for gcc 3.4 and above - these are defined by
> > + * gcc and need to be kept as close to the original definition as possible to
> > + * remain compatible.
> > + */
> > +#define GCOV_COUNTERS         5
> > +#define GCOV_DATA_MAGIC       ((unsigned int) 0x67636461)
> > +#define GCOV_TAG_FUNCTION     ((unsigned int) 0x01000000)
> > +#define GCOV_TAG_COUNTER_BASE ((unsigned int) 0x01a10000)
> > +#define GCOV_TAG_FOR_COUNTER(count) \
> > +    (GCOV_TAG_COUNTER_BASE + ((unsigned int) (count) << 17))
> > +
> > +#if BITS_PER_LONG >= 64
> > +typedef long gcov_type;
> > +#else
> > +typedef long long gcov_type;
> > +#endif
> 
> What's this???
> 

This came from Linux, probably for Xen a simple

typedef long gcov_type

is ok.

> > +/**
> > + * struct gcov_ctr_info - 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
> > +{
> > +    unsigned int num;
> > +    gcov_type *values;
> > +    void (*merge)(gcov_type *, unsigned int);
> > +};
> 
> Pointers, and even more so function ones, can't be part of the
> public interface.
> 

Yes, probably there is no need to expose this to public.

Probably defining only the exported blob would be ok and have no licence
issues.

> Jan
> 

Frediano

Re: [PATCH 2/4] Adding support for coverage information

[Jan Beulich]

>>> On 04.02.13 at 16:29, Frediano Ziglio <frediano.ziglio@citrix.com> wrote:
> On Mon, 2013-02-04 at 09:39 +0000, Jan Beulich wrote:
>> >>> On 01.02.13 at 15:48, Frediano Ziglio <frediano.ziglio@citrix.com> wrote:
>> > +ifeq ($(coverage),y)
>> > +$(filter-out %.init.o,$(obj-y) $(obj-bin-y)): CFLAGS += -fprofile-arcs -ftest-coverage -DTEST_COVERAGE
>> > +endif
>> 
>> For one - isn't simply using $(obj-y) here sufficient (i.e. without the
>> $(filter-out ...))?
>> 
>> And second, I would thing this then ought to become a single line:
>> 
>> $(obj-$(coverage)): CFLAGS += -fprofile-arcs -ftest-coverage -DTEST_COVERAGE
>> 
> 
> Yes, it works fine and seems to compile all proper files but I don't
> understand why.
> What's the difference between $(obj-bin-y) and $(obj-y) ??

The former is the set of all .init.o files, which (when building with
clang instead of gcc) must not undergo certain transformations
(and aren't worth it, as init-time only code isn't really performance
critical).

>> > +
>> > +void init_coverage(void)
>> 
>> __init
>> 
> 
> I removed from previous and put in the header as suggestion.

Not sure what you mean here, but just in case - __init
annotations do specifically not belong on declarations, but
only on definitions.

>> > +#if BITS_PER_LONG >= 64
>> > +typedef long gcov_type;
>> > +#else
>> > +typedef long long gcov_type;
>> > +#endif
>> 
>> What's this???
>> 
> 
> This came from Linux, probably for Xen a simple
> 
> typedef long gcov_type
> 
> is ok.

No. Your goal - just to reiterate - must be not to use types the
size of which depends on the guest word size.

Jan

[PATCH 3/4] Implement code to read coverage informations

[Frediano Ziglio]

Implement 4 operations
- check if coverage is compiled in
- read size of coverage information
- read coverage information
- reset coverage counters

Information are stored in a single blob in a format specific to Xen designed
to make code that generate coverage as small as possible.

Check does not check for any permission while other operations require a
privileged domain. This cause these statistics can lead to security
problems.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 xen/common/gcov/gcov.c    |  157 +++++++++++++++++++++++++++++++++++++++++++--
 xen/include/public/gcov.h |   24 +++++++
 2 files changed, 177 insertions(+), 4 deletions(-)

diff --git a/xen/common/gcov/gcov.c b/xen/common/gcov/gcov.c
index e208596..d5d74d0 100644
--- a/xen/common/gcov/gcov.c
+++ b/xen/common/gcov/gcov.c
@@ -19,10 +19,10 @@
 #include <xen/hypercall.h>
 #include <xen/gcov.h>
 #include <xen/errno.h>
+#include <xen/guest_access.h>
 #include <public/xen.h>
 
 static struct gcov_info *info_list;
-static unsigned num_info = 0;
 
 /*
  * __gcov_init is called by gcc-generated constructor code for each object
@@ -37,7 +37,6 @@ void __gcov_init(struct gcov_info *info)
     /* add new profiling data structure to list */
     info->next = info_list;
     info_list = info;
-    ++num_info;
 }
 
 /*
@@ -78,15 +77,165 @@ void init_coverage(void)
         __CTOR_LIST__.funcs[n]();
 
 #ifndef NDEBUG
-    printk(XENLOG_INFO "Initialized %u coverage strucures\n", num_info);
     if ( info_list )
         printk(XENLOG_INFO "First coverage file %s\n", info_list->filename);
 #endif
 }
 
+static inline int counter_active(const struct gcov_info *info, unsigned int type)
+{
+    return (1 << type) & info->ctr_mask;
+}
+
+DEFINE_XEN_GUEST_HANDLE(uint8_t);
+
+typedef struct write_iter_t
+{
+    XEN_GUEST_HANDLE(uint8_t) ptr;
+    int real;
+    unsigned write_offset;
+} write_iter_t;
+
+static int write_raw(struct write_iter_t *iter, const void *data,
+                     size_t data_len)
+{
+    if ( iter->real &&
+        copy_to_guest_offset(iter->ptr, iter->write_offset,
+                             (const unsigned char *) data, data_len) )
+        return -EFAULT;
+
+    iter->write_offset =
+        (iter->write_offset + data_len + (sizeof(uint32_t) - 1)) &
+        -sizeof(uint32_t);
+    return 0;
+}
+
+#define chk(v) do { ret=(v); if ( ret ) return ret; } while(0)
+
+static inline int write32(write_iter_t *iter, uint32_t val)
+{
+    return write_raw(iter, &val, sizeof(val));
+}
+
+static int write_string(write_iter_t *iter, const char *s)
+{
+    int ret;
+    size_t len = strlen(s);
+
+    chk(write32(iter, len));
+    return write_raw(iter, s, len);
+}
+
+static inline int next_type(const struct gcov_info *info, int *type)
+{
+    while ( ++*type < GCOV_COUNTERS && !counter_active(info, *type) )
+        continue;
+    return *type;
+}
+
+static int write_gcov(write_iter_t *iter)
+{
+    struct gcov_info *info;
+    int ret;
+
+    /* reset offset */
+    iter->write_offset = 0;
+
+    /* 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);
+
+        chk(write32(iter, GCOV_DATA_MAGIC));
+        chk(write32(iter, info->version));
+        chk(write32(iter, info->stamp));
+        chk(write_string(iter, info->filename));
+
+        /* dump counters */
+        ctr = info->counts;
+        for ( type = -1; next_type(info, &type) < GCOV_COUNTERS; ++ctr )
+        {
+            chk(write32(iter, GCOV_TAG_FOR_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 */
+        chk(write32(iter, GCOV_TAG_FUNCTION));
+        chk(write32(iter, info->n_functions));
+        chk(write_raw(iter, info->functions, info->n_functions * size_fn));
+    }
+
+    /* stop tag */
+    chk(write32(iter, 0));
+    return 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) < GCOV_COUNTERS; ++ctr )
+            memset(ctr->values, 0, ctr->num * sizeof(ctr->values[0]));
+    }
+
+    return 0;
+}
+
 long do_coverage_op(int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
 {
-    return -EINVAL;
+    long ret = -EINVAL;
+    uint32_t len;
+    write_iter_t iter;
+
+    /*
+     * just return success to check for coverage support,
+     * no permission check
+     */
+    if ( op == XEN_COVERAGE_enabled )
+        return 0;
+
+    /*
+     * all other information although statistical one
+     * are too accurate and could lead to security issues
+     */
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
+
+    switch ( op )
+    {
+    case XEN_COVERAGE_get_total_size:
+        iter.real = 0;
+
+        write_gcov(&iter);
+        len = iter.write_offset;
+        ret = copy_to_guest(uarg, &len, 1) ? -EFAULT : 0;
+        break;
+
+    case XEN_COVERAGE_read:
+        iter.ptr = guest_handle_cast(uarg, uint8_t);
+        iter.real = 1;
+
+        ret = write_gcov(&iter);
+        break;
+
+    case XEN_COVERAGE_reset:
+        ret = reset_counters();
+        break;
+    }
+    return ret;
 }
 
 #ifdef CONFIG_COMPAT
diff --git a/xen/include/public/gcov.h b/xen/include/public/gcov.h
index 67aedf6..a2863f7 100644
--- a/xen/include/public/gcov.h
+++ b/xen/include/public/gcov.h
@@ -90,4 +90,28 @@ struct gcov_info
     struct gcov_ctr_info      counts[0];
 };
 
+
+/*
+ * Check if coverage informations are available
+ * return just success or error, no parameters
+ */
+#define XEN_COVERAGE_enabled        0
+
+/*
+ * Get total size of information, to help allocate
+ * the buffer. The pointer points to a 32 bit value.
+ */
+#define XEN_COVERAGE_get_total_size 1
+
+/*
+ * Read coverage information in a single run
+ * You must use a tool to split them
+ */
+#define XEN_COVERAGE_read           2
+
+/*
+ * Reset all the coverage counters to 0
+ */
+#define XEN_COVERAGE_reset          3
+
 #endif /* XEN_PUBLIC_GCOV_H */
-- 
1.7.9.5

[PATCH 4/4] Use weak symbol to avoid bad trick in Makefiles

[Frediano Ziglio]

Define do_coverage_op to point to do_ni_hypercall as weak symbol.
This allow the linked to redirect coverage operations to void function if
coverage are not enabled.
This save some space and compression bytes if coverage is disabled.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 xen/common/Makefile        |    2 +-
 xen/common/compat/kernel.c |    1 +
 xen/common/gcov/Makefile   |    5 +----
 xen/common/gcov/nogcov.c   |   22 ----------------------
 xen/common/kernel.c        |    4 ++++
 5 files changed, 7 insertions(+), 27 deletions(-)
 delete mode 100644 xen/common/gcov/nogcov.c

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 51191d6..8a0c506 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -59,7 +59,7 @@ subdir-$(CONFIG_COMPAT) += compat
 
 subdir-$(x86_64) += hvm
 
-subdir-y += gcov
+subdir-$(coverage) += gcov
 
 subdir-y += libelf
 subdir-$(HAS_DEVICE_TREE) += libfdt
diff --git a/xen/common/compat/kernel.c b/xen/common/compat/kernel.c
index a2a2751..564c2a1 100644
--- a/xen/common/compat/kernel.c
+++ b/xen/common/compat/kernel.c
@@ -42,6 +42,7 @@ CHECK_TYPE(domain_handle);
 #define xennmi_callback_t compat_nmi_callback_t
 
 #define DO(fn) int compat_##fn
+#define NAME(fn) "compat_" #fn
 #define COMPAT
 
 #include "../kernel.c"
diff --git a/xen/common/gcov/Makefile b/xen/common/gcov/Makefile
index 43a692c..c9efe6c 100644
--- a/xen/common/gcov/Makefile
+++ b/xen/common/gcov/Makefile
@@ -1,5 +1,2 @@
-ifneq ($(coverage),y)
-obj-y += nogcov.o
-endif
-obj-$(coverage) += gcov.o
+obj-y += gcov.o
 
diff --git a/xen/common/gcov/nogcov.c b/xen/common/gcov/nogcov.c
deleted file mode 100644
index 9642c96..0000000
--- a/xen/common/gcov/nogcov.c
+++ /dev/null
@@ -1,22 +0,0 @@
-/*
- * nogov.c
- *
- * Compiled if coverage information are not enabled
- */
-
-#include <xen/config.h>
-#include <xen/lib.h>
-#include <xen/errno.h>
-#include <public/xen.h>
-
-long do_coverage_op(int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
-{
-    return -ENOSYS;
-}
-
-#ifdef CONFIG_COMPAT
-int compat_coverage_op(int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
-    __attribute__ ((alias ("do_coverage_op")));
-#endif
-
-
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 55caff6..25ba667 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -197,6 +197,7 @@ void __init do_initcalls(void)
 }
 
 # define DO(fn) long do_##fn
+# define NAME(fn) "do_" #fn
 
 #endif
 
@@ -367,6 +368,9 @@ DO(ni_hypercall)(void)
     return -ENOSYS;
 }
 
+DO(coverage_op)(int op, XEN_GUEST_HANDLE_PARAM(void) uarg)
+    __attribute__ ((weak, alias(NAME(ni_hypercall))));
+
 /*
  * Local variables:
  * mode: C
-- 
1.7.9.5

Re: [RFC PATCH] Coverage support

[Frediano Ziglio]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

On Fri, 2013-02-01 at 14:48 +0000, Frediano Ziglio wrote:
> Updated set of patches for coverage.
> Changes:
> - implemented way of getting information
> - use only 2 parameter in hypercall (still not sure it's better to use
>   sysctl)
> - use "coverage" for configuration option as suggested by Ian Campbell
> - split header in include/public/gcov.h and include/xen/gcov.h to allow
>   tools to include public header
> - use weak to avoid defining compat_coverage_op
> 
> I manage to write 2 tools, one that call the new hypercall and dump the
> blob in a file, one that split this file into required .gcda files. 
> Both very hack ones and dirty but works and prove the hypervisor code is 
> fine. Should I just send them anyway?
> 
> The forth patch allow to consume just 0 bytes if coverage is disabled.
> 

These are the two utilities. If anybody wants to try.

Frediano


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: get_coverage.c --]
[-- Type: text/x-csrc; name="get_coverage.c", Size: 2783 bytes --]

/* get_coverage - program to get coverage informations from Xen
 * 
 * Copyright (C) 2013  - Citrix Systems
 * -----
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License, or
 * (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */

#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/ioctl.h>
#include <inttypes.h>
#include <sys/mman.h>
#include <err.h>

typedef struct privcmd_hypercall
{
	uint64_t op;
	uint64_t arg[5];
} privcmd_hypercall_t;

#define IOCTL_PRIVCMD_HYPERCALL								 \
	_IOC(_IOC_NONE, 'P', 0, sizeof(privcmd_hypercall_t))

#define PAGE_SIZE 4096u

static int privcmd_fd = -1;

int get_gcov_info(int op, void *ptr)
{
	privcmd_hypercall_t hc[1];
	int ret;

	hc->op = 40; // TODO
	hc->arg[0] = op;
	hc->arg[1] = (intptr_t) ptr;
	ret = ioctl(privcmd_fd, IOCTL_PRIVCMD_HYPERCALL, hc);
	if (ret >= 0)
		return ret;
	return -errno;
}

int main(void)
{
	// open priv command
	privcmd_fd = open("/proc/xen/privcmd", O_RDWR);
	if (privcmd_fd < 0)
		err(1, "opening privcmd");

	// check support
	if (get_gcov_info(0, NULL) < 0)
		err(1, "checking coverage support");

	// allocate
	uint8_t *p = mmap(0, PAGE_SIZE, PROT_WRITE|PROT_READ, MAP_PRIVATE|MAP_ANON|MAP_LOCKED, -1, 0);
	if (p == (uint8_t *) -1)
		err(1, "allocating memory");

	// get total length
	uint32_t *ui = (uint32_t *) p;
	*ui = 0;
	if (get_gcov_info(1, ui) < 0)
		err(1, "getting total length");
	printf("returned %u bytes\n", (unsigned) *ui);

	// safe check
	uint32_t total_len = *ui;
	if (total_len > 16u * 1024u * 1024u)
		errx(1, "coverage size too big %u bytes\n", total_len);

	// reallocate
	munmap(p, PAGE_SIZE);
	size_t size = total_len + PAGE_SIZE;
	size -= (size % PAGE_SIZE);
	p = mmap(0, size, PROT_WRITE|PROT_READ, MAP_PRIVATE|MAP_ANON|MAP_LOCKED, -1, 0);
	if (p == (uint8_t *) -1)
		err(1, "mapping memory for coverage");

	// get data
	if (get_gcov_info(2, p) < 0)
		err(1, "getting coverage information");

	// write to a file
	FILE *f = fopen("gcda.dat", "w");
	if (!f)
		err(1, "opening output file");
	if (fwrite(p, 1, total_len, f) != total_len)
		err(1, "writing coverage to file");
	fclose(f);

	return 0;
}

[-- Attachment #3: split_coverage.pl --]
[-- Type: application/x-perl, Size: 3957 bytes --]

[-- Attachment #4: Type: text/plain, Size: 126 bytes --]

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

Re: [RFC PATCH] Coverage support

[Ian Campbell]

On Fri, 2013-02-01 at 15:46 +0000, Frediano Ziglio wrote:
> On Fri, 2013-02-01 at 14:48 +0000, Frediano Ziglio wrote:
> > Updated set of patches for coverage.
> > Changes:
> > - implemented way of getting information
> > - use only 2 parameter in hypercall (still not sure it's better to use
> >   sysctl)
> > - use "coverage" for configuration option as suggested by Ian Campbell
> > - split header in include/public/gcov.h and include/xen/gcov.h to allow
> >   tools to include public header
> > - use weak to avoid defining compat_coverage_op
> > 
> > I manage to write 2 tools, one that call the new hypercall and dump the
> > blob in a file, one that split this file into required .gcda files. 
> > Both very hack ones and dirty but works and prove the hypervisor code is 
> > fine. Should I just send them anyway?
> > 
> > The forth patch allow to consume just 0 bytes if coverage is disabled.
> > 
> 
> These are the two utilities. If anybody wants to try.

Thanks, are you going to eventually post these for inclusion in the
tree? (tools/misc or somewhere perhaps).

Ian.

Re: [RFC PATCH] Coverage support

[Jan Beulich]

>>> On 01.02.13 at 15:48, Frediano Ziglio <frediano.ziglio@citrix.com> wrote:
> Updated set of patches for coverage.
> Changes:
> - implemented way of getting information
> - use only 2 parameter in hypercall (still not sure it's better to use
>   sysctl)
> - use "coverage" for configuration option as suggested by Ian Campbell
> - split header in include/public/gcov.h and include/xen/gcov.h to allow
>   tools to include public header
> - use weak to avoid defining compat_coverage_op

While probably not formally documented anywhere, we decided
against using weak some time ago.

Furthermore, iirc I already told you that with a suitably defined
interface you won't need a compat variant at all (and in particular
that would be the case if you went the sysctl route, as was
suggested to you).

Jan

Re: [RFC PATCH] Coverage support

[Frediano Ziglio]

On Mon, 2013-02-04 at 09:27 +0000, Jan Beulich wrote:
> >>> On 01.02.13 at 15:48, Frediano Ziglio <frediano.ziglio@citrix.com> wrote:
> > Updated set of patches for coverage.
> > Changes:
> > - implemented way of getting information
> > - use only 2 parameter in hypercall (still not sure it's better to use
> >   sysctl)
> > - use "coverage" for configuration option as suggested by Ian Campbell
> > - split header in include/public/gcov.h and include/xen/gcov.h to allow
> >   tools to include public header
> > - use weak to avoid defining compat_coverage_op
> 
> While probably not formally documented anywhere, we decided
> against using weak some time ago.
> 
> Furthermore, iirc I already told you that with a suitably defined
> interface you won't need a compat variant at all (and in particular
> that would be the case if you went the sysctl route, as was
> suggested to you).
> 
> Jan
> 

I changed today the interface to use sysctl instead of directly
hypercall. No weak symbols and easier makefiles. Still to pack updated
version (see my mail about possible license issues).

Frediano