| Message ID | 20160902163952.16600-1-tamas.lengyel@zentific.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/09/16 17:39, Tamas K Lengyel wrote: > While debugging applications built on top of libxc with Valgrind we get a lot > of complaining about relying on uninitialized values allocated in libxc. > While these warnings are safe to ignore, zero-initializing the structures > reduces Valgrind clutter a lot and aids in spotting real bugs. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > --- > tools/libxc/xc_private.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h > index 75b761c..4e9073b 100644 > --- a/tools/libxc/xc_private.h > +++ b/tools/libxc/xc_private.h > @@ -59,11 +59,11 @@ struct iovec { > #include <sys/uio.h> > #endif > > -#define DECLARE_DOMCTL struct xen_domctl domctl > -#define DECLARE_SYSCTL struct xen_sysctl sysctl > -#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op > -#define DECLARE_FLASK_OP struct xen_flask_op op > -#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op > +#define DECLARE_DOMCTL struct xen_domctl domctl = {0} > +#define DECLARE_SYSCTL struct xen_sysctl sysctl = {0} > +#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op = {0} > +#define DECLARE_FLASK_OP struct xen_flask_op op = {0} > +#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op = {0} I specifically took those out in the past, because it hides real problems from Valgrind. Instead, I would recommend removing these wrappers entirely. They serve no useful purpose. Taking a random example of xc_get_pfn_type_batch(), it would be rather more efficient to write ... DECLARE_HYPERCALL_BOUNCE(arr, sizeof(*arr) * num, XC_HYPERCALL_BUFFER_BOUNCE_BOTH); struct xen_domctl domctl = { .cmd = XEN_DOMCTL_getpageframeinfo3, .domain = dom, .u.getpageframeinfo3.num = num, }; ... as it permits the compiler more freedom in how xen_domctl gets constructed, as well as being able to plainly see exactly what is done to the memory. ~Andrew
On Fri, Sep 2, 2016 at 11:18 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 02/09/16 17:39, Tamas K Lengyel wrote: >> While debugging applications built on top of libxc with Valgrind we get a lot >> of complaining about relying on uninitialized values allocated in libxc. >> While these warnings are safe to ignore, zero-initializing the structures >> reduces Valgrind clutter a lot and aids in spotting real bugs. >> >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> >> --- >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Wei Liu <wei.liu2@citrix.com> >> --- >> tools/libxc/xc_private.h | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h >> index 75b761c..4e9073b 100644 >> --- a/tools/libxc/xc_private.h >> +++ b/tools/libxc/xc_private.h >> @@ -59,11 +59,11 @@ struct iovec { >> #include <sys/uio.h> >> #endif >> >> -#define DECLARE_DOMCTL struct xen_domctl domctl >> -#define DECLARE_SYSCTL struct xen_sysctl sysctl >> -#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op >> -#define DECLARE_FLASK_OP struct xen_flask_op op >> -#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op >> +#define DECLARE_DOMCTL struct xen_domctl domctl = {0} >> +#define DECLARE_SYSCTL struct xen_sysctl sysctl = {0} >> +#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op = {0} >> +#define DECLARE_FLASK_OP struct xen_flask_op op = {0} >> +#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op = {0} > > I specifically took those out in the past, because it hides real > problems from Valgrind. > > Instead, I would recommend removing these wrappers entirely. They serve > no useful purpose. > > Taking a random example of xc_get_pfn_type_batch(), it would be rather > more efficient to write > > ... > DECLARE_HYPERCALL_BOUNCE(arr, sizeof(*arr) * num, > XC_HYPERCALL_BUFFER_BOUNCE_BOTH); > struct xen_domctl domctl = { > .cmd = XEN_DOMCTL_getpageframeinfo3, > .domain = dom, > .u.getpageframeinfo3.num = num, > }; > ... > > as it permits the compiler more freedom in how xen_domctl gets > constructed, as well as being able to plainly see exactly what is done > to the memory. > Yea I don't really see much point using these macros as they are either and the one you propose certainly would make more sense. Tamas
On Fri, Sep 02, 2016 at 11:22:04AM -0600, Tamas K Lengyel wrote: > On Fri, Sep 2, 2016 at 11:18 AM, Andrew Cooper > <andrew.cooper3@citrix.com> wrote: > > On 02/09/16 17:39, Tamas K Lengyel wrote: > >> While debugging applications built on top of libxc with Valgrind we get a lot > >> of complaining about relying on uninitialized values allocated in libxc. > >> While these warnings are safe to ignore, zero-initializing the structures > >> reduces Valgrind clutter a lot and aids in spotting real bugs. > >> > >> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> > >> --- > >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> Cc: Wei Liu <wei.liu2@citrix.com> > >> --- > >> tools/libxc/xc_private.h | 10 +++++----- > >> 1 file changed, 5 insertions(+), 5 deletions(-) > >> > >> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h > >> index 75b761c..4e9073b 100644 > >> --- a/tools/libxc/xc_private.h > >> +++ b/tools/libxc/xc_private.h > >> @@ -59,11 +59,11 @@ struct iovec { > >> #include <sys/uio.h> > >> #endif > >> > >> -#define DECLARE_DOMCTL struct xen_domctl domctl > >> -#define DECLARE_SYSCTL struct xen_sysctl sysctl > >> -#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op > >> -#define DECLARE_FLASK_OP struct xen_flask_op op > >> -#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op > >> +#define DECLARE_DOMCTL struct xen_domctl domctl = {0} > >> +#define DECLARE_SYSCTL struct xen_sysctl sysctl = {0} > >> +#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op = {0} > >> +#define DECLARE_FLASK_OP struct xen_flask_op op = {0} > >> +#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op = {0} > > > > I specifically took those out in the past, because it hides real > > problems from Valgrind. > > > > Instead, I would recommend removing these wrappers entirely. They serve > > no useful purpose. > > > > Taking a random example of xc_get_pfn_type_batch(), it would be rather > > more efficient to write > > > > ... > > DECLARE_HYPERCALL_BOUNCE(arr, sizeof(*arr) * num, > > XC_HYPERCALL_BUFFER_BOUNCE_BOTH); > > struct xen_domctl domctl = { > > .cmd = XEN_DOMCTL_getpageframeinfo3, > > .domain = dom, > > .u.getpageframeinfo3.num = num, > > }; > > ... > > > > as it permits the compiler more freedom in how xen_domctl gets > > constructed, as well as being able to plainly see exactly what is done > > to the memory. > > > > Yea I don't really see much point using these macros as they are > either and the one you propose certainly would make more sense. > One reason I can think of why we would want those macros is that we don't want to change all locations when the code fragment changes. But I don't see how code segment is going to change for the macros under discussion. All in all, I don't object to eliminating those macros. Ian? Wei. > Tamas
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h index 75b761c..4e9073b 100644 --- a/tools/libxc/xc_private.h +++ b/tools/libxc/xc_private.h @@ -59,11 +59,11 @@ struct iovec { #include <sys/uio.h> #endif -#define DECLARE_DOMCTL struct xen_domctl domctl -#define DECLARE_SYSCTL struct xen_sysctl sysctl -#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op -#define DECLARE_FLASK_OP struct xen_flask_op op -#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op +#define DECLARE_DOMCTL struct xen_domctl domctl = {0} +#define DECLARE_SYSCTL struct xen_sysctl sysctl = {0} +#define DECLARE_PHYSDEV_OP struct physdev_op physdev_op = {0} +#define DECLARE_FLASK_OP struct xen_flask_op op = {0} +#define DECLARE_PLATFORM_OP struct xen_platform_op platform_op = {0} #undef PAGE_SHIFT #undef PAGE_SIZE
While debugging applications built on top of libxc with Valgrind we get a lot of complaining about relying on uninitialized values allocated in libxc. While these warnings are safe to ignore, zero-initializing the structures reduces Valgrind clutter a lot and aids in spotting real bugs. Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- tools/libxc/xc_private.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)