Re: [Xen-devel] [PATCH v2 5/9] gcov: add new interface and 3.4 and 4.7 format support

[Wei Liu]

On Mon, Oct 10, 2016 at 02:11:58PM +0100, Wei Liu wrote:
[...]
> > > +
> > > +#include "gcov.h"
> > > +
> > > +/*
> > > + * __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.
> > 
> > I don't follow - we do such references elsewhere, so I can't see a
> > reason not to follow that model here too as long the call here
> > happens before .init.* get discarded.
> > 
> 
> This is residual from previous implementation. I haven't checked if the
> statement is true or not. If this statement is not true I'm happy to
> make corresponding adjustments.

It turns out that annotating __gcov_init as __init works. I will make
that adjustment in next version.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 5/9] gcov: add new interface and 3.4 and 4.7 format support

[Wei Liu]

On Mon, Oct 10, 2016 at 06:56:52AM -0600, Jan Beulich wrote:
> >>> On 10.10.16 at 14:23,  wrote:
> > On 10/10/16 12:56, Jan Beulich wrote:
> > On 10.10.16 at 11:40,  wrote:
> >>> +struct type_info {
> >>> +int ctr_type;
> >> Can this be negative?
> > 
> > This code is largely imported straight from Linux.  We should not
> > needlessly deviate.
> 
> Hmm, in that case some of the constification I did ask for on v1 and
> which is now there should also be undone? Perhaps for such a

I'm fine with either way. Since the modification is quite heavy, adding
some constification is just minor issue.

> purpose it would help if the original Linux files got first imported
> verbatim (without any review comments, and without getting wired
> up), in order to then be modified minimally to build/work on Xen?
> 

This would be ugly because the code used is only a small portion of what
Linux has (we would end up deleting most of the code -- memory
allocation, seqfile ops etc), and the modification to adapt it to Xen is
quite heavy because the interface is entirely different (procfs vs
hypercall, customised packed file).

Wei.

> Jan
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 5/9] gcov: add new interface and 3.4 and 4.7 format support

[Wei Liu]

On Mon, Oct 10, 2016 at 05:56:35AM -0600, Jan Beulich wrote:
[...]
> Who is 9 (more similar ones below)?
> 
> > +static int counter_active(const struct gcov_info *info, unsigned int type)
> > +{
> > +return info->merge[type] ? 1 : 0;
> 
> Return type bool and preferably with !! instead of conditional
> expression.
> 

Andrew beat me to it: the code and data structures above are mostly
imported from Linux.

> > +size_t gcov_store_u32(void *buffer, size_t off, u32 v)
> > +{
> > +u32 *data;
> 
> Please be consistent wrt u32 vs ...
> 
> > +static int gcov_info_dump_payload(const struct gcov_info *info,
> > +  XEN_GUEST_HANDLE_PARAM(char) buffer,
> > +  uint32_t *off)
> > +{
> > +char *buf;
> > +uint32_t buf_size;
> 
> ... uint32_t (and alike; I'd suggest using only the latter, and I think
> we should get rid of u32 / __u32 and their siblings eventually).
> 

Right. I will switch to using uint32_t.

> > +static uint32_t gcov_get_size(void)
> > +{
> > +uint32_t total_size;
> > +struct gcov_info *info = NULL;
> > +
> > +/* Magic number XCOV */
> > +total_size = sizeof(uint32_t);
> 
> Again - can't this be the initializer?
> 

Sure. I will move it up.

