[PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/common/gcov/gcov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/gcov/gcov.c b/xen/common/gcov/gcov.c
index 2f18f6d176..35653fd8d8 100644
--- a/xen/common/gcov/gcov.c
+++ b/xen/common/gcov/gcov.c
@@ -239,7 +239,7 @@ int sysctl_gcov_op(struct xen_sysctl_gcov_op *op)
         break;
 
     default:
-        ret = -EINVAL;
+        ret = -ENOSYS;
         break;
     }
 
-- 
2.13.5 (Apple Git-94)


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

Re: [PATCH for-4.10 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[Ian Jackson]

Roger Pau Monne writes ("[PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl"):
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
...
>      default:
> -        ret = -EINVAL;
> +        ret = -ENOSYS;
>          break;
>      }

Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

I think this is a bugfix which should go into 4.10.  Julien ?
(Subject line changed by me.)

Ian.

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

Re: [PATCH for-4.10 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[Julien Grall]

Hi,

On 27/10/17 14:59, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl"):
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ...
>>       default:
>> -        ret = -EINVAL;
>> +        ret = -ENOSYS;
>>           break;
>>       }
> 
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> I think this is a bugfix which should go into 4.10.  Julien ?
> (Subject line changed by me.)

Yes I agree.

Release-acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

-- 
Julien Grall

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

Re: [PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[Jan Beulich]

>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
> --- a/xen/common/gcov/gcov.c
> +++ b/xen/common/gcov/gcov.c
> @@ -239,7 +239,7 @@ int sysctl_gcov_op(struct xen_sysctl_gcov_op *op)
>          break;
>  
>      default:
> -        ret = -EINVAL;
> +        ret = -ENOSYS;
>          break;
>      }

Very certainly ENOSYS is not in any way better. Despite the many
misuses of it, we've started enforcing that this wouldn't be spread.
-EOPNOTSUPP may be fine here, but -EINVAL is suitable as well.
-ENOSYS exclusively means that a _top level_ hypercall is
unimplemented (i.e. with very few exceptions there should be
exactly one place where it gets returned, which is in the main
hypercall dispatch code).

Jan


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

Re: [PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl"):
> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
> > --- a/xen/common/gcov/gcov.c
> > +++ b/xen/common/gcov/gcov.c
> > @@ -239,7 +239,7 @@ int sysctl_gcov_op(struct xen_sysctl_gcov_op *op)
> >          break;
> >  
> >      default:
> > -        ret = -EINVAL;
> > +        ret = -ENOSYS;
> >          break;
> >      }
> 
> Very certainly ENOSYS is not in any way better. Despite the many
> misuses of it, we've started enforcing that this wouldn't be spread.
> -EOPNOTSUPP may be fine here, but -EINVAL is suitable as well.
> -ENOSYS exclusively means that a _top level_ hypercall is
> unimplemented (i.e. with very few exceptions there should be
> exactly one place where it gets returned, which is in the main
> hypercall dispatch code).

The distinction between unimplemented status of a top-level hypercall
and unimplemented status of a sub-op is rarely useful to the caller.

Conversely, the distinction between an unimplemented facility, and a
facility which is exists but is being used improperly, is vitally
important to anyone who is trying to write compatibility code.

I don't mind if you want to insist on the former distinction,
reserving ENOSYS for top-level hypercalls and EOPNOTSUPP for other
functions.

But I absolutely do mind the use of EINVAL for "unsupported function".
I appreciate that much of the hypervisor has historically used EINVAL
this way, but this is (a) a pain for callers (b) evil, bad, and wrong
(c) unnecessary since EOPNOTSUPP is available.  We should at least not
perpetrate any more of this.  In an unreleased API we should change it
before release.

Thanks for your attention.

Ian.

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

Re: [PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[Jan Beulich]

>>> On 06.11.17 at 12:16, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH for-next 1/9] gcov: return ENOSYS for 
> unimplemented gcov domctl"):
>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
>> > --- a/xen/common/gcov/gcov.c
>> > +++ b/xen/common/gcov/gcov.c
>> > @@ -239,7 +239,7 @@ int sysctl_gcov_op(struct xen_sysctl_gcov_op *op)
>> >          break;
>> >  
>> >      default:
>> > -        ret = -EINVAL;
>> > +        ret = -ENOSYS;
>> >          break;
>> >      }
>> 
>> Very certainly ENOSYS is not in any way better. Despite the many
>> misuses of it, we've started enforcing that this wouldn't be spread.
>> -EOPNOTSUPP may be fine here, but -EINVAL is suitable as well.
>> -ENOSYS exclusively means that a _top level_ hypercall is
>> unimplemented (i.e. with very few exceptions there should be
>> exactly one place where it gets returned, which is in the main
>> hypercall dispatch code).
> 
> The distinction between unimplemented status of a top-level hypercall
> and unimplemented status of a sub-op is rarely useful to the caller.
> 
> Conversely, the distinction between an unimplemented facility, and a
> facility which is exists but is being used improperly, is vitally
> important to anyone who is trying to write compatibility code.
> 
> I don't mind if you want to insist on the former distinction,
> reserving ENOSYS for top-level hypercalls and EOPNOTSUPP for other
> functions.
> 
> But I absolutely do mind the use of EINVAL for "unsupported function".
> I appreciate that much of the hypervisor has historically used EINVAL
> this way, but this is (a) a pain for callers (b) evil, bad, and wrong
> (c) unnecessary since EOPNOTSUPP is available.  We should at least not
> perpetrate any more of this.  In an unreleased API we should change it
> before release.

Okay, so EOPNOTSUPP is it then, which is also my preference
(due to there being so many uses of EINVAL elsewhere). I've
merely mentioned that EINVAL would be suitable since,
technically speaking, the value in a "sub-operation" field being
invalid is no different from this being the case for the value in
any other field.

Jan


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

Re: [PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[Roger Pau Monné]

On Mon, Nov 06, 2017 at 05:06:18AM -0700, Jan Beulich wrote:
> >>> On 06.11.17 at 12:16, <ian.jackson@eu.citrix.com> wrote:
> > Jan Beulich writes ("Re: [PATCH for-next 1/9] gcov: return ENOSYS for 
> > unimplemented gcov domctl"):
> >> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
> >> > --- a/xen/common/gcov/gcov.c
> >> > +++ b/xen/common/gcov/gcov.c
> >> > @@ -239,7 +239,7 @@ int sysctl_gcov_op(struct xen_sysctl_gcov_op *op)
> >> >          break;
> >> >  
> >> >      default:
> >> > -        ret = -EINVAL;
> >> > +        ret = -ENOSYS;
> >> >          break;
> >> >      }
> >> 
> >> Very certainly ENOSYS is not in any way better. Despite the many
> >> misuses of it, we've started enforcing that this wouldn't be spread.
> >> -EOPNOTSUPP may be fine here, but -EINVAL is suitable as well.
> >> -ENOSYS exclusively means that a _top level_ hypercall is
> >> unimplemented (i.e. with very few exceptions there should be
> >> exactly one place where it gets returned, which is in the main
> >> hypercall dispatch code).
> > 
> > The distinction between unimplemented status of a top-level hypercall
> > and unimplemented status of a sub-op is rarely useful to the caller.
> > 
> > Conversely, the distinction between an unimplemented facility, and a
> > facility which is exists but is being used improperly, is vitally
> > important to anyone who is trying to write compatibility code.
> > 
> > I don't mind if you want to insist on the former distinction,
> > reserving ENOSYS for top-level hypercalls and EOPNOTSUPP for other
> > functions.
> > 
> > But I absolutely do mind the use of EINVAL for "unsupported function".
> > I appreciate that much of the hypervisor has historically used EINVAL
> > this way, but this is (a) a pain for callers (b) evil, bad, and wrong
> > (c) unnecessary since EOPNOTSUPP is available.  We should at least not
> > perpetrate any more of this.  In an unreleased API we should change it
> > before release.

This API has actually been released since ~2013 IIRC, when it was
added to Xen.

> Okay, so EOPNOTSUPP is it then, which is also my preference
> (due to there being so many uses of EINVAL elsewhere). I've
> merely mentioned that EINVAL would be suitable since,
> technically speaking, the value in a "sub-operation" field being
> invalid is no different from this being the case for the value in
> any other field.

If I don't get any more comments I will re-send this patch separately
using EOPNOTSUPP instead of ENOSYS. I will also keep the Acks gathered
so far unless anyone objects.

Thanks, Roger.

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

Re: [PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[Wei Liu]

On Tue, Nov 07, 2017 at 09:41:58AM +0000, Roger Pau Monné wrote:
> > Okay, so EOPNOTSUPP is it then, which is also my preference
> > (due to there being so many uses of EINVAL elsewhere). I've
> > merely mentioned that EINVAL would be suitable since,
> > technically speaking, the value in a "sub-operation" field being
> > invalid is no different from this being the case for the value in
> > any other field.
> 
> If I don't get any more comments I will re-send this patch separately
> using EOPNOTSUPP instead of ENOSYS. I will also keep the Acks gathered
> so far unless anyone objects.
> 

Please send a new patch. I believe this one is already applied.

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

[PATCH for-next 2/9] gcov: rename folder and header to coverage

[Roger Pau Monne]

Preparatory change before adding llvm profiling support.
No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/common/Makefile                       | 2 +-
 xen/common/{gcov => coverage}/Makefile    | 0
 xen/common/{gcov => coverage}/gcc_3_4.c   | 0
 xen/common/{gcov => coverage}/gcc_4_7.c   | 0
 xen/common/{gcov => coverage}/gcc_4_9.c   | 0
 xen/common/{gcov => coverage}/gcc_5.c     | 0
 xen/common/{gcov => coverage}/gcc_7.c     | 0
 xen/common/{gcov => coverage}/gcov.c      | 0
 xen/common/{gcov => coverage}/gcov.h      | 0
 xen/common/{gcov => coverage}/gcov_base.c | 0
 xen/common/sysctl.c                       | 2 +-
 xen/include/xen/{gcov.h => coverage.h}    | 0
 12 files changed, 2 insertions(+), 2 deletions(-)
 rename xen/common/{gcov => coverage}/Makefile (100%)
 rename xen/common/{gcov => coverage}/gcc_3_4.c (100%)
 rename xen/common/{gcov => coverage}/gcc_4_7.c (100%)
 rename xen/common/{gcov => coverage}/gcc_4_9.c (100%)
 rename xen/common/{gcov => coverage}/gcc_5.c (100%)
 rename xen/common/{gcov => coverage}/gcc_7.c (100%)
 rename xen/common/{gcov => coverage}/gcov.c (100%)
 rename xen/common/{gcov => coverage}/gcov.h (100%)
 rename xen/common/{gcov => coverage}/gcov_base.c (100%)
 rename xen/include/xen/{gcov.h => coverage.h} (100%)

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 66cc2c8995..ad181636f6 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -74,7 +74,7 @@ tmem-y := tmem.o tmem_xen.o tmem_control.o
 tmem-$(CONFIG_COMPAT) += compat/tmem_xen.o
 obj-$(CONFIG_TMEM) += $(tmem-y)
 
-subdir-$(CONFIG_GCOV) += gcov
+subdir-$(CONFIG_GCOV) += coverage
 subdir-$(CONFIG_UBSAN) += ubsan
 
 subdir-y += libelf
diff --git a/xen/common/gcov/Makefile b/xen/common/coverage/Makefile
similarity index 100%
rename from xen/common/gcov/Makefile
rename to xen/common/coverage/Makefile
diff --git a/xen/common/gcov/gcc_3_4.c b/xen/common/coverage/gcc_3_4.c
similarity index 100%
rename from xen/common/gcov/gcc_3_4.c
rename to xen/common/coverage/gcc_3_4.c
diff --git a/xen/common/gcov/gcc_4_7.c b/xen/common/coverage/gcc_4_7.c
similarity index 100%
rename from xen/common/gcov/gcc_4_7.c
rename to xen/common/coverage/gcc_4_7.c
diff --git a/xen/common/gcov/gcc_4_9.c b/xen/common/coverage/gcc_4_9.c
similarity index 100%
rename from xen/common/gcov/gcc_4_9.c
rename to xen/common/coverage/gcc_4_9.c
diff --git a/xen/common/gcov/gcc_5.c b/xen/common/coverage/gcc_5.c
similarity index 100%
rename from xen/common/gcov/gcc_5.c
rename to xen/common/coverage/gcc_5.c
diff --git a/xen/common/gcov/gcc_7.c b/xen/common/coverage/gcc_7.c
similarity index 100%
rename from xen/common/gcov/gcc_7.c
rename to xen/common/coverage/gcc_7.c
diff --git a/xen/common/gcov/gcov.c b/xen/common/coverage/gcov.c
similarity index 100%
rename from xen/common/gcov/gcov.c
rename to xen/common/coverage/gcov.c
diff --git a/xen/common/gcov/gcov.h b/xen/common/coverage/gcov.h
similarity index 100%
rename from xen/common/gcov/gcov.h
rename to xen/common/coverage/gcov.h
diff --git a/xen/common/gcov/gcov_base.c b/xen/common/coverage/gcov_base.c
similarity index 100%
rename from xen/common/gcov/gcov_base.c
rename to xen/common/coverage/gcov_base.c
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 08198b7150..56def766e6 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -27,7 +27,7 @@
 #include <xsm/xsm.h>
 #include <xen/pmstat.h>
 #include <xen/livepatch.h>
-#include <xen/gcov.h>
+#include <xen/coverage.h>
 
 long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
 {
diff --git a/xen/include/xen/gcov.h b/xen/include/xen/coverage.h
similarity index 100%
rename from xen/include/xen/gcov.h
rename to xen/include/xen/coverage.h
-- 
2.13.5 (Apple Git-94)


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

Re: [PATCH for-next 2/9] gcov: rename folder and header to coverage

[Wei Liu]

> diff --git a/xen/include/xen/gcov.h b/xen/include/xen/coverage.h
> similarity index 100%
> rename from xen/include/xen/gcov.h
> rename to xen/include/xen/coverage.h

Please also rename the define guard in this file.

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

[PATCH for-next 3/9] gcov: rename sysctl and functions

[Roger Pau Monne]

Change gcov to cov.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
---
 tools/misc/xencov.c         | 28 ++++++++++++++--------------
 xen/common/coverage/gcov.c  |  8 ++++----
 xen/common/sysctl.c         |  4 ++--
 xen/include/public/sysctl.h | 12 ++++++------
 xen/include/xen/coverage.h  |  2 +-
 5 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/tools/misc/xencov.c b/tools/misc/xencov.c
index 4130f425dc..5db00c37db 100644
--- a/tools/misc/xencov.c
+++ b/tools/misc/xencov.c
@@ -26,31 +26,31 @@
 
 static xc_interface *xch = NULL;
 
-int gcov_sysctl(int op, struct xen_sysctl *sysctl,
-                struct xc_hypercall_buffer *buf, uint32_t buf_size)
+int cov_sysctl(int op, struct xen_sysctl *sysctl,
+               struct xc_hypercall_buffer *buf, uint32_t buf_size)
 {
     DECLARE_HYPERCALL_BUFFER_ARGUMENT(buf);
 
     memset(sysctl, 0, sizeof(*sysctl));
-    sysctl->cmd = XEN_SYSCTL_gcov_op;
+    sysctl->cmd = XEN_SYSCTL_cov_op;
 
-    sysctl->u.gcov_op.cmd = op;
-    sysctl->u.gcov_op.size = buf_size;
-    set_xen_guest_handle(sysctl->u.gcov_op.buffer, buf);
+    sysctl->u.cov_op.cmd = op;
+    sysctl->u.cov_op.size = buf_size;
+    set_xen_guest_handle(sysctl->u.cov_op.buffer, buf);
 
     return xc_sysctl(xch, sysctl);
 }
 
-static void gcov_read(const char *fn)
+static void cov_read(const char *fn)
 {
     struct xen_sysctl sys;
     uint32_t total_len;
     DECLARE_HYPERCALL_BUFFER(uint8_t, p);
     FILE *f;
 
-    if (gcov_sysctl(XEN_SYSCTL_GCOV_get_size, &sys, NULL, 0) < 0)
+    if (cov_sysctl(XEN_SYSCTL_COV_get_size, &sys, NULL, 0) < 0)
         err(1, "getting total length");
-    total_len = sys.u.gcov_op.size;
+    total_len = sys.u.cov_op.size;
 
     /* Shouldn't exceed a few hundred kilobytes */
     if (total_len > 8u * 1024u * 1024u)
@@ -61,7 +61,7 @@ static void gcov_read(const char *fn)
         err(1, "allocating buffer");
 
     memset(p, 0, total_len);
-    if (gcov_sysctl(XEN_SYSCTL_GCOV_read, &sys, HYPERCALL_BUFFER(p),
+    if (cov_sysctl(XEN_SYSCTL_COV_read, &sys, HYPERCALL_BUFFER(p),
                     total_len) < 0)
         err(1, "getting gcov data");
 
@@ -82,11 +82,11 @@ static void gcov_read(const char *fn)
     xc_hypercall_buffer_free(xch, p);
 }
 
-static void gcov_reset(void)
+static void cov_reset(void)
 {
     struct xen_sysctl sys;
 
-    if (gcov_sysctl(XEN_SYSCTL_GCOV_reset, &sys, NULL, 0) < 0)
+    if (cov_sysctl(XEN_SYSCTL_COV_reset, &sys, NULL, 0) < 0)
         err(1, "resetting gcov information");
 }
 
@@ -126,9 +126,9 @@ int main(int argc, char **argv)
         err(1, "opening xc interface");
 
     if (strcmp(argv[0], "reset") == 0)
-        gcov_reset();
+        cov_reset();
     else if (strcmp(argv[0], "read") == 0)
-        gcov_read(argc > 1 ? argv[1] : "-");
+        cov_read(argc > 1 ? argv[1] : "-");
     else
         usage(1);
 
diff --git a/xen/common/coverage/gcov.c b/xen/common/coverage/gcov.c
index 35653fd8d8..b1735693be 100644
--- a/xen/common/coverage/gcov.c
+++ b/xen/common/coverage/gcov.c
@@ -209,18 +209,18 @@ static int gcov_dump_all(XEN_GUEST_HANDLE_PARAM(char) buffer,
     return ret;
 }
 
-int sysctl_gcov_op(struct xen_sysctl_gcov_op *op)
+int sysctl_cov_op(struct xen_sysctl_cov_op *op)
 {
     int ret;
 
     switch ( op->cmd )
     {
-    case XEN_SYSCTL_GCOV_get_size:
+    case XEN_SYSCTL_COV_get_size:
         op->size = gcov_get_size();
         ret = 0;
         break;
 
-    case XEN_SYSCTL_GCOV_read:
+    case XEN_SYSCTL_COV_read:
     {
         XEN_GUEST_HANDLE_PARAM(char) buf;
         uint32_t size = op->size;
@@ -233,7 +233,7 @@ int sysctl_gcov_op(struct xen_sysctl_gcov_op *op)
         break;
     }
 
-    case XEN_SYSCTL_GCOV_reset:
+    case XEN_SYSCTL_COV_reset:
         gcov_reset_all_counters();
         ret = 0;
         break;
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 56def766e6..da3e1246b1 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -397,8 +397,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
     break;
 
 #ifdef CONFIG_GCOV
-    case XEN_SYSCTL_gcov_op:
-        ret = sysctl_gcov_op(&op->u.gcov_op);
+    case XEN_SYSCTL_cov_op:
+        ret = sysctl_cov_op(&op->u.cov_op);
         copyback = 1;
         break;
 #endif
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 6140f1a059..654b37cdce 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -646,11 +646,11 @@ struct xen_sysctl_scheduler_op {
 
 #define XEN_GCOV_FORMAT_MAGIC    0x58434f56 /* XCOV */
 
-#define XEN_SYSCTL_GCOV_get_size 0 /* Get total size of output data */
-#define XEN_SYSCTL_GCOV_read     1 /* Read output data */
-#define XEN_SYSCTL_GCOV_reset    2 /* Reset all counters */
+#define XEN_SYSCTL_COV_get_size 0 /* Get total size of output data */
+#define XEN_SYSCTL_COV_read     1 /* Read output data */
+#define XEN_SYSCTL_COV_reset    2 /* Reset all counters */
 
-struct xen_sysctl_gcov_op {
+struct xen_sysctl_cov_op {
     uint32_t cmd;
     uint32_t size; /* IN/OUT: size of the buffer  */
     XEN_GUEST_HANDLE_64(char) buffer; /* OUT */
@@ -1065,7 +1065,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_numainfo                      17
 #define XEN_SYSCTL_cpupool_op                    18
 #define XEN_SYSCTL_scheduler_op                  19
-#define XEN_SYSCTL_gcov_op                       20
+#define XEN_SYSCTL_cov_op                        20
 #define XEN_SYSCTL_psr_cmt_op                    21
 #define XEN_SYSCTL_pcitopoinfo                   22
 #define XEN_SYSCTL_psr_cat_op                    23
@@ -1095,7 +1095,7 @@ struct xen_sysctl {
         struct xen_sysctl_lockprof_op       lockprof_op;
         struct xen_sysctl_cpupool_op        cpupool_op;
         struct xen_sysctl_scheduler_op      scheduler_op;
-        struct xen_sysctl_gcov_op           gcov_op;
+        struct xen_sysctl_cov_op            cov_op;
         struct xen_sysctl_psr_cmt_op        psr_cmt_op;
         struct xen_sysctl_psr_cat_op        psr_cat_op;
         struct xen_sysctl_tmem_op           tmem_op;
diff --git a/xen/include/xen/coverage.h b/xen/include/xen/coverage.h
index ef22eafc1f..bdfd29d3bb 100644
--- a/xen/include/xen/coverage.h
+++ b/xen/include/xen/coverage.h
@@ -3,7 +3,7 @@
 
 #ifdef CONFIG_GCOV
 #include <public/sysctl.h>
-int sysctl_gcov_op(struct xen_sysctl_gcov_op *op);
+int sysctl_cov_op(struct xen_sysctl_cov_op *op);
 #endif
 
 #endif	/* _XEN_GCOV_H */
-- 
2.13.5 (Apple Git-94)


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

Re: [PATCH for-next 3/9] gcov: rename sysctl and functions

[Wei Liu]

On Thu, Oct 26, 2017 at 10:19:32AM +0100, Roger Pau Monne wrote:
> Change gcov to cov.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-next 3/9] gcov: rename sysctl and functions

[Jan Beulich]

>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -646,11 +646,11 @@ struct xen_sysctl_scheduler_op {
>  
>  #define XEN_GCOV_FORMAT_MAGIC    0x58434f56 /* XCOV */
>  
> -#define XEN_SYSCTL_GCOV_get_size 0 /* Get total size of output data */
> -#define XEN_SYSCTL_GCOV_read     1 /* Read output data */
> -#define XEN_SYSCTL_GCOV_reset    2 /* Reset all counters */
> +#define XEN_SYSCTL_COV_get_size 0 /* Get total size of output data */
> +#define XEN_SYSCTL_COV_read     1 /* Read output data */
> +#define XEN_SYSCTL_COV_reset    2 /* Reset all counters */
>  
> -struct xen_sysctl_gcov_op {
> +struct xen_sysctl_cov_op {
>      uint32_t cmd;
>      uint32_t size; /* IN/OUT: size of the buffer  */
>      XEN_GUEST_HANDLE_64(char) buffer; /* OUT */
> @@ -1065,7 +1065,7 @@ struct xen_sysctl {
>  #define XEN_SYSCTL_numainfo                      17
>  #define XEN_SYSCTL_cpupool_op                    18
>  #define XEN_SYSCTL_scheduler_op                  19
> -#define XEN_SYSCTL_gcov_op                       20
> +#define XEN_SYSCTL_cov_op                        20
>  #define XEN_SYSCTL_psr_cmt_op                    21
>  #define XEN_SYSCTL_pcitopoinfo                   22
>  #define XEN_SYSCTL_psr_cat_op                    23
> @@ -1095,7 +1095,7 @@ struct xen_sysctl {
>          struct xen_sysctl_lockprof_op       lockprof_op;
>          struct xen_sysctl_cpupool_op        cpupool_op;
>          struct xen_sysctl_scheduler_op      scheduler_op;
> -        struct xen_sysctl_gcov_op           gcov_op;
> +        struct xen_sysctl_cov_op            cov_op;
>          struct xen_sysctl_psr_cmt_op        psr_cmt_op;
>          struct xen_sysctl_psr_cat_op        psr_cat_op;
>          struct xen_sysctl_tmem_op           tmem_op;

While for internal things "cov" is probably fine for now, for the
public interface I think I would prefer it to be a little longer,
perhaps even fully spelled out "coverage".

Jan


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

[PATCH for-next 4/9] gcov: introduce hooks for the sysctl

[Roger Pau Monne]

So that other implementations of the sysctl can be added.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/common/coverage/gcov.c | 13 ++++++++++---
 xen/include/xen/coverage.h |  7 +++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/xen/common/coverage/gcov.c b/xen/common/coverage/gcov.c
index b1735693be..66c4075f8a 100644
--- a/xen/common/coverage/gcov.c
+++ b/xen/common/coverage/gcov.c
@@ -19,6 +19,7 @@
 #include <xen/errno.h>
 #include <xen/guest_access.h>
 #include <xen/types.h>
+#include <xen/coverage.h>
 
 #include <public/sysctl.h>
 
@@ -209,6 +210,12 @@ static int gcov_dump_all(XEN_GUEST_HANDLE_PARAM(char) buffer,
     return ret;
 }
 
+static struct cov_sysctl_ops cov_ops = {
+    .get_size = gcov_get_size,
+    .reset_counters = gcov_reset_all_counters,
+    .dump = gcov_dump_all,
+};
+
 int sysctl_cov_op(struct xen_sysctl_cov_op *op)
 {
     int ret;
@@ -216,7 +223,7 @@ int sysctl_cov_op(struct xen_sysctl_cov_op *op)
     switch ( op->cmd )
     {
     case XEN_SYSCTL_COV_get_size:
-        op->size = gcov_get_size();
+        op->size = cov_ops.get_size();
         ret = 0;
         break;
 
@@ -227,14 +234,14 @@ int sysctl_cov_op(struct xen_sysctl_cov_op *op)
 
         buf = guest_handle_cast(op->buffer, char);
 
-        ret = gcov_dump_all(buf, &size);
+        ret = cov_ops.dump(buf, &size);
         op->size = size;
 
         break;
     }
 
     case XEN_SYSCTL_COV_reset:
-        gcov_reset_all_counters();
+        cov_ops.reset_counters();
         ret = 0;
         break;
 
diff --git a/xen/include/xen/coverage.h b/xen/include/xen/coverage.h
index bdfd29d3bb..9078330109 100644
--- a/xen/include/xen/coverage.h
+++ b/xen/include/xen/coverage.h
@@ -3,6 +3,13 @@
 
 #ifdef CONFIG_GCOV
 #include <public/sysctl.h>
+
+struct cov_sysctl_ops {
+    uint32_t (*get_size)(void);
+    void     (*reset_counters)(void);
+    int      (*dump)(XEN_GUEST_HANDLE_PARAM(char), uint32_t *);
+};
+
 int sysctl_cov_op(struct xen_sysctl_cov_op *op);
 #endif
 
-- 
2.13.5 (Apple Git-94)


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

Re: [PATCH for-next 4/9] gcov: introduce hooks for the sysctl

[Andrew Cooper]

On 26/10/17 10:19, Roger Pau Monne wrote:
> @@ -209,6 +210,12 @@ static int gcov_dump_all(XEN_GUEST_HANDLE_PARAM(char) buffer,
>      return ret;
>  }
>  
> +static struct cov_sysctl_ops cov_ops = {

static const.

~Andrew

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

[PATCH for-next 5/9] coverage: introduce generic file

[Roger Pau Monne]

It will contain the generic implementation of sysctl_cov_op, which
will be shared between all the coverage implementations.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/common/coverage/Makefile   |  2 +-
 xen/common/coverage/coverage.c | 71 ++++++++++++++++++++++++++++++++++++++++++
 xen/common/coverage/gcov.c     | 41 +-----------------------
 xen/include/xen/coverage.h     |  1 +
 4 files changed, 74 insertions(+), 41 deletions(-)
 create mode 100644 xen/common/coverage/coverage.c

diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
index f68d050eca..0e0510679e 100644
--- a/xen/common/coverage/Makefile
+++ b/xen/common/coverage/Makefile
@@ -1,4 +1,4 @@
-obj-y += gcov_base.o gcov.o
+obj-y += gcov_base.o gcov.o coverage.o
 obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
 obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
 obj-$(CONFIG_GCOV_FORMAT_4_9) += gcc_4_9.o
diff --git a/xen/common/coverage/coverage.c b/xen/common/coverage/coverage.c
new file mode 100644
index 0000000000..1dec6944be
--- /dev/null
+++ b/xen/common/coverage/coverage.c
@@ -0,0 +1,71 @@
+/*
+ * Generic functionality for coverage analysis.
+ *
+ * Copyright (C) 2017 Citrix Systems R&D
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * 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, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/errno.h>
+#include <xen/guest_access.h>
+#include <xen/types.h>
+#include <xen/coverage.h>
+
+#include <public/sysctl.h>
+
+int sysctl_cov_op(struct xen_sysctl_cov_op *op)
+{
+    int ret;
+
+    switch ( op->cmd )
+    {
+    case XEN_SYSCTL_COV_get_size:
+        op->size = cov_ops.get_size();
+        ret = 0;
+        break;
+
+    case XEN_SYSCTL_COV_read:
+    {
+        XEN_GUEST_HANDLE_PARAM(char) buf;
+        uint32_t size = op->size;
+
+        buf = guest_handle_cast(op->buffer, char);
+
+        ret = cov_ops.dump(buf, &size);
+        op->size = size;
+
+        break;
+    }
+
+    case XEN_SYSCTL_COV_reset:
+        cov_ops.reset_counters();
+        ret = 0;
+        break;
+
+    default:
+        ret = -ENOSYS;
+        break;
+    }
+
+    return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/coverage/gcov.c b/xen/common/coverage/gcov.c
index 66c4075f8a..d321cbc3fc 100644
--- a/xen/common/coverage/gcov.c
+++ b/xen/common/coverage/gcov.c
@@ -21,8 +21,6 @@
 #include <xen/types.h>
 #include <xen/coverage.h>
 
-#include <public/sysctl.h>
-
 #include "gcov.h"
 
 /**
@@ -210,49 +208,12 @@ static int gcov_dump_all(XEN_GUEST_HANDLE_PARAM(char) buffer,
     return ret;
 }
 
-static struct cov_sysctl_ops cov_ops = {
+struct cov_sysctl_ops cov_ops = {
     .get_size = gcov_get_size,
     .reset_counters = gcov_reset_all_counters,
     .dump = gcov_dump_all,
 };
 
-int sysctl_cov_op(struct xen_sysctl_cov_op *op)
-{
-    int ret;
-
-    switch ( op->cmd )
-    {
-    case XEN_SYSCTL_COV_get_size:
-        op->size = cov_ops.get_size();
-        ret = 0;
-        break;
-
-    case XEN_SYSCTL_COV_read:
-    {
-        XEN_GUEST_HANDLE_PARAM(char) buf;
-        uint32_t size = op->size;
-
-        buf = guest_handle_cast(op->buffer, char);
-
-        ret = cov_ops.dump(buf, &size);
-        op->size = size;
-
-        break;
-    }
-
-    case XEN_SYSCTL_COV_reset:
-        cov_ops.reset_counters();
-        ret = 0;
-        break;
-
-    default:
-        ret = -ENOSYS;
-        break;
-    }
-
-    return ret;
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/coverage.h b/xen/include/xen/coverage.h
index 9078330109..de400620bf 100644
--- a/xen/include/xen/coverage.h
+++ b/xen/include/xen/coverage.h
@@ -9,6 +9,7 @@ struct cov_sysctl_ops {
     void     (*reset_counters)(void);
     int      (*dump)(XEN_GUEST_HANDLE_PARAM(char), uint32_t *);
 };
+extern struct cov_sysctl_ops cov_ops;
 
 int sysctl_cov_op(struct xen_sysctl_cov_op *op);
 #endif
-- 
2.13.5 (Apple Git-94)


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

Re: [PATCH for-next 5/9] coverage: introduce generic file

[Wei Liu]

On Thu, Oct 26, 2017 at 10:19:34AM +0100, Roger Pau Monne wrote:
> 
> diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
> index f68d050eca..0e0510679e 100644
> --- a/xen/common/coverage/Makefile
> +++ b/xen/common/coverage/Makefile
> @@ -1,4 +1,4 @@
> -obj-y += gcov_base.o gcov.o
> +obj-y += gcov_base.o gcov.o coverage.o
>  obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
>  obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
>  obj-$(CONFIG_GCOV_FORMAT_4_9) += gcc_4_9.o
> diff --git a/xen/common/coverage/coverage.c b/xen/common/coverage/coverage.c
> new file mode 100644
> index 0000000000..1dec6944be
> --- /dev/null
> +++ b/xen/common/coverage/coverage.c
> @@ -0,0 +1,71 @@
> +/*
> + * Generic functionality for coverage analysis.
> + *
> + * Copyright (C) 2017 Citrix Systems R&D
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * 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, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/guest_access.h>
> +#include <xen/types.h>
> +#include <xen/coverage.h>

Please sort this.

The rest appears to be pure code motion to me. It looks fine.

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

Re: [PATCH for-next 5/9] coverage: introduce generic file

[Roger Pau Monné]

On Mon, Oct 30, 2017 at 04:48:21PM +0000, Wei Liu wrote:
> On Thu, Oct 26, 2017 at 10:19:34AM +0100, Roger Pau Monne wrote:
> > 
> > diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
> > index f68d050eca..0e0510679e 100644
> > --- a/xen/common/coverage/Makefile
> > +++ b/xen/common/coverage/Makefile
> > @@ -1,4 +1,4 @@
> > -obj-y += gcov_base.o gcov.o
> > +obj-y += gcov_base.o gcov.o coverage.o
> >  obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
> >  obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
> >  obj-$(CONFIG_GCOV_FORMAT_4_9) += gcc_4_9.o
> > diff --git a/xen/common/coverage/coverage.c b/xen/common/coverage/coverage.c
> > new file mode 100644
> > index 0000000000..1dec6944be
> > --- /dev/null
> > +++ b/xen/common/coverage/coverage.c
> > @@ -0,0 +1,71 @@
> > +/*
> > + * Generic functionality for coverage analysis.
> > + *
> > + * Copyright (C) 2017 Citrix Systems R&D
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms and conditions of the GNU General Public
> > + * License, version 2, as published by the Free Software Foundation.
> > + *
> > + * 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, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <xen/errno.h>
> > +#include <xen/guest_access.h>
> > +#include <xen/types.h>
> > +#include <xen/coverage.h>
> 
> Please sort this.

OK, I will have to include type.h in coverage.h then.

Thanks, Roger.

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

Re: [PATCH for-next 5/9] coverage: introduce generic file

[Jan Beulich]

>>> On 30.10.17 at 17:57, <roger.pau@citrix.com> wrote:
> On Mon, Oct 30, 2017 at 04:48:21PM +0000, Wei Liu wrote:
>> On Thu, Oct 26, 2017 at 10:19:34AM +0100, Roger Pau Monne wrote:
>> > --- /dev/null
>> > +++ b/xen/common/coverage/coverage.c
>> > @@ -0,0 +1,71 @@
>> > +/*
>> > + * Generic functionality for coverage analysis.
>> > + *
>> > + * Copyright (C) 2017 Citrix Systems R&D
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > + * modify it under the terms and conditions of the GNU General Public
>> > + * License, version 2, as published by the Free Software Foundation.
>> > + *
>> > + * 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, see <http://www.gnu.org/licenses/>.
>> > + */
>> > +
>> > +#include <xen/errno.h>
>> > +#include <xen/guest_access.h>
>> > +#include <xen/types.h>
>> > +#include <xen/coverage.h>
>> 
>> Please sort this.
> 
> OK, I will have to include type.h in coverage.h then.

But that's not something depending on the ordering request: No
matter that we have many examples to the contrary, headers
(at least non-private ones) shouldn't really rely on other things
to have been included up front.

Jan


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

Re: [PATCH for-next 5/9] coverage: introduce generic file

[Jan Beulich]

>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
> --- a/xen/common/coverage/Makefile
> +++ b/xen/common/coverage/Makefile
> @@ -1,4 +1,4 @@
> -obj-y += gcov_base.o gcov.o
> +obj-y += gcov_base.o gcov.o coverage.o

These would better be sorted, too.

> --- a/xen/include/xen/coverage.h
> +++ b/xen/include/xen/coverage.h
> @@ -9,6 +9,7 @@ struct cov_sysctl_ops {
>      void     (*reset_counters)(void);
>      int      (*dump)(XEN_GUEST_HANDLE_PARAM(char), uint32_t *);
>  };
> +extern struct cov_sysctl_ops cov_ops;

I don't think this is the right place to put this declaration (and
perhaps also the struct one) - it's not part of the outside
visible interface of the coverage code, but an internal aspect.
Such should live in a private header, unless they need to be
exposed to e.g. arch specific code (which doesn't appear to
be the case here).

Jan


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

[PATCH for-next 6/9] kconfig: add llvm coverage option

[Roger Pau Monne]

Just add the Kconfig option and modify the makefiles so the llvm
coverage specific code can be added in a follow up patch.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/Kconfig.debug            | 16 ++++++++++++++++
 xen/Rules.mk                 |  4 ++++
 xen/common/Makefile          |  2 +-
 xen/common/coverage/Makefile |  4 ++++
 xen/common/sysctl.c          |  2 +-
 xen/include/xen/coverage.h   |  2 +-
 6 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index 8d70f63743..46c72ea8bb 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -28,10 +28,17 @@ config FRAME_POINTER
 	  maybe slower, but it gives very useful debugging information
 	  in case of any Xen bugs.
 
+# Hidden option enabled when either GCOV or LLVM coverage support is enabled.
+# This allows to enable the shared coverage bits in Xen without having to
+# check for each possible coverage implementation.
+config COVERAGE
+	bool
+
 config GCOV
 	bool "Gcov Support"
 	depends on !LIVEPATCH
 	select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
+	select COVERAGE
 	---help---
 	  Enable gcov (a test coverage program in GCC) support.
 
@@ -83,6 +90,15 @@ config GCOV_FORMAT_3_4
 
 endchoice
 
+config LLVM_COVERAGE
+	bool "LLVM coverage support"
+	depends on !LIVEPATCH && !GCOV
+	select COVERAGE
+	---help---
+	  Enable LLVM coverage support.
+
+	  If unsure, say N here.
+
 config LOCK_PROFILE
 	bool "Lock Profiling"
 	---help---
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 2659f8a4d1..076ba93185 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y)
 $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fprofile-arcs -ftest-coverage
 endif
 
+ifeq ($(CONFIG_LLVM_COVERAGE),y)
+$(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fprofile-instr-generate -fcoverage-mapping
+endif
+
 ifeq ($(CONFIG_UBSAN),y)
 $(filter-out %.init.o $(noubsan-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fsanitize=undefined
 endif
diff --git a/xen/common/Makefile b/xen/common/Makefile
index ad181636f6..3a349f478b 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -74,7 +74,7 @@ tmem-y := tmem.o tmem_xen.o tmem_control.o
 tmem-$(CONFIG_COMPAT) += compat/tmem_xen.o
 obj-$(CONFIG_TMEM) += $(tmem-y)
 
-subdir-$(CONFIG_GCOV) += coverage
+subdir-$(CONFIG_COVERAGE) += coverage
 subdir-$(CONFIG_UBSAN) += ubsan
 
 subdir-y += libelf
diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
index 0e0510679e..e4541a1233 100644
--- a/xen/common/coverage/Makefile
+++ b/xen/common/coverage/Makefile
@@ -1,3 +1,4 @@
+ifeq ($(CONFIG_GCOV),y)
 obj-y += gcov_base.o gcov.o coverage.o
 obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
 obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
@@ -9,3 +10,6 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call cc-ifversion,lt,0x040700, \
 						gcc_4_7.o, $(call cc-ifversion,lt,0x050000, \
 						gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \
 						gcc_5.o, gcc_7.o))))
+else
+obj-y += coverage.o
+endif
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index da3e1246b1..e989b2711d 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -396,7 +396,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
     }
     break;
 
-#ifdef CONFIG_GCOV
+#ifdef CONFIG_COVERAGE
     case XEN_SYSCTL_cov_op:
         ret = sysctl_cov_op(&op->u.cov_op);
         copyback = 1;
diff --git a/xen/include/xen/coverage.h b/xen/include/xen/coverage.h
index de400620bf..666bf624f9 100644
--- a/xen/include/xen/coverage.h
+++ b/xen/include/xen/coverage.h
@@ -1,7 +1,7 @@
 #ifndef _XEN_GCOV_H
 #define _XEN_GCOV_H
 
-#ifdef CONFIG_GCOV
+#ifdef CONFIG_COVERAGE
 #include <public/sysctl.h>
 
 struct cov_sysctl_ops {
-- 
2.13.5 (Apple Git-94)


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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Wei Liu]

On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote:
>  config GCOV
>  	bool "Gcov Support"
>  	depends on !LIVEPATCH

&& !LLVM_COVERAGE

(will review in detail later)

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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Roger Pau Monné]

On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote:
> On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote:
> >  config GCOV
> >  	bool "Gcov Support"
> >  	depends on !LIVEPATCH
> 
> && !LLVM_COVERAGE

That was my idea, but sadly that's not possible because you generate a
circular dependency. The best solution that I found is to only mark
one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option
below).

Thanks, Roger.

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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Wei Liu]

On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote:
> On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote:
> > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote:
> > >  config GCOV
> > >  	bool "Gcov Support"
> > >  	depends on !LIVEPATCH
> > 
> > && !LLVM_COVERAGE
> 
> That was my idea, but sadly that's not possible because you generate a
> circular dependency. The best solution that I found is to only mark
> one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option
> below).
> 

In that case, why not just use "choice" to let user pick the
implementation?

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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Jan Beulich]

>>> On 26.10.17 at 12:10, <wei.liu2@citrix.com> wrote:
> On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote:
>> On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote:
>> > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote:
>> > >  config GCOV
>> > >  	bool "Gcov Support"
>> > >  	depends on !LIVEPATCH
>> > 
>> > && !LLVM_COVERAGE
>> 
>> That was my idea, but sadly that's not possible because you generate a
>> circular dependency. The best solution that I found is to only mark
>> one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option
>> below).
> 
> In that case, why not just use "choice" to let user pick the
> implementation?

Plus then this choice should probably have an auto-detect option
just like gcov's gcc version one - at least I assume that it should
be pretty straightforward to tell at build time which variant to use.
In fact - what would happen if one enabled the wrong option for
the compiler in use? IOW I wonder whether auto-detection (and
then also for the gcc version) should be the only valid mode).

Jan

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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Roger Pau Monné]

On Wed, Nov 08, 2017 at 01:13:29AM -0700, Jan Beulich wrote:
> >>> On 26.10.17 at 12:10, <wei.liu2@citrix.com> wrote:
> > On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote:
> >> On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote:
> >> > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote:
> >> > >  config GCOV
> >> > >  	bool "Gcov Support"
> >> > >  	depends on !LIVEPATCH
> >> > 
> >> > && !LLVM_COVERAGE
> >> 
> >> That was my idea, but sadly that's not possible because you generate a
> >> circular dependency. The best solution that I found is to only mark
> >> one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option
> >> below).
> > 
> > In that case, why not just use "choice" to let user pick the
> > implementation?
> 
> Plus then this choice should probably have an auto-detect option
> just like gcov's gcc version one - at least I assume that it should
> be pretty straightforward to tell at build time which variant to use.
> In fact - what would happen if one enabled the wrong option for
> the compiler in use? IOW I wonder whether auto-detection (and
> then also for the gcc version) should be the only valid mode).

I don't plan to introduce llvm/clang version choices here, I think
autodetection should be fine.

I can remove the gcc ones also, leaving this as a boolean choice (ie:
enable code coverage support), but I would like to have some
confirmation from the gcc side.

Thanks, Roger.

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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Jan Beulich]

>>> On 08.11.17 at 09:49, <roger.pau@citrix.com> wrote:
> On Wed, Nov 08, 2017 at 01:13:29AM -0700, Jan Beulich wrote:
>> >>> On 26.10.17 at 12:10, <wei.liu2@citrix.com> wrote:
>> > On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote:
>> >> On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote:
>> >> > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote:
>> >> > >  config GCOV
>> >> > >  	bool "Gcov Support"
>> >> > >  	depends on !LIVEPATCH
>> >> > 
>> >> > && !LLVM_COVERAGE
>> >> 
>> >> That was my idea, but sadly that's not possible because you generate a
>> >> circular dependency. The best solution that I found is to only mark
>> >> one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option
>> >> below).
>> > 
>> > In that case, why not just use "choice" to let user pick the
>> > implementation?
>> 
>> Plus then this choice should probably have an auto-detect option
>> just like gcov's gcc version one - at least I assume that it should
>> be pretty straightforward to tell at build time which variant to use.
>> In fact - what would happen if one enabled the wrong option for
>> the compiler in use? IOW I wonder whether auto-detection (and
>> then also for the gcc version) should be the only valid mode).
> 
> I don't plan to introduce llvm/clang version choices here, I think
> autodetection should be fine.

And I didn't think about version choices for clang here anyway -
the point really was just about gcc vs clang.

> I can remove the gcc ones also, leaving this as a boolean choice (ie:
> enable code coverage support), but I would like to have some
> confirmation from the gcc side.

So let's ask Wei: What was the reason for the Kconfig level
version choice here in the first place? After all, if the wrong one
is being selected, the build will fail afaict due to the #error
directives in the version specific files.

Jan

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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Wei Liu]

On Wed, Nov 08, 2017 at 04:07:35AM -0700, Jan Beulich wrote:
> >>> On 08.11.17 at 09:49, <roger.pau@citrix.com> wrote:
> > On Wed, Nov 08, 2017 at 01:13:29AM -0700, Jan Beulich wrote:
> >> >>> On 26.10.17 at 12:10, <wei.liu2@citrix.com> wrote:
> >> > On Thu, Oct 26, 2017 at 11:08:21AM +0100, Roger Pau Monné wrote:
> >> >> On Thu, Oct 26, 2017 at 11:03:13AM +0100, Wei Liu wrote:
> >> >> > On Thu, Oct 26, 2017 at 10:19:35AM +0100, Roger Pau Monne wrote:
> >> >> > >  config GCOV
> >> >> > >  	bool "Gcov Support"
> >> >> > >  	depends on !LIVEPATCH
> >> >> > 
> >> >> > && !LLVM_COVERAGE
> >> >> 
> >> >> That was my idea, but sadly that's not possible because you generate a
> >> >> circular dependency. The best solution that I found is to only mark
> >> >> one as exclusive (ie: have depends !GCOV in the LLVM_COVERAGE option
> >> >> below).
> >> > 
> >> > In that case, why not just use "choice" to let user pick the
> >> > implementation?
> >> 
> >> Plus then this choice should probably have an auto-detect option
> >> just like gcov's gcc version one - at least I assume that it should
> >> be pretty straightforward to tell at build time which variant to use.
> >> In fact - what would happen if one enabled the wrong option for
> >> the compiler in use? IOW I wonder whether auto-detection (and
> >> then also for the gcc version) should be the only valid mode).
> > 
> > I don't plan to introduce llvm/clang version choices here, I think
> > autodetection should be fine.
> 
> And I didn't think about version choices for clang here anyway -
> the point really was just about gcc vs clang.
> 
> > I can remove the gcc ones also, leaving this as a boolean choice (ie:
> > enable code coverage support), but I would like to have some
> > confirmation from the gcc side.
> 
> So let's ask Wei: What was the reason for the Kconfig level
> version choice here in the first place? After all, if the wrong one
> is being selected, the build will fail afaict due to the #error
> directives in the version specific files.
> 

The #error on versions was added in later iterations IIRC. Looking back
I think it would make sense to just have autodetect.

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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Andrew Cooper]

On 26/10/17 10:19, Roger Pau Monne wrote:
> diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
> index 0e0510679e..e4541a1233 100644
> --- a/xen/common/coverage/Makefile
> +++ b/xen/common/coverage/Makefile
> @@ -1,3 +1,4 @@
> +ifeq ($(CONFIG_GCOV),y)
>  obj-y += gcov_base.o gcov.o coverage.o
>  obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
>  obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
> @@ -9,3 +10,6 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call cc-ifversion,lt,0x040700, \
>  						gcc_4_7.o, $(call cc-ifversion,lt,0x050000, \
>  						gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \
>  						gcc_5.o, gcc_7.o))))
> +else
> +obj-y += coverage.o
> +endif

How about

obj-y += coverage.o

ifeq ($(CONFIG_GCOV),y)
...
endif

seeing as coverage.o is common now?

~Andrew

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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Jan Beulich]

>>> On 26.10.17 at 12:24, <andrew.cooper3@citrix.com> wrote:
> On 26/10/17 10:19, Roger Pau Monne wrote:
>> diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
>> index 0e0510679e..e4541a1233 100644
>> --- a/xen/common/coverage/Makefile
>> +++ b/xen/common/coverage/Makefile
>> @@ -1,3 +1,4 @@
>> +ifeq ($(CONFIG_GCOV),y)
>>  obj-y += gcov_base.o gcov.o coverage.o
>>  obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o
>>  obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o
>> @@ -9,3 +10,6 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call 
> cc-ifversion,lt,0x040700, \
>>  						gcc_4_7.o, $(call cc-ifversion,lt,0x050000, \
>>  						gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \
>>  						gcc_5.o, gcc_7.o))))
>> +else
>> +obj-y += coverage.o
>> +endif
> 
> How about
> 
> obj-y += coverage.o
> 
> ifeq ($(CONFIG_GCOV),y)
> ...
> endif