> > +int sysctl_gcov_op(xen_sysctl_gcov_op_t *op)
> > +{
> > +int ret;
> > +
> > +switch ( op->cmd )
> > +{
> > +case XEN_SYSCTL_GCOV_get_size:
> > +op->size = gcov_get_size();
> > +ret = 0;
> > +break;
> > +case XEN_SYSCTL_GCOV_read:
> 
> Blank lines between non-fall-through case statements please.
> 

OK.

> > --- /dev/null
> > +++ b/xen/common/gcov/gcov.h
> > @@ -0,0 +1,36 @@
> > +#ifndef _GCOV_H_
> > +#define _GCOV_H_
> > +
> > +#include 
> > +#include 
> > +
> > +/*
> > + * 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_DATA_MAGIC ((unsigned int) 0x67636461)
> > +#define GCOV_TAG_FUNCTION   ((unsigned int) 0x0100)
> > +#define GCOV_TAG_COUNTER_BASE   ((unsigned int) 0x01a1)
> > +#define GCOV_TAG_FOR_COUNTER(count)\
> > +   _TAG_COUNTER_BASE + ((unsigned int) (count) << 17))
> 
> Stray blanks after casts.
> 

Will fix.

> > --- /dev/null
> > +++ b/xen/common/gcov/gcov_base.c
> > @@ -0,0 +1,64 @@
> > +/*
> > + * Common code across gcov implementations
> > + *
> > + * Copyright Citrix Systems R UK
> > + * Author(s): Wei Liu 
> > + *
> > + *Uses gcc-internal data definitions.
> > + *Based on the gcov-kernel patch by:
> > + *   Hubertus Franke 
> > + *   Nigel Hinds 
> > + *   Rajan Ravindran 
> > + *   Peter Oberparleiter 
> > + *   Paul Larson
> > + */
> > +
> > +#include "gcov.h"
> > +
> > +/*
> > + * __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.
> 
> I don't follow - we do such references elsewhere, so I can't see a
> reason not to follow that model here too as long the call here
> happens before .init.* get discarded.
> 

This is residual from previous implementation. I haven't checked if the
statement is true or not. If this statement is not true I'm happy to
make corresponding adjustments.

> Having reached the end here - what if the user gets the Kconfig setting
> wrong? Wouldn't it be helpful if the gcov__.c files
> #error-ed upon an out of range gcc version?
> 

That's a good idea.

Wei.

> Jan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 5/9] gcov: add new interface and 3.4 and 4.7 format support

[Jan Beulich]

>>> On 10.10.16 at 14:23,  wrote:
> On 10/10/16 12:56, Jan Beulich wrote:
> On 10.10.16 at 11:40,  wrote:
>>> +struct type_info {
>>> +int ctr_type;
>> Can this be negative?
> 
> This code is largely imported straight from Linux.  We should not
> needlessly deviate.

Hmm, in that case some of the constification I did ask for on v1 and
which is now there should also be undone? Perhaps for such a
purpose it would help if the original Linux files got first imported
verbatim (without any review comments, and without getting wired
up), in order to then be modified minimally to build/work on Xen?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 5/9] gcov: add new interface and 3.4 and 4.7 format support

[Andrew Cooper]

On 10/10/16 12:56, Jan Beulich wrote:
 On 10.10.16 at 11:40,  wrote:
>> +struct type_info {
>> +int ctr_type;
> Can this be negative?

This code is largely imported straight from Linux.  We should not
needlessly deviate.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 5/9] gcov: add new interface and 3.4 and 4.7 format support

[Jan Beulich]

>>> On 10.10.16 at 11:40,  wrote:
> +struct type_info {
> +int ctr_type;

Can this be negative?

> +unsigned int offset;
> +};
> +
> +/**
> + * struct gcov_iterator - specifies current file position in logical records
> + * @info: associated profiling data
> + * @record: record type
> + * @function: function number
> + * @type: counter type
> + * @count: index into values array
> + * @num_types: number of counter types
> + * @type_info: helper array to get values-array offset for current function
> + */
> +struct gcov_iterator {
> +const struct gcov_info *info;
> +
> +int record;

This one indeed looks like it can be.

> +unsigned int function;
> +unsigned int type;
> +unsigned int count;
> +
> +int num_types;

But judging by its name, this surely can't be.

> +struct type_info type_info[GCOV_COUNTERS];
> +};
> +
> +/* Mapping of logical record number to actual file content. */
> +#define RECORD_FILE_MAGIC   0
> +#define RECORD_GCOV_VERSION 1
> +#define RECORD_TIME_STAMP   2
> +#define RECORD_FUNCTION_TAG 3
> +#define RECORD_FUNCTON_TAG_LEN  4
> +#define RECORD_FUNCTION_IDENT   5
> +#define RECORD_FUNCTION_CHECK   6
> +#define RECORD_COUNT_TAG7
> +#define RECORD_COUNT_LEN8
> +#define RECORD_COUNT9
> +
> +static int counter_active(const struct gcov_info *info, unsigned int type)
> +{
> +return (1 << type) & info->ctr_mask;

If the function return type is really meant to be a bitmask, then it
should be unsigned int (and 1u). However, I suspect this really
wants to be bool.

> +static size_t get_fn_size(const struct gcov_info *info)
> +{
> +size_t size;
> +
> +size = sizeof(struct gcov_fn_info) + num_counter_active(info) *
> +sizeof(unsigned int);

Any reason this is not the variable's initializer? Also please use
sizeof(*info) and alignof(*info) (below here).

> +static struct gcov_fn_info *get_fn_info(const struct gcov_info *info,
> +unsigned int fn)
> +{
> +return (struct gcov_fn_info *)
> +((char *) info->functions + fn * get_fn_size(info));

Stray blank after (char *) and sub-optimal indentation.

> +static int gcov_iter_next(struct gcov_iterator *iter)
> +{
> +switch ( iter->record )
> +{
> +case RECORD_FILE_MAGIC:
> +case RECORD_GCOV_VERSION:
> +case RECORD_FUNCTION_TAG:
> +case RECORD_FUNCTON_TAG_LEN:
> +case RECORD_FUNCTION_IDENT:
> +case RECORD_COUNT_TAG:
> +/* Advance to next record */
> +iter->record++;
> +break;
> +case RECORD_COUNT:
> +/* Advance to next count */
> +iter->count++;
> +/* fall through */
> +case RECORD_COUNT_LEN:
> +if ( iter->count < get_func(iter)->n_ctrs[iter->type] )
> +{
> +iter->record = 9;

Who is 9 (more similar ones below)?

> +static int counter_active(const struct gcov_info *info, unsigned int type)
> +{
> +return info->merge[type] ? 1 : 0;

Return type bool and preferably with !! instead of conditional
expression.

> +size_t gcov_store_u32(void *buffer, size_t off, u32 v)
> +{
> +u32 *data;

Please be consistent wrt u32 vs ...

> +static int gcov_info_dump_payload(const struct gcov_info *info,
> +  XEN_GUEST_HANDLE_PARAM(char) buffer,
> +  uint32_t *off)
> +{
> +char *buf;
> +uint32_t buf_size;

... uint32_t (and alike; I'd suggest using only the latter, and I think
we should get rid of u32 / __u32 and their siblings eventually).

> +static uint32_t gcov_get_size(void)
> +{
> +uint32_t total_size;
> +struct gcov_info *info = NULL;
> +
> +/* Magic number XCOV */
> +total_size = sizeof(uint32_t);

Again - can't this be the initializer?

> +int sysctl_gcov_op(xen_sysctl_gcov_op_t *op)
> +{
> +int ret;
> +
> +switch ( op->cmd )
> +{
> +case XEN_SYSCTL_GCOV_get_size:
> +op->size = gcov_get_size();
> +ret = 0;
> +break;
> +case XEN_SYSCTL_GCOV_read:

Blank lines between non-fall-through case statements please.

> --- /dev/null
> +++ b/xen/common/gcov/gcov.h
> @@ -0,0 +1,36 @@
> +#ifndef _GCOV_H_
> +#define _GCOV_H_
> +
> +#include 
> +#include 
> +
> +/*
> + * 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_DATA_MAGIC ((unsigned int) 0x67636461)
> +#define GCOV_TAG_FUNCTION   ((unsigned int) 0x0100)
> +#define GCOV_TAG_COUNTER_BASE   ((unsigned int) 0x01a1)
> +#define GCOV_TAG_FOR_COUNTER(count)  \
> + _TAG_COUNTER_BASE + ((unsigned int) (count) << 17))

Stray blanks after casts.

> --- /dev/null
> +++ b/xen/common/gcov/gcov_base.c
> @@ -0,0 +1,64 @@
> +/*
> + * Common code across gcov implementations
> + *
> + * Copyright Citrix Systems R UK
> + * 

[Xen-devel] [PATCH v2 5/9] gcov: add new interface and 3.4 and 4.7 format support

[Wei Liu]

A new sysctl interface for passing gcov data back to userspace. The new
interface uses a customised record file format. The new sysctl reuses
original sysctl number but renames the op to gcov_op.

Both gcc 3.4 and 4.7 format are supported. The code is rewritten so that
a new format can be easily added in the future.  Version specific code
is grouped into different files. The format one needs to use can be
picked via Kconfig. The default format is 4.7 format.

Userspace programs to handle extracted data will come in a later patch.

Signed-off-by: Wei Liu 
---
v2:
1. Use tab in Kconfig and indent help text properly.
2. Bump XEN_SYSCTL_INTERFACE_VERSION.
3. Fix cosmetic issues.

Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
---
 xen/Kconfig.debug   |  24 ++-
 xen/common/gcov/Makefile|   3 +
 xen/common/gcov/gcc_3_4.c   | 363 
 xen/common/gcov/gcc_4_7.c   | 201 
 xen/common/gcov/gcov.c  | 257 +++
 xen/common/gcov/gcov.h  |  36 +
 xen/common/gcov/gcov_base.c |  64 
 xen/common/sysctl.c |   8 +
 xen/include/public/sysctl.h |  39 -
 xen/include/xen/gcov.h  |   9 ++
 10 files changed, 997 insertions(+), 7 deletions(-)
 create mode 100644 xen/common/gcov/gcc_3_4.c
 create mode 100644 xen/common/gcov/gcc_4_7.c
 create mode 100644 xen/common/gcov/gcov.c
 create mode 100644 xen/common/gcov/gcov.h
 create mode 100644 xen/common/gcov/gcov_base.c
 create mode 100644 xen/include/xen/gcov.h

diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index ef863dc..c65e543 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -30,16 +30,30 @@ config FRAME_POINTER
 
 config GCOV
bool "Gcov Support"
-   depends on BROKEN
---help---
  Enable gcov (a test coverage program in GCC) support.
 
- Currently the data structure and hypercall interface are tied
- to GCC 3.4 gcov format. You need to have a version of GCC
- that is compatible with that format to make gcov work.
-
  If unsure, say N here.
 
+choice
+   prompt "Specify Gcov format"
+   depends on GCOV
+   default GCOV_FORMAT_4_7
+   ---help---
+ The gcov format is determined by gcc version.
+
+config GCOV_FORMAT_4_7
+   bool "GCC 4.7 format"
+   ---help---
+ Select this option to use the format specified in GCC 4.7.
+
+config GCOV_FORMAT_3_4
+   bool "GCC 3.4 format"
+   ---help---
+ Select this option to use the format specified in GCC 3.4.
+
+endchoice
+
 config LOCK_PROFILE
bool "Lock Profiling"
---help---
diff --git a/xen/common/gcov/Makefile b/xen/common/gcov/Makefile
index e69de29..03ac1e5 100644
--- a/xen/common/gcov/Makefile
+++ b/xen/common/gcov/Makefile
@@ -0,0 +1,3 @@
+obj-y += gcov_base.o gcov.o
+obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
+obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
diff --git a/xen/common/gcov/gcc_3_4.c b/xen/common/gcov/gcc_3_4.c
new file mode 100644
index 000..2b62b3f
--- /dev/null
+++ b/xen/common/gcov/gcc_3_4.c
@@ -0,0 +1,363 @@
+/*
+ *  This code provides functions to handle gcc's profiling data format
+ *  introduced with gcc 3.4. Future versions of gcc may change the gcov
+ *  format (as happened before), so all format-specific information needs
+ *  to be kept modular and easily exchangeable.
+ *
+ *  This file is based on gcc-internal definitions. Functions and 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 
+ *
+ *Uses gcc-internal data definitions.
+ *
+ *  Imported from Linux and modified for Xen by
+ *Wei Liu 
+ */
+
+
+#include 
+
+#include "gcov.h"
+
+#define GCOV_COUNTERS 5
+
+static struct gcov_info *gcov_info_head;
+
+/**
+ * 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