Except please obj-$(CONFIG_GCOV) instead of ifeq.

Jan


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

Re: [PATCH for-next 6/9] kconfig: add llvm coverage option

[Jan Beulich]

>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -28,10 +28,17 @@ config FRAME_POINTER
>  	  maybe slower, but it gives very useful debugging information
>  	  in case of any Xen bugs.
>  
> +# Hidden option enabled when either GCOV or LLVM coverage support is enabled.
> +# This allows to enable the shared coverage bits in Xen without having to
> +# check for each possible coverage implementation.
> +config COVERAGE
> +	bool

In case this is still needed after the conversion to "choice", the
comment should be dropped - it is the very purpose of
prompt-less options that is being described there.

> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -119,6 +119,10 @@ ifeq ($(CONFIG_GCOV),y)
>  $(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fprofile-arcs -ftest-coverage
>  endif
>  
> +ifeq ($(CONFIG_LLVM_COVERAGE),y)
> +$(filter-out %.init.o $(nogcov-y),$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += -fprofile-instr-generate -fcoverage-mapping
> +endif

The use of nogcov-y here suggests that the earlier patch abstracting
away the "g" should be extended to remove that letter here, too.

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -396,7 +396,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>      }
>      break;
>  
> -#ifdef CONFIG_GCOV
> +#ifdef CONFIG_COVERAGE
>      case XEN_SYSCTL_cov_op:
>          ret = sysctl_cov_op(&op->u.cov_op);
>          copyback = 1;

Couldn't you better do away with the #ifdef altogether, by
introducing a suitable inline function for the coverage-disabled
case (returning -EOPNOTSUPP)?

Jan


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

[PATCH for-next 7/9] coverage: introduce support for llvm profiling

[Roger Pau Monne]

Introduce the functionality in order to fill the hooks of the
cov_sysctl_ops struct.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: llvm-dev@lists.llvm.org
---
Note that the file that contains the helpers is under a BSD 2-clause
license. This is done so it can be shared with other OSes that use the
llvm/clang compiler.
---
 xen/common/coverage/Makefile |   2 +-
 xen/common/coverage/llvm.c   | 148 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/public/sysctl.h  |   6 ++
 3 files changed, 155 insertions(+), 1 deletion(-)
 create mode 100644 xen/common/coverage/llvm.c

diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
index e4541a1233..f2ffb2b8de 100644
--- a/xen/common/coverage/Makefile
+++ b/xen/common/coverage/Makefile
@@ -11,5 +11,5 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call cc-ifversion,lt,0x040700, \
 						gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \
 						gcc_5.o, gcc_7.o))))
 else
-obj-y += coverage.o
+obj-y += llvm.o coverage.o
 endif
diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c
new file mode 100644
index 0000000000..c8905070a3
--- /dev/null
+++ b/xen/common/coverage/llvm.c
@@ -0,0 +1,148 @@
+/*
+ * Copyright (C) 2017 Citrix Systems R&D
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <xen/errno.h>
+#include <xen/guest_access.h>
+#include <xen/types.h>
+#include <xen/coverage.h>
+
+#ifndef __clang__
+#error "LLVM coverage selected without clang compiler"
+#endif
+
+#define LLVM_PROFILE_MAGIC_64 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
+       (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
+        (uint64_t)'f' << 16 | (uint64_t)'r' << 8 | (uint64_t)129
+#define LLVM_PROFILE_MAGIC_32 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
+       (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
+        (uint64_t)'f' << 16 | (uint64_t)'R' << 8 | (uint64_t)129
+
+#if __clang_major__ >= 4 || (__clang_major__ == 3 && __clang_minor__ == 9)
+#define LLVM_PROFILE_VERSION    4
+#define LLVM_PROFILE_NUM_KINDS  2
+#else
+#error "clang version not supported with coverage"
+#endif
+
+struct llvm_profile_data {
+    uint64_t name_ref;
+    uint64_t function_hash;
+    void *counter;
+    void *function;
+    void *values;
+    uint32_t nr_counters;
+    uint16_t nr_value_sites[LLVM_PROFILE_NUM_KINDS];
+};
+
+struct llvm_profile_header {
+    uint64_t magic;
+    uint64_t version;
+    uint64_t data_size;
+    uint64_t counters_size;
+    uint64_t names_size;
+    uint64_t counters_delta;
+    uint64_t names_delta;
+    uint64_t value_kind_last;
+};
+
+int __llvm_profile_runtime;
+
+extern struct llvm_profile_data __start___llvm_prf_data;
+extern struct llvm_profile_data __stop___llvm_prf_data;
+extern uint64_t __start___llvm_prf_cnts;
+extern uint64_t __stop___llvm_prf_cnts;
+extern char __start___llvm_prf_names;
+extern char __stop___llvm_prf_names;
+
+static void *start_data = &__start___llvm_prf_data;
+static void *end_data = &__stop___llvm_prf_data;
+static void *start_counters = &__start___llvm_prf_cnts;
+static void *end_counters = &__stop___llvm_prf_cnts;
+static void *start_names = &__start___llvm_prf_names;
+static void *end_names = &__stop___llvm_prf_names;
+
+static void reset_counters(void)
+{
+    memset(start_counters, 0, end_counters - start_counters);
+}
+
+static uint32_t get_size(void)
+{
+    return ROUNDUP(sizeof(struct llvm_profile_header) + end_data - start_data +
+                   end_counters - start_counters + end_names - start_names, 8);
+}
+
+static int dump(XEN_GUEST_HANDLE_PARAM(char) buffer, uint32_t *buf_size)
+{
+    struct llvm_profile_header header = {
+#if BITS_PER_LONG == 64
+        .magic = LLVM_PROFILE_MAGIC_64,
+#else
+        .magic = LLVM_PROFILE_MAGIC_32,
+#endif
+        .version = LLVM_PROFILE_VERSION,
+        .data_size = (end_data - start_data) / sizeof(struct llvm_profile_data),
+        .counters_size = (end_counters - start_counters) / sizeof(uint64_t),
+        .names_size = end_names - start_names,
+        .counters_delta = (uintptr_t)start_counters,
+        .names_delta = (uintptr_t)start_names,
+        .value_kind_last = LLVM_PROFILE_NUM_KINDS - 1,
+    };
+    unsigned int off = 0;
+
+#define APPEND_TO_BUFFER(src, size)                     \
+({                                                      \
+    if ( off + size > *buf_size )                       \
+        return -ENOMEM;                                 \
+    copy_to_guest_offset(buffer, off, src, size);       \
+    off += size;                                        \
+})
+    APPEND_TO_BUFFER((char *)&header, sizeof(struct llvm_profile_header));
+    APPEND_TO_BUFFER((char *)start_data, end_data - start_data);
+    APPEND_TO_BUFFER((char *)start_counters, end_counters - start_counters);
+    APPEND_TO_BUFFER((char *)start_names, end_names - start_names);
+#undef APPEND_TO_BUFFER
+
+    clear_guest_offset(buffer, off, *buf_size - off);
+
+    return 0;
+}
+
+struct cov_sysctl_ops cov_ops = {
+    .get_size = get_size,
+    .reset_counters = reset_counters,
+    .dump = dump,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 654b37cdce..62824b18f2 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -646,6 +646,12 @@ struct xen_sysctl_scheduler_op {
 
 #define XEN_GCOV_FORMAT_MAGIC    0x58434f56 /* XCOV */
 
+/*
+ * Ouput format of LLVM coverage data is just a raw stream, as would be
+ * written by the compiler_rt run time library into a .profraw file. There
+ * are no special Xen tags or delimiters because none are needed.
+ */
+
 #define XEN_SYSCTL_COV_get_size 0 /* Get total size of output data */
 #define XEN_SYSCTL_COV_read     1 /* Read output data */
 #define XEN_SYSCTL_COV_reset    2 /* Reset all counters */
-- 
2.13.5 (Apple Git-94)


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

[PATCH for-next 8/9] xsm: add bodge when compiling with llvm coverage support

[Roger Pau Monne]

llvm coverage support seems to disable some of the optimizations
needed in order to compile xsm, and the end result is that references
to __xsm_action_mismatch_detected are left in the object files.

Since coverage support cannot be used in production, introduce
__xsm_action_mismatch_detected for llvm coverage builds.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/include/xsm/dummy.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index b2cd56cdc5..674dc8ea1b 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -24,8 +24,22 @@
  * if references remain at link time.
  */
 #define LINKER_BUG_ON(x) do { if (x) __xsm_action_mismatch_detected(); } while (0)
+
+#ifdef CONFIG_LLVM_COVERAGE
+/*
+ * LLVM coverage support seems to disable some of the optimizations needed in
+ * order for XSM to compile. Since coverage should not be used in production
+ * provide an implementation of __xsm_action_mismatch_detected to satisfy the
+ * linker.
+ */
+static void __xsm_action_mismatch_detected(void)
+{
+    ASSERT_UNREACHABLE();
+}
+#else
 /* DO NOT implement this function; it is supposed to trigger link errors */
 void __xsm_action_mismatch_detected(void);
+#endif
 
 #ifdef CONFIG_XSM
 
-- 
2.13.5 (Apple Git-94)


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

Re: [PATCH for-next 8/9] xsm: add bodge when compiling with llvm coverage support

[Daniel De Graaf]

On 10/26/2017 05:19 AM, Roger Pau Monne wrote:
> llvm coverage support seems to disable some of the optimizations
> needed in order to compile xsm, and the end result is that references
> to __xsm_action_mismatch_detected are left in the object files.
> 
> Since coverage support cannot be used in production, introduce
> __xsm_action_mismatch_detected for llvm coverage builds.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

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

Re: [PATCH for-next 8/9] xsm: add bodge when compiling with llvm coverage support

[Jan Beulich]

>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -24,8 +24,22 @@
>   * if references remain at link time.
>   */
>  #define LINKER_BUG_ON(x) do { if (x) __xsm_action_mismatch_detected(); } while (0)
> +
> +#ifdef CONFIG_LLVM_COVERAGE
> +/*
> + * LLVM coverage support seems to disable some of the optimizations needed in
> + * order for XSM to compile. Since coverage should not be used in production
> + * provide an implementation of __xsm_action_mismatch_detected to satisfy the
> + * linker.
> + */
> +static void __xsm_action_mismatch_detected(void)
> +{
> +    ASSERT_UNREACHABLE();
> +}

I'm pretty sure this wants to be "inline".

Jan


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

[PATCH for-next 9/9] coverage: add documentation for LLVM coverage

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 docs/misc/coverage.markdown | 47 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/docs/misc/coverage.markdown b/docs/misc/coverage.markdown
index 0a32c48f4b..565644631a 100644
--- a/docs/misc/coverage.markdown
+++ b/docs/misc/coverage.markdown
@@ -8,6 +8,8 @@ information. Every basic block in the code will be instrumented by the compiler
 to compute these statistics. It should not be used in production as it slows
 down your hypervisor.
 
+# GCOV (GCC coverage)
+
 ## Enable coverage
 
 Test coverage support can be turned on compiling Xen with the `coverage` option set
@@ -87,3 +89,48 @@ blob extracted from xencov!**
 * See output in a browser
 
         firefox cov/index.html
+
+# LLVM coverage
+
+## Enable coverage
+
+Coverage can be enabled using a Kconfig option, from the top-level directory
+use the following command to display the Kconfig menu:
+
+    gmake -C xen menuconfig clang=y
+
+The LLVM coverage option can be found inside of the "Debugging Options"
+section. After enabling it just compile Xen as you would normally do:
+
+   gmake xen clang=y
+
+## Extract coverage data
+
+LLVM coverage can be extracted from the hypervisor using the `xencov` tool.
+The following actions are available:
+
+* `xencov read` extract data
+* `xencov reset` reset all coverage counters
+* `xencov read-reset` extract data and reset counters at the same time.
+
+## Possible use
+
+**This section is just an example on how to use these tools!**
+
+This example assumes you compiled Xen and copied the xen-syms file from
+xen/xen-syms into your current directory.
+
+* Extract the coverage data from Xen:
+
+    xencov read xen.profraw
+
+* Convert the data into a profile. Note that you can merge more than one
+  profraw file into a single profdata file.
+
+    llvm-profdata merge xen.profraw -o xen.profdata
+
+* Generate a HTML report of the code coverage:
+
+    llvm-cov show -format=html -output-dir=cov/ xen-syms -instr-profile=xen.profdata
+
+* Open cov/index.html with your browser in order to display the profile.
-- 
2.13.5 (Apple Git-94)


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

Re: [PATCH for-next 9/9] coverage: add documentation for LLVM coverage

[Wei Liu]

On Thu, Oct 26, 2017 at 10:19:38AM +0100, Roger Pau Monne wrote:
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  docs/misc/coverage.markdown | 47 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/docs/misc/coverage.markdown b/docs/misc/coverage.markdown
> index 0a32c48f4b..565644631a 100644
> --- a/docs/misc/coverage.markdown
> +++ b/docs/misc/coverage.markdown
> @@ -8,6 +8,8 @@ information. Every basic block in the code will be instrumented by the compiler
>  to compute these statistics. It should not be used in production as it slows
>  down your hypervisor.
>  
> +# GCOV (GCC coverage)
> +
>  ## Enable coverage
>  
>  Test coverage support can be turned on compiling Xen with the `coverage` option set
> @@ -87,3 +89,48 @@ blob extracted from xencov!**
>  * See output in a browser
>  
>          firefox cov/index.html
> +
> +# LLVM coverage
> +
> +## Enable coverage
> +
> +Coverage can be enabled using a Kconfig option, from the top-level directory
> +use the following command to display the Kconfig menu:
> +
> +    gmake -C xen menuconfig clang=y
> +
> +The LLVM coverage option can be found inside of the "Debugging Options"
> +section. After enabling it just compile Xen as you would normally do:
> +
> +   gmake xen clang=y
> +

It can be a bit confusing when make and gmake appear in the same
document. I suggest you use make all over and add a footnote saying it
should be gmake on FreeBSD.

This footnote should apply to both gcov and llvm-cov -- I believe you
should be able to build gcov support on FreeBSD, too.

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

Re: [PATCH for-next 9/9] coverage: add documentation for LLVM coverage

[Roger Pau Monné]

On Mon, Oct 30, 2017 at 04:48:09PM +0000, Wei Liu wrote:
> On Thu, Oct 26, 2017 at 10:19:38AM +0100, Roger Pau Monne wrote:
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  docs/misc/coverage.markdown | 47 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/docs/misc/coverage.markdown b/docs/misc/coverage.markdown
> > index 0a32c48f4b..565644631a 100644
> > --- a/docs/misc/coverage.markdown
> > +++ b/docs/misc/coverage.markdown
> > @@ -8,6 +8,8 @@ information. Every basic block in the code will be instrumented by the compiler
> >  to compute these statistics. It should not be used in production as it slows
> >  down your hypervisor.
> >  
> > +# GCOV (GCC coverage)
> > +
> >  ## Enable coverage
> >  
> >  Test coverage support can be turned on compiling Xen with the `coverage` option set
> > @@ -87,3 +89,48 @@ blob extracted from xencov!**
> >  * See output in a browser
> >  
> >          firefox cov/index.html
> > +
> > +# LLVM coverage
> > +
> > +## Enable coverage
> > +
> > +Coverage can be enabled using a Kconfig option, from the top-level directory
> > +use the following command to display the Kconfig menu:
> > +
> > +    gmake -C xen menuconfig clang=y
> > +
> > +The LLVM coverage option can be found inside of the "Debugging Options"
> > +section. After enabling it just compile Xen as you would normally do:
> > +
> > +   gmake xen clang=y
> > +
> 
> It can be a bit confusing when make and gmake appear in the same
> document. I suggest you use make all over and add a footnote saying it
> should be gmake on FreeBSD.

This is my fault from copying the runes from my command line. I'm not
sure a footnote is even needed, everything on BSDs should use gmake
instead of make, and I don't plan to edit all the documentation files.

Thanks, Roger.

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

Re: [PATCH for-next 7/9] coverage: introduce support for llvm profiling

[Wei Liu]

On Thu, Oct 26, 2017 at 10:19:36AM +0100, Roger Pau Monne wrote:
> Introduce the functionality in order to fill the hooks of the
> cov_sysctl_ops struct.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: llvm-dev@lists.llvm.org
> ---
> Note that the file that contains the helpers is under a BSD 2-clause
> license. This is done so it can be shared with other OSes that use the
> llvm/clang compiler.

It would be helpful if you can put a reference to the original code
somewhere, either in commit message or in the comment section of the
file.

> ---
>  xen/common/coverage/Makefile |   2 +-
>  xen/common/coverage/llvm.c   | 148 +++++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/sysctl.h  |   6 ++
>  3 files changed, 155 insertions(+), 1 deletion(-)
>  create mode 100644 xen/common/coverage/llvm.c
> 
> diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
> index e4541a1233..f2ffb2b8de 100644
> --- a/xen/common/coverage/Makefile
> +++ b/xen/common/coverage/Makefile
> @@ -11,5 +11,5 @@ obj-$(CONFIG_GCOV_FORMAT_AUTODETECT) += $(call cc-ifversion,lt,0x040700, \
>  						gcc_4_9.o, $(call cc-ifversion,lt,0x070000, \
>  						gcc_5.o, gcc_7.o))))
>  else
> -obj-y += coverage.o
> +obj-y += llvm.o coverage.o
>  endif
> diff --git a/xen/common/coverage/llvm.c b/xen/common/coverage/llvm.c
> new file mode 100644
> index 0000000000..c8905070a3
> --- /dev/null
> +++ b/xen/common/coverage/llvm.c
> +#include <xen/errno.h>
> +#include <xen/guest_access.h>
> +#include <xen/types.h>
> +#include <xen/coverage.h>
> +

Order.

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

Re: [PATCH for-next 7/9] coverage: introduce support for llvm profiling

[Jan Beulich]

>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
> --- /dev/null
> +++ b/xen/common/coverage/llvm.c
> @@ -0,0 +1,148 @@
> +/*
> + * Copyright (C) 2017 Citrix Systems R&D
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/guest_access.h>
> +#include <xen/types.h>
> +#include <xen/coverage.h>
> +
> +#ifndef __clang__
> +#error "LLVM coverage selected without clang compiler"
> +#endif
> +
> +#define LLVM_PROFILE_MAGIC_64 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
> +       (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
> +        (uint64_t)'f' << 16 | (uint64_t)'r' << 8 | (uint64_t)129
> +#define LLVM_PROFILE_MAGIC_32 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
> +       (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
> +        (uint64_t)'f' << 16 | (uint64_t)'R' << 8 | (uint64_t)129

Judging by the file having Xen style, I imply this isn't intended to
very closely match some other original. With that, parentheses
should be added around all the shift operations above.

> +#if __clang_major__ >= 4 || (__clang_major__ == 3 && __clang_minor__ == 9)

Is it certain that there's never going to be a 3.10? Otherwise
>= might be more suitable for the minor version check.

> +int __llvm_profile_runtime;

This isn't used anywhere - please add a brief comment explaining
the need for it (the more that its type being plain int is also
suspicious).

> +extern struct llvm_profile_data __start___llvm_prf_data;
> +extern struct llvm_profile_data __stop___llvm_prf_data;
> +extern uint64_t __start___llvm_prf_cnts;
> +extern uint64_t __stop___llvm_prf_cnts;
> +extern char __start___llvm_prf_names;
> +extern char __stop___llvm_prf_names;

I guess all of these really want to have [] added, making ...

> +static void *start_data = &__start___llvm_prf_data;
> +static void *end_data = &__stop___llvm_prf_data;
> +static void *start_counters = &__start___llvm_prf_cnts;
> +static void *end_counters = &__stop___llvm_prf_cnts;
> +static void *start_names = &__start___llvm_prf_names;
> +static void *end_names = &__stop___llvm_prf_names;

... the &s here unnecessary. But then - do these really need to
be statics (rather than #define-s)?

I would also guess that at least the names ones could be const.

> +static int dump(XEN_GUEST_HANDLE_PARAM(char) buffer, uint32_t *buf_size)
> +{
> +    struct llvm_profile_header header = {
> +#if BITS_PER_LONG == 64
> +        .magic = LLVM_PROFILE_MAGIC_64,
> +#else
> +        .magic = LLVM_PROFILE_MAGIC_32,
> +#endif

I think this should just be LLVM_PROFILE_MAGIC, with the #ifdef
moved to the #define-s.

> +        .version = LLVM_PROFILE_VERSION,
> +        .data_size = (end_data - start_data) / sizeof(struct llvm_profile_data),
> +        .counters_size = (end_counters - start_counters) / sizeof(uint64_t),
> +        .names_size = end_names - start_names,
> +        .counters_delta = (uintptr_t)start_counters,
> +        .names_delta = (uintptr_t)start_names,
> +        .value_kind_last = LLVM_PROFILE_NUM_KINDS - 1,
> +    };
> +    unsigned int off = 0;
> +
> +#define APPEND_TO_BUFFER(src, size)                     \
> +({                                                      \
> +    if ( off + size > *buf_size )                       \

(size)

> +        return -ENOMEM;                                 \
> +    copy_to_guest_offset(buffer, off, src, size);       \
> +    off += size;                                        \

Ideally here too, albeit only the comma operator has lower
precedence, and that would require parenthesizing in the macro
invocation already (which is also why the arguments to
copy_to_guest_offset() explicitly do not need extra parentheses
added).

> +})
> +    APPEND_TO_BUFFER((char *)&header, sizeof(struct llvm_profile_header));

sizeof(header)

> +    APPEND_TO_BUFFER((char *)start_data, end_data - start_data);
> +    APPEND_TO_BUFFER((char *)start_counters, end_counters - start_counters);
> +    APPEND_TO_BUFFER((char *)start_names, end_names - start_names);

The casts should all be to const char*, and perhaps that would
better be done inside the macro anyway.

> +#undef APPEND_TO_BUFFER
> +
> +    clear_guest_offset(buffer, off, *buf_size - off);
> +
> +    return 0;
> +}
> +
> +struct cov_sysctl_ops cov_ops = {

const

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -646,6 +646,12 @@ struct xen_sysctl_scheduler_op {
>  
>  #define XEN_GCOV_FORMAT_MAGIC    0x58434f56 /* XCOV */

Hmm, shouldn't the private magic #define-s actually be put here
(in which case you'd indeed need to retain both 32- and 64-bit
variants)?

Jan

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

Re: [PATCH for-next 7/9] coverage: introduce support for llvm profiling

[Roger Pau Monné]

On Wed, Nov 08, 2017 at 01:38:59AM -0700, Jan Beulich wrote:
> >>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
> > --- /dev/null
> > +++ b/xen/common/coverage/llvm.c
> > +#define LLVM_PROFILE_MAGIC_64 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
> > +       (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
> > +        (uint64_t)'f' << 16 | (uint64_t)'r' << 8 | (uint64_t)129
> > +#define LLVM_PROFILE_MAGIC_32 (uint64_t)255 << 56 | (uint64_t)'l' << 48 | \
> > +       (uint64_t)'p' << 40 | (uint64_t)'r' << 32 | (uint64_t)'o' << 24 |  \
> > +        (uint64_t)'f' << 16 | (uint64_t)'R' << 8 | (uint64_t)129
> 
> Judging by the file having Xen style, I imply this isn't intended to
> very closely match some other original. With that, parentheses
> should be added around all the shift operations above.
> 
> > +#if __clang_major__ >= 4 || (__clang_major__ == 3 && __clang_minor__ == 9)
> 
> Is it certain that there's never going to be a 3.10? Otherwise
> >= might be more suitable for the minor version check.

No, there's not going to be a clang 3.10.

> > +int __llvm_profile_runtime;
> 
> This isn't used anywhere - please add a brief comment explaining
> the need for it (the more that its type being plain int is also
> suspicious).

This is an internal symbol that must be present because Xen is not
linked against the run-time coverage library. It's described as an int
here:

https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#using-the-profiling-runtime-without-static-initializers

Without this symbol linkage fails.

> > +extern struct llvm_profile_data __start___llvm_prf_data;
> > +extern struct llvm_profile_data __stop___llvm_prf_data;
> > +extern uint64_t __start___llvm_prf_cnts;
> > +extern uint64_t __stop___llvm_prf_cnts;
> > +extern char __start___llvm_prf_names;
> > +extern char __stop___llvm_prf_names;
> 
> I guess all of these really want to have [] added, making ...
> 
> > +static void *start_data = &__start___llvm_prf_data;
> > +static void *end_data = &__stop___llvm_prf_data;
> > +static void *start_counters = &__start___llvm_prf_cnts;
> > +static void *end_counters = &__stop___llvm_prf_cnts;
> > +static void *start_names = &__start___llvm_prf_names;
> > +static void *end_names = &__stop___llvm_prf_names;
> 
> ... the &s here unnecessary. But then - do these really need to
> be statics (rather than #define-s)?
> 
> I would also guess that at least the names ones could be const.

Could make them defines indeed.

> > +    APPEND_TO_BUFFER((char *)start_data, end_data - start_data);
> > +    APPEND_TO_BUFFER((char *)start_counters, end_counters - start_counters);
> > +    APPEND_TO_BUFFER((char *)start_names, end_names - start_names);
> 
> The casts should all be to const char*, and perhaps that would
> better be done inside the macro anyway.

Seems better indeed.

> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -646,6 +646,12 @@ struct xen_sysctl_scheduler_op {
> >  
> >  #define XEN_GCOV_FORMAT_MAGIC    0x58434f56 /* XCOV */
> 
> Hmm, shouldn't the private magic #define-s actually be put here
> (in which case you'd indeed need to retain both 32- and 64-bit
> variants)?

I don't think so, here XEN_GCOV_FORMAT_MAGIC is a Xen specific gcov
magic number.

OTOH LLVM_PROFILE_MAGIC_{64/32} is an llvm defined magic number,
that's not under our control. Hence I don't think it should be
exported in Xen public headers.

Roger.

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

Re: [PATCH for-next 7/9] coverage: introduce support for llvm profiling

[Jan Beulich]

>>> On 08.11.17 at 09:56, <roger.pau@citrix.com> wrote:
> On Wed, Nov 08, 2017 at 01:38:59AM -0700, Jan Beulich wrote:
>> >>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
>> > --- a/xen/include/public/sysctl.h
>> > +++ b/xen/include/public/sysctl.h
>> > @@ -646,6 +646,12 @@ struct xen_sysctl_scheduler_op {
>> >  
>> >  #define XEN_GCOV_FORMAT_MAGIC    0x58434f56 /* XCOV */
>> 
>> Hmm, shouldn't the private magic #define-s actually be put here
>> (in which case you'd indeed need to retain both 32- and 64-bit
>> variants)?
> 
> I don't think so, here XEN_GCOV_FORMAT_MAGIC is a Xen specific gcov
> magic number.
> 
> OTOH LLVM_PROFILE_MAGIC_{64/32} is an llvm defined magic number,
> that's not under our control. Hence I don't think it should be
> exported in Xen public headers.

Okay.

Jan


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

Re: [llvm-dev] [PATCH for-next 7/9] coverage: introduce support for llvm profiling

[Vedant Kumar]

- llvm-dev, since the code review can continue without that audience.

vedant

> On Nov 8, 2017, at 3:08 AM, Jan Beulich via llvm-dev <llvm-dev@lists.llvm.org> wrote:
> 
>>>> On 08.11.17 at 09:56, <roger.pau@citrix.com> wrote:
>> On Wed, Nov 08, 2017 at 01:38:59AM -0700, Jan Beulich wrote:
>>>>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> wrote:
>>>> --- a/xen/include/public/sysctl.h
>>>> +++ b/xen/include/public/sysctl.h
>>>> @@ -646,6 +646,12 @@ struct xen_sysctl_scheduler_op {
>>>> 
>>>> #define XEN_GCOV_FORMAT_MAGIC    0x58434f56 /* XCOV */
>>> 
>>> Hmm, shouldn't the private magic #define-s actually be put here
>>> (in which case you'd indeed need to retain both 32- and 64-bit
>>> variants)?
>> 
>> I don't think so, here XEN_GCOV_FORMAT_MAGIC is a Xen specific gcov
>> magic number.
>> 
>> OTOH LLVM_PROFILE_MAGIC_{64/32} is an llvm defined magic number,
>> that's not under our control. Hence I don't think it should be
>> exported in Xen public headers.
> 
> Okay.
> 
> Jan
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


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