Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Thu, Oct 13, 2016 at 03:15:09AM -0600, Jan Beulich wrote: > >>> On 13.10.16 at 10:49, wrote: > > On Thu, Oct 13, 2016 at 02:29:08AM -0600, Jan Beulich wrote: > > [...] > >> >> >> ... this structure's trailing fields actually getting used by the > >> >> >> code > >> >> >> won't work well when changing compiler versions without cleaning > >> >> >> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c > >> >> >> #define-ing their GCOV_COUNTERS and then #include-ing this > >> >> >> shared source file. Plus btw, I don't think gcc 5.0.x (the > >> >> >> development variant of 5.x) would use anything different from > >> >> >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not > >> >> >> normally be necessary anymore with gcc 5+. > >> >> >> > >> >> > > >> >> > I think you misread here: __GNUC_MINOR__ is the "x" part of 5.x.y, the > >> >> > "y" part is __GNUC_PATCHLEVEL__. > >> >> > >> >> No, I didn't. From 5.x onwards the information previously carried in > >> >> __GNUC_PATCHLEVEL__ is now in __GNUC_MINOR__. And as much > >> >> as previously you would not normally need to look at the former, > >> >> with newer gcc you shouldn't need to look at the latter. > >> >> > >> > > >> > I can't find relevant information in GCC cpp manual. > >> > > >> > Specifically, I look at 4.9.4 and 5.4.0 doc: > >> > > >> > > > https://gcc.gnu.org/onlinedocs/gcc-4.9.4/cpp/Common-Predefined-Macros.html#Comm > > > > > >> > on-Predefined-Macros > >> > > > https://gcc.gnu.org/onlinedocs/gcc-5.4.0/cpp/Common-Predefined-Macros.html#Comm > > > > > >> > on-Predefined-Macros > >> > > >> > The sections about __GNUC_* macros are identical, their semantics stay > >> > the same. > >> > > >> > What did I miss? > >> > >> Their change in how version numbers get used. I'm sure you've noticed > >> there never was a released 5.0.0 or 6.0.0, and that the stable updates > >> following 5.1.0 were 5.2.0, 5.3.0, etc. > >> > > > > OK. I found the bits at https://gcc.gnu.org/develop.html. I see what you > > meant previously. > > > > It doesn't seem to be a problem to me to compare to 5.1 though -- that's > > the first release of gcc 5, which should be what people use anyway. > > But your check should cover the introduction point of the feature, > which is 5.0.0 imo. > > > If it is the complexity of the macro that concerns you, now it has been > > changed to use GCC_VERSION macro in gcov.h, which is a lot simpler to > > reason about. Are you happy with such arrangement? > > If you mean this to be an adjustment newer than v3, then I think > I'd be fine with that, provided you cover the full range (as indicated > above), i.e. starting at 5.0.0. > OK. The check is now like: #if GCC_VERSION < 5 #error "Wrong version of GCC used to compile gcov" #endif GCC_VERSION is a convenient marco that you can find in this patch. Then all reference to gcc 5.1 will be changed to gcc 5. Wei. > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
>>> On 13.10.16 at 10:49, wrote: > On Thu, Oct 13, 2016 at 02:29:08AM -0600, Jan Beulich wrote: > [...] >> >> >> ... this structure's trailing fields actually getting used by the code >> >> >> won't work well when changing compiler versions without cleaning >> >> >> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c >> >> >> #define-ing their GCOV_COUNTERS and then #include-ing this >> >> >> shared source file. Plus btw, I don't think gcc 5.0.x (the >> >> >> development variant of 5.x) would use anything different from >> >> >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not >> >> >> normally be necessary anymore with gcc 5+. >> >> >> >> >> > >> >> > I think you misread here: __GNUC_MINOR__ is the "x" part of 5.x.y, the >> >> > "y" part is __GNUC_PATCHLEVEL__. >> >> >> >> No, I didn't. From 5.x onwards the information previously carried in >> >> __GNUC_PATCHLEVEL__ is now in __GNUC_MINOR__. And as much >> >> as previously you would not normally need to look at the former, >> >> with newer gcc you shouldn't need to look at the latter. >> >> >> > >> > I can't find relevant information in GCC cpp manual. >> > >> > Specifically, I look at 4.9.4 and 5.4.0 doc: >> > >> > > https://gcc.gnu.org/onlinedocs/gcc-4.9.4/cpp/Common-Predefined-Macros.html#Comm > > >> > on-Predefined-Macros >> > > https://gcc.gnu.org/onlinedocs/gcc-5.4.0/cpp/Common-Predefined-Macros.html#Comm > > >> > on-Predefined-Macros >> > >> > The sections about __GNUC_* macros are identical, their semantics stay >> > the same. >> > >> > What did I miss? >> >> Their change in how version numbers get used. I'm sure you've noticed >> there never was a released 5.0.0 or 6.0.0, and that the stable updates >> following 5.1.0 were 5.2.0, 5.3.0, etc. >> > > OK. I found the bits at https://gcc.gnu.org/develop.html. I see what you > meant previously. > > It doesn't seem to be a problem to me to compare to 5.1 though -- that's > the first release of gcc 5, which should be what people use anyway. But your check should cover the introduction point of the feature, which is 5.0.0 imo. > If it is the complexity of the macro that concerns you, now it has been > changed to use GCC_VERSION macro in gcov.h, which is a lot simpler to > reason about. Are you happy with such arrangement? If you mean this to be an adjustment newer than v3, then I think I'd be fine with that, provided you cover the full range (as indicated above), i.e. starting at 5.0.0. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Thu, Oct 13, 2016 at 02:29:08AM -0600, Jan Beulich wrote: [...] > >> >> ... this structure's trailing fields actually getting used by the code > >> >> won't work well when changing compiler versions without cleaning > >> >> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c > >> >> #define-ing their GCOV_COUNTERS and then #include-ing this > >> >> shared source file. Plus btw, I don't think gcc 5.0.x (the > >> >> development variant of 5.x) would use anything different from > >> >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not > >> >> normally be necessary anymore with gcc 5+. > >> >> > >> > > >> > I think you misread here: __GNUC_MINOR__ is the "x" part of 5.x.y, the > >> > "y" part is __GNUC_PATCHLEVEL__. > >> > >> No, I didn't. From 5.x onwards the information previously carried in > >> __GNUC_PATCHLEVEL__ is now in __GNUC_MINOR__. And as much > >> as previously you would not normally need to look at the former, > >> with newer gcc you shouldn't need to look at the latter. > >> > > > > I can't find relevant information in GCC cpp manual. > > > > Specifically, I look at 4.9.4 and 5.4.0 doc: > > > > https://gcc.gnu.org/onlinedocs/gcc-4.9.4/cpp/Common-Predefined-Macros.html#Comm > > > > on-Predefined-Macros > > https://gcc.gnu.org/onlinedocs/gcc-5.4.0/cpp/Common-Predefined-Macros.html#Comm > > > > on-Predefined-Macros > > > > The sections about __GNUC_* macros are identical, their semantics stay > > the same. > > > > What did I miss? > > Their change in how version numbers get used. I'm sure you've noticed > there never was a released 5.0.0 or 6.0.0, and that the stable updates > following 5.1.0 were 5.2.0, 5.3.0, etc. > OK. I found the bits at https://gcc.gnu.org/develop.html. I see what you meant previously. It doesn't seem to be a problem to me to compare to 5.1 though -- that's the first release of gcc 5, which should be what people use anyway. If it is the complexity of the macro that concerns you, now it has been changed to use GCC_VERSION macro in gcov.h, which is a lot simpler to reason about. Are you happy with such arrangement? If you feel strongly about this version comparison thing, I'm fine with just comparing it to the major number, too. Wei. > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
>>> On 12.10.16 at 19:07, wrote: > On Wed, Oct 12, 2016 at 09:42:17AM -0600, Jan Beulich wrote: >> >>> On 12.10.16 at 17:33, wrote: >> > On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: >> >> >>> On 11.10.16 at 12:31, wrote: >> >> > --- /dev/null >> >> > +++ b/xen/common/gcov/gcc_4_7.c >> >> > @@ -0,0 +1,205 @@ >> >> > +/* >> >> > + * This code provides functions to handle gcc's profiling data format >> >> > + * introduced with gcc 4.7. >> >> > + * >> >> > + * This file is based heavily on gcc_3_4.c file. >> >> > + * >> >> > + * For a better understanding, refer to gcc source: >> >> > + * gcc/gcov-io.h >> >> > + * libgcc/libgcov.c >> >> > + * >> >> > + * Uses gcc-internal data definitions. >> >> > + * >> >> > + * Imported from Linux and modified for Xen by >> >> > + *Wei Liu >> >> > + */ >> >> > + >> >> > +#include >> >> > + >> >> > +#include "gcov.h" >> >> > + >> >> > +#if GCC_VERSION < 40700 >> >> > +#error "Wrong version of GCC used to compile gcov" >> >> > +#endif >> >> > + >> >> > +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) >> >> > +#define GCOV_COUNTERS 10 >> >> > +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 >> >> > +#define GCOV_COUNTERS 9 >> >> > +#else >> >> > +#define GCOV_COUNTERS 8 >> >> > +#endif >> >> >> >> I'm sorry for not having pointed this out on v2 (I had noticed it, >> >> but then didn't finish analyzing the situation), but I'm afraid this >> >> together with ... >> >> >> >> > +struct gcov_info { >> >> > +unsigned int version; >> >> > +struct gcov_info *next; >> >> > +unsigned int stamp; >> >> > +const char *filename; >> >> > +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); >> >> > +unsigned int n_functions; >> >> > +struct gcov_fn_info **functions; >> >> > +}; >> >> >> >> ... this structure's trailing fields actually getting used by the code >> >> won't work well when changing compiler versions without cleaning >> >> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c >> >> #define-ing their GCOV_COUNTERS and then #include-ing this >> >> shared source file. Plus btw, I don't think gcc 5.0.x (the >> >> development variant of 5.x) would use anything different from >> >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not >> >> normally be necessary anymore with gcc 5+. >> >> >> > >> > I think you misread here: __GNUC_MINOR__ is the "x" part of 5.x.y, the >> > "y" part is __GNUC_PATCHLEVEL__. >> >> No, I didn't. From 5.x onwards the information previously carried in >> __GNUC_PATCHLEVEL__ is now in __GNUC_MINOR__. And as much >> as previously you would not normally need to look at the former, >> with newer gcc you shouldn't need to look at the latter. >> > > I can't find relevant information in GCC cpp manual. > > Specifically, I look at 4.9.4 and 5.4.0 doc: > > https://gcc.gnu.org/onlinedocs/gcc-4.9.4/cpp/Common-Predefined-Macros.html#Comm > > on-Predefined-Macros > https://gcc.gnu.org/onlinedocs/gcc-5.4.0/cpp/Common-Predefined-Macros.html#Comm > > on-Predefined-Macros > > The sections about __GNUC_* macros are identical, their semantics stay > the same. > > What did I miss? Their change in how version numbers get used. I'm sure you've noticed there never was a released 5.0.0 or 6.0.0, and that the stable updates following 5.1.0 were 5.2.0, 5.3.0, etc. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On 12.10.2016 18:21, Konrad Rzeszutek Wilk wrote: On Wed, Oct 12, 2016 at 04:17:57PM +0200, Martin Pohlack wrote: On 12.10.2016 15:44, Konrad Rzeszutek Wilk wrote: On Wed, Oct 12, 2016 at 07:31:52AM -0600, Jan Beulich wrote: On 12.10.16 at 15:23, wrote: And then - how is all of this supposed to be working in conjucntion with live patching, where the patch may have been created by yet another compiler version? Uh, I hope one does not create a livepatch patch with another compiler version! Let me put on the TODO to make livepatch-build-tools check gcc against compile.h so that one does not try this. What's wrong with mixing compiler versions in general? Besides scaring me? The one issue we had encountered was with compilers generating random named symbols for the switch tables. Those end up being called "CSWTCH.XYZ" where the XYZ depends on the position of the moon along with how many goats you have sacrificied to the altar of GCC gods. Older compilers don't neccessarily do it, newer ones do, and every time you build an livepatch the naming is different. Frustrating. It maybe that newer versions of GCC are more predictable about this naming. Maybe Martin can share some of his experience? CC-ing him. There are a couple of naming conventions for internal symbols and also static symbols where you basically have to pray that gcc implementation does not change. Interestingly, icc has some conventions that make those symbol names a bit more stable. The tricky thing is matchmaking between the existing build and the new build to construct the binary diff and to match up symbols for which you want to provide replacement code. We use a reproducible build environment to construct hotpatches for an existing build in the absolutely same environment (gcc version, lib versions, gas version, binutils etc.). This sidesteps most of the problems. I think the matchmaking process does not solve per say some tricky CSWTCH issues. If a patch mucks with a switch statement (e.g. add a new case) we are pretty much guaranteed to get in trouble. And really a change in any control structure may cause gcc to take different code path, causing it to renumber CSWTCH. Or worst, renumber it to the one that the hypervisor is using for some other switch statements. I think the size of the symbol vs the one in the hypervisor is different so one can check for this. Bad things happen if it is the same size, but bcmp can come in handy there. If you change a switch statement, the containing function's binary representation will change (+ inlining effects). This means you will have to ship a new version of this function with the hotpatch. I have seen gcc put jump tables for switch statements into dedicated .rodata* sections, at least with -ffunction-sections and -fdata-sections. You need to treat those as belonging to the corresponding function and also ship them with the hotpatch. If you restrict the match-making to function-level symbols and retain references between such a function and its rodata section, you should be fine. Are there any ways to make GCC be predictable or some patches to make GCC be less random. Perhaps instead of XYZ it would use the function name.. You sidestep some issues by making source-code patches as line-neutral as possible and introducing new symbols and definitions close to usage instead of in header files for hotpatches. This reduces cascading effects for such renames. Some luck, tweaking, and inlining of definitions is sometimes required. Martin P.S. GCC scares me because the code comes in these big patches with not much explanation on how it suppose to work. It probably is a piece of cake for folks who have been marinating in compilers but for a newbie like me it is hardcore black magic. Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 09:42:17AM -0600, Jan Beulich wrote: > >>> On 12.10.16 at 17:33, wrote: > > On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: > >> >>> On 11.10.16 at 12:31, wrote: > >> > --- /dev/null > >> > +++ b/xen/common/gcov/gcc_4_7.c > >> > @@ -0,0 +1,205 @@ > >> > +/* > >> > + * This code provides functions to handle gcc's profiling data format > >> > + * introduced with gcc 4.7. > >> > + * > >> > + * This file is based heavily on gcc_3_4.c file. > >> > + * > >> > + * For a better understanding, refer to gcc source: > >> > + * gcc/gcov-io.h > >> > + * libgcc/libgcov.c > >> > + * > >> > + * Uses gcc-internal data definitions. > >> > + * > >> > + * Imported from Linux and modified for Xen by > >> > + *Wei Liu > >> > + */ > >> > + > >> > +#include > >> > + > >> > +#include "gcov.h" > >> > + > >> > +#if GCC_VERSION < 40700 > >> > +#error "Wrong version of GCC used to compile gcov" > >> > +#endif > >> > + > >> > +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) > >> > +#define GCOV_COUNTERS 10 > >> > +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 > >> > +#define GCOV_COUNTERS 9 > >> > +#else > >> > +#define GCOV_COUNTERS 8 > >> > +#endif > >> > >> I'm sorry for not having pointed this out on v2 (I had noticed it, > >> but then didn't finish analyzing the situation), but I'm afraid this > >> together with ... > >> > >> > +struct gcov_info { > >> > +unsigned int version; > >> > +struct gcov_info *next; > >> > +unsigned int stamp; > >> > +const char *filename; > >> > +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); > >> > +unsigned int n_functions; > >> > +struct gcov_fn_info **functions; > >> > +}; > >> > >> ... this structure's trailing fields actually getting used by the code > >> won't work well when changing compiler versions without cleaning > >> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c > >> #define-ing their GCOV_COUNTERS and then #include-ing this > >> shared source file. Plus btw, I don't think gcc 5.0.x (the > >> development variant of 5.x) would use anything different from > >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not > >> normally be necessary anymore with gcc 5+. > >> > > > > I think you misread here: __GNUC_MINOR__ is the "x" part of 5.x.y, the > > "y" part is __GNUC_PATCHLEVEL__. > > No, I didn't. From 5.x onwards the information previously carried in > __GNUC_PATCHLEVEL__ is now in __GNUC_MINOR__. And as much > as previously you would not normally need to look at the former, > with newer gcc you shouldn't need to look at the latter. > I can't find relevant information in GCC cpp manual. Specifically, I look at 4.9.4 and 5.4.0 doc: https://gcc.gnu.org/onlinedocs/gcc-4.9.4/cpp/Common-Predefined-Macros.html#Common-Predefined-Macros https://gcc.gnu.org/onlinedocs/gcc-5.4.0/cpp/Common-Predefined-Macros.html#Common-Predefined-Macros The sections about __GNUC_* macros are identical, their semantics stay the same. What did I miss? > > I've broken down things into several files as well as provided > > corresponding Kconfig options: > > > > gcc_4_7_base.c: the body of what is now gcc_4_7.c, better name is > > welcome > > Why don't you keep it gcc_4_7.c, with its counter definition being > conditional upon the symbol not already being defined? > Fixed as discussed on IRC. Wei. > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 04:17:57PM +0200, Martin Pohlack wrote: > On 12.10.2016 15:44, Konrad Rzeszutek Wilk wrote: > > On Wed, Oct 12, 2016 at 07:31:52AM -0600, Jan Beulich wrote: > > > > > > On 12.10.16 at 15:23, wrote: > > > > > And then - how is all of this supposed to be working in conjucntion > > > > > with live patching, where the patch may have been created by yet > > > > > another compiler version? > > > > > > > > Uh, I hope one does not create a livepatch patch with another compiler > > > > version! > > > > > > > > Let me put on the TODO to make livepatch-build-tools check gcc against > > > > compile.h so that one does not try this. > > > > > > What's wrong with mixing compiler versions in general? > > > > Besides scaring me? > > > > The one issue we had encountered was with compilers generating random named > > symbols for the switch tables. Those end up being called "CSWTCH.XYZ" > > where the XYZ depends on the position of the moon along with how many > > goats you have sacrificied to the altar of GCC gods. > > > > Older compilers don't neccessarily do it, newer ones do, and every time > > you build an livepatch the naming is different. Frustrating. > > > > It maybe that newer versions of GCC are more predictable about this > > naming. > > > > Maybe Martin can share some of his experience? CC-ing him. > > There are a couple of naming conventions for internal symbols and also > static symbols where you basically have to pray that gcc implementation does > not change. Interestingly, icc has some conventions that make those symbol > names a bit more stable. > > The tricky thing is matchmaking between the existing build and the new build > to construct the binary diff and to match up symbols for which you want to > provide replacement code. > > We use a reproducible build environment to construct hotpatches for an > existing build in the absolutely same environment (gcc version, lib > versions, gas version, binutils etc.). This sidesteps most of the problems. I think the matchmaking process does not solve per say some tricky CSWTCH issues. If a patch mucks with a switch statement (e.g. add a new case) we are pretty much guaranteed to get in trouble. And really a change in any control structure may cause gcc to take different code path, causing it to renumber CSWTCH. Or worst, renumber it to the one that the hypervisor is using for some other switch statements. I think the size of the symbol vs the one in the hypervisor is different so one can check for this. Bad things happen if it is the same size, but bcmp can come in handy there. Are there any ways to make GCC be predictable or some patches to make GCC be less random. Perhaps instead of XYZ it would use the function name.. P.S. GCC scares me because the code comes in these big patches with not much explanation on how it suppose to work. It probably is a piece of cake for folks who have been marinating in compilers but for a newbie like me it is hardcore black magic. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
>>> On 12.10.16 at 17:33, wrote: > On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: >> >>> On 11.10.16 at 12:31, wrote: >> > --- /dev/null >> > +++ b/xen/common/gcov/gcc_4_7.c >> > @@ -0,0 +1,205 @@ >> > +/* >> > + * This code provides functions to handle gcc's profiling data format >> > + * introduced with gcc 4.7. >> > + * >> > + * This file is based heavily on gcc_3_4.c file. >> > + * >> > + * For a better understanding, refer to gcc source: >> > + * gcc/gcov-io.h >> > + * libgcc/libgcov.c >> > + * >> > + * Uses gcc-internal data definitions. >> > + * >> > + * Imported from Linux and modified for Xen by >> > + *Wei Liu >> > + */ >> > + >> > +#include >> > + >> > +#include "gcov.h" >> > + >> > +#if GCC_VERSION < 40700 >> > +#error "Wrong version of GCC used to compile gcov" >> > +#endif >> > + >> > +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) >> > +#define GCOV_COUNTERS 10 >> > +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 >> > +#define GCOV_COUNTERS 9 >> > +#else >> > +#define GCOV_COUNTERS 8 >> > +#endif >> >> I'm sorry for not having pointed this out on v2 (I had noticed it, >> but then didn't finish analyzing the situation), but I'm afraid this >> together with ... >> >> > +struct gcov_info { >> > +unsigned int version; >> > +struct gcov_info *next; >> > +unsigned int stamp; >> > +const char *filename; >> > +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); >> > +unsigned int n_functions; >> > +struct gcov_fn_info **functions; >> > +}; >> >> ... this structure's trailing fields actually getting used by the code >> won't work well when changing compiler versions without cleaning >> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c >> #define-ing their GCOV_COUNTERS and then #include-ing this >> shared source file. Plus btw, I don't think gcc 5.0.x (the >> development variant of 5.x) would use anything different from >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not >> normally be necessary anymore with gcc 5+. >> > > I think you misread here: __GNUC_MINOR__ is the "x" part of 5.x.y, the > "y" part is __GNUC_PATCHLEVEL__. No, I didn't. From 5.x onwards the information previously carried in __GNUC_PATCHLEVEL__ is now in __GNUC_MINOR__. And as much as previously you would not normally need to look at the former, with newer gcc you shouldn't need to look at the latter. > I've broken down things into several files as well as provided > corresponding Kconfig options: > > gcc_4_7_base.c: the body of what is now gcc_4_7.c, better name is > welcome Why don't you keep it gcc_4_7.c, with its counter definition being conditional upon the symbol not already being defined? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: > >>> On 11.10.16 at 12:31, wrote: > > --- /dev/null > > +++ b/xen/common/gcov/gcc_4_7.c > > @@ -0,0 +1,205 @@ > > +/* > > + * This code provides functions to handle gcc's profiling data format > > + * introduced with gcc 4.7. > > + * > > + * This file is based heavily on gcc_3_4.c file. > > + * > > + * For a better understanding, refer to gcc source: > > + * gcc/gcov-io.h > > + * libgcc/libgcov.c > > + * > > + * Uses gcc-internal data definitions. > > + * > > + * Imported from Linux and modified for Xen by > > + *Wei Liu > > + */ > > + > > +#include > > + > > +#include "gcov.h" > > + > > +#if GCC_VERSION < 40700 > > +#error "Wrong version of GCC used to compile gcov" > > +#endif > > + > > +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) > > +#define GCOV_COUNTERS 10 > > +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 > > +#define GCOV_COUNTERS 9 > > +#else > > +#define GCOV_COUNTERS 8 > > +#endif > > I'm sorry for not having pointed this out on v2 (I had noticed it, > but then didn't finish analyzing the situation), but I'm afraid this > together with ... > > > +struct gcov_info { > > +unsigned int version; > > +struct gcov_info *next; > > +unsigned int stamp; > > +const char *filename; > > +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); > > +unsigned int n_functions; > > +struct gcov_fn_info **functions; > > +}; > > ... this structure's trailing fields actually getting used by the code > won't work well when changing compiler versions without cleaning > the tree. I think instead you need thin gcc_5.c and gcc_4_9.c > #define-ing their GCOV_COUNTERS and then #include-ing this > shared source file. Plus btw, I don't think gcc 5.0.x (the > development variant of 5.x) would use anything different from > 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not > normally be necessary anymore with gcc 5+. > I think you misread here: __GNUC_MINOR__ is the "x" part of 5.x.y, the "y" part is __GNUC_PATCHLEVEL__. I've broken down things into several files as well as provided corresponding Kconfig options: gcc_4_7_base.c: the body of what is now gcc_4_7.c, better name is welcome gcc_4_7.c: gcov format in gcc version [4.7, 4,9), nr counters 8 gcc_4_9.c: gcov format in gcc version [4.9, 5.1), nr counters 9 gcc_5_1.c: gcov format in gcc version [5.1, ...), nr counters 10 Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On 12.10.2016 15:44, Konrad Rzeszutek Wilk wrote: On Wed, Oct 12, 2016 at 07:31:52AM -0600, Jan Beulich wrote: On 12.10.16 at 15:23, wrote: And then - how is all of this supposed to be working in conjucntion with live patching, where the patch may have been created by yet another compiler version? Uh, I hope one does not create a livepatch patch with another compiler version! Let me put on the TODO to make livepatch-build-tools check gcc against compile.h so that one does not try this. What's wrong with mixing compiler versions in general? Besides scaring me? The one issue we had encountered was with compilers generating random named symbols for the switch tables. Those end up being called "CSWTCH.XYZ" where the XYZ depends on the position of the moon along with how many goats you have sacrificied to the altar of GCC gods. Older compilers don't neccessarily do it, newer ones do, and every time you build an livepatch the naming is different. Frustrating. It maybe that newer versions of GCC are more predictable about this naming. Maybe Martin can share some of his experience? CC-ing him. There are a couple of naming conventions for internal symbols and also static symbols where you basically have to pray that gcc implementation does not change. Interestingly, icc has some conventions that make those symbol names a bit more stable. The tricky thing is matchmaking between the existing build and the new build to construct the binary diff and to match up symbols for which you want to provide replacement code. We use a reproducible build environment to construct hotpatches for an existing build in the absolutely same environment (gcc version, lib versions, gas version, binutils etc.). This sidesteps most of the problems. Martin Amazon Development Center Germany GmbH Berlin - Dresden - Aachen main office: Krausenstr. 38, 10117 Berlin Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger Ust-ID: DE289237879 Eingetragen am Amtsgericht Charlottenburg HRB 149173 B ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
>>> On 12.10.16 at 15:44, wrote: > On Wed, Oct 12, 2016 at 07:31:52AM -0600, Jan Beulich wrote: >> >>> On 12.10.16 at 15:23, wrote: >> >> And then - how is all of this supposed to be working in conjucntion >> >> with live patching, where the patch may have been created by yet >> >> another compiler version? >> > >> > Uh, I hope one does not create a livepatch patch with another compiler >> > version! >> > >> > Let me put on the TODO to make livepatch-build-tools check gcc against >> > compile.h so that one does not try this. >> >> What's wrong with mixing compiler versions in general? > > Besides scaring me? What is it that scares you? > The one issue we had encountered was with compilers generating random named > symbols for the switch tables. Those end up being called "CSWTCH.XYZ" > where the XYZ depends on the position of the moon along with how many > goats you have sacrificied to the altar of GCC gods. > > Older compilers don't neccessarily do it, newer ones do, and every time > you build an livepatch the naming is different. Frustrating. But this would mean you don't just depend on gcc version, but even on the specific build (as the numbering you refer to may change with whatever patches a distro applies on top of the upstream tarball, as well as perhaps with configure and build options). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On 12/10/16 14:34, Andrew Cooper wrote: > On 12/10/16 14:26, George Dunlap wrote: >> On 12/10/16 14:24, George Dunlap wrote: >>> On 12/10/16 14:06, Wei Liu wrote: On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: On 11.10.16 at 12:31, wrote: >> --- /dev/null >> +++ b/xen/common/gcov/gcc_4_7.c >> @@ -0,0 +1,205 @@ >> +/* >> + * This code provides functions to handle gcc's profiling data format >> + * introduced with gcc 4.7. >> + * >> + * This file is based heavily on gcc_3_4.c file. >> + * >> + * For a better understanding, refer to gcc source: >> + * gcc/gcov-io.h >> + * libgcc/libgcov.c >> + * >> + * Uses gcc-internal data definitions. >> + * >> + * Imported from Linux and modified for Xen by >> + *Wei Liu >> + */ >> + >> +#include >> + >> +#include "gcov.h" >> + >> +#if GCC_VERSION < 40700 >> +#error "Wrong version of GCC used to compile gcov" >> +#endif >> + >> +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) >> +#define GCOV_COUNTERS 10 >> +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 >> +#define GCOV_COUNTERS 9 >> +#else >> +#define GCOV_COUNTERS 8 >> +#endif > I'm sorry for not having pointed this out on v2 (I had noticed it, > but then didn't finish analyzing the situation), but I'm afraid this > together with ... > >> +struct gcov_info { >> +unsigned int version; >> +struct gcov_info *next; >> +unsigned int stamp; >> +const char *filename; >> +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); >> +unsigned int n_functions; >> +struct gcov_fn_info **functions; >> +}; > ... this structure's trailing fields actually getting used by the code > won't work well when changing compiler versions without cleaning > the tree. I think instead you need thin gcc_5.c and gcc_4_9.c > #define-ing their GCOV_COUNTERS and then #include-ing this > shared source file. Plus btw, I don't think gcc 5.0.x (the > development variant of 5.x) would use anything different from > 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not > normally be necessary anymore with gcc 5+. > Right. I will do something about this. Thanks for catching this. > And then - how is all of this supposed to be working in conjucntion > with live patching, where the patch may have been created by yet > another compiler version? > There is a version field in gcov_info, so we can compare that and reject incompatible version. We need to use hooks in livepatching to call the constructor / destructor when applying / reverting a live-patch. We might need to be cautious about locks or whatever, but I'm sure it can be figured out. But I have no idea how useful it would be to use gcov and livepatching together. For now the easiest thing to do is to depends on !LIVEPATCH in Kconfig. >>> Wouldn't it be just as easy, and more useful, to set a "has been >>> livepatched" flag, and return errors for all gcov hypercalls if its' set? >>> >>> I would expect most users to want to build a single hypervisor that can >>> be used for both gcov testing and live patching (under different >>> circumstances). >> I mean software provider, not user, of course. That's what I would want >> for CentOS, and I'm sure that's what the XenServer (and probably Oracle) >> guys want as well. > > GCOV is majority invasive, both in terms of performance (every basic > block needs to do a locked increment of a counter) and data size > (metadata for all basic blocks). > > No software vendor is ever going to have it enabled in a production > scenario. You're right, I wasn't thinking. But also presumably you'd like to be able to minimize the difference between the thing you're testing and the thing you ship; having to disable LivePatch increases the delta slightly. Anyway, I still think having them both Kconfig-ured is a good idea, but not so much that I'd spend any more time arguing for it. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 09:46:51AM -0400, Konrad Rzeszutek Wilk wrote: > On Wed, Oct 12, 2016 at 02:40:08PM +0100, Wei Liu wrote: > > On Wed, Oct 12, 2016 at 09:29:04AM -0400, Konrad Rzeszutek Wilk wrote: > > [...] > > > > > > > > Wouldn't it be just as easy, and more useful, to set a "has been > > > > livepatched" flag, and return errors for all gcov hypercalls if its' > > > > set? > > > > > > > > I would expect most users to want to build a single hypervisor that can > > > > be used for both gcov testing and live patching (under different > > > > circumstances). > > > > > > I actually would welcome livepatching and gcov to see if the test-cases I > > > wrote > > > cover most of the code. > > > > > > > I don't follow. Gcov doesn't give you a call graph -- if that's what > > you're after. > > It gives me an idea which functions/branches have run (not the livepatch > itself - just the infrastructure around adding a livepatch, doing ELF checks, > etc). > > And more importantly - which ones haven't run and need some more test-cases. > Right. Thanks for the explanation. That would indeed be useful. Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 02:40:08PM +0100, Wei Liu wrote: > On Wed, Oct 12, 2016 at 09:29:04AM -0400, Konrad Rzeszutek Wilk wrote: > [...] > > > > > > Wouldn't it be just as easy, and more useful, to set a "has been > > > livepatched" flag, and return errors for all gcov hypercalls if its' set? > > > > > > I would expect most users to want to build a single hypervisor that can > > > be used for both gcov testing and live patching (under different > > > circumstances). > > > > I actually would welcome livepatching and gcov to see if the test-cases I > > wrote > > cover most of the code. > > > > I don't follow. Gcov doesn't give you a call graph -- if that's what > you're after. It gives me an idea which functions/branches have run (not the livepatch itself - just the infrastructure around adding a livepatch, doing ELF checks, etc). And more importantly - which ones haven't run and need some more test-cases. > > > Adding in checking livepatch (common/livepatch.c: prepare_payload) to > > examine > > the .gcov_info and see if it matches the hypervisor one, is fine too. > > > > This then involves a non-trivial amount of work to figure out all the > corner cases. It's better to defer that to a later stage. Sure thing. And the !LIVEPATCH is fine for now. I just need to get an idea of what this would entail so it can get done. > > Wei. > > > > > > > -George > > > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 07:31:52AM -0600, Jan Beulich wrote: > >>> On 12.10.16 at 15:23, wrote: > >> And then - how is all of this supposed to be working in conjucntion > >> with live patching, where the patch may have been created by yet > >> another compiler version? > > > > Uh, I hope one does not create a livepatch patch with another compiler > > version! > > > > Let me put on the TODO to make livepatch-build-tools check gcc against > > compile.h so that one does not try this. > > What's wrong with mixing compiler versions in general? Besides scaring me? The one issue we had encountered was with compilers generating random named symbols for the switch tables. Those end up being called "CSWTCH.XYZ" where the XYZ depends on the position of the moon along with how many goats you have sacrificied to the altar of GCC gods. Older compilers don't neccessarily do it, newer ones do, and every time you build an livepatch the naming is different. Frustrating. It maybe that newer versions of GCC are more predictable about this naming. Maybe Martin can share some of his experience? CC-ing him. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On 12/10/16 14:41, George Dunlap wrote: > On 12/10/16 14:34, Andrew Cooper wrote: >> On 12/10/16 14:26, George Dunlap wrote: >>> On 12/10/16 14:24, George Dunlap wrote: On 12/10/16 14:06, Wei Liu wrote: > On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: > On 11.10.16 at 12:31, wrote: >>> --- /dev/null >>> +++ b/xen/common/gcov/gcc_4_7.c >>> @@ -0,0 +1,205 @@ >>> +/* >>> + * This code provides functions to handle gcc's profiling data format >>> + * introduced with gcc 4.7. >>> + * >>> + * This file is based heavily on gcc_3_4.c file. >>> + * >>> + * For a better understanding, refer to gcc source: >>> + * gcc/gcov-io.h >>> + * libgcc/libgcov.c >>> + * >>> + * Uses gcc-internal data definitions. >>> + * >>> + * Imported from Linux and modified for Xen by >>> + *Wei Liu >>> + */ >>> + >>> +#include >>> + >>> +#include "gcov.h" >>> + >>> +#if GCC_VERSION < 40700 >>> +#error "Wrong version of GCC used to compile gcov" >>> +#endif >>> + >>> +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) >>> +#define GCOV_COUNTERS 10 >>> +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 >>> +#define GCOV_COUNTERS 9 >>> +#else >>> +#define GCOV_COUNTERS 8 >>> +#endif >> I'm sorry for not having pointed this out on v2 (I had noticed it, >> but then didn't finish analyzing the situation), but I'm afraid this >> together with ... >> >>> +struct gcov_info { >>> +unsigned int version; >>> +struct gcov_info *next; >>> +unsigned int stamp; >>> +const char *filename; >>> +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); >>> +unsigned int n_functions; >>> +struct gcov_fn_info **functions; >>> +}; >> ... this structure's trailing fields actually getting used by the code >> won't work well when changing compiler versions without cleaning >> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c >> #define-ing their GCOV_COUNTERS and then #include-ing this >> shared source file. Plus btw, I don't think gcc 5.0.x (the >> development variant of 5.x) would use anything different from >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not >> normally be necessary anymore with gcc 5+. >> > Right. I will do something about this. Thanks for catching this. > >> And then - how is all of this supposed to be working in conjucntion >> with live patching, where the patch may have been created by yet >> another compiler version? >> > There is a version field in gcov_info, so we can compare that and reject > incompatible version. > > We need to use hooks in livepatching to call the constructor / > destructor when applying / reverting a live-patch. We might need to be > cautious about locks or whatever, but I'm sure it can be figured out. > > But I have no idea how useful it would be to use gcov and livepatching > together. For now the easiest thing to do is to > >depends on !LIVEPATCH > > in Kconfig. Wouldn't it be just as easy, and more useful, to set a "has been livepatched" flag, and return errors for all gcov hypercalls if its' set? I would expect most users to want to build a single hypervisor that can be used for both gcov testing and live patching (under different circumstances). >>> I mean software provider, not user, of course. That's what I would want >>> for CentOS, and I'm sure that's what the XenServer (and probably Oracle) >>> guys want as well. >> GCOV is majority invasive, both in terms of performance (every basic >> block needs to do a locked increment of a counter) and data size >> (metadata for all basic blocks). >> >> No software vendor is ever going to have it enabled in a production >> scenario. > You're right, I wasn't thinking. > > But also presumably you'd like to be able to minimize the difference > between the thing you're testing and the thing you ship; having to > disable LivePatch increases the delta slightly. > > Anyway, I still think having them both Kconfig-ured is a good idea, but > not so much that I'd spend any more time arguing for it. It is a testing feature. It would certainly be nice to get code coverage of the livepatching parts, but that absolutely shouldn't block making gcov usable in the first place. It might be worth sticking a #TODO make GCOV worth with Livepatching in the kconfig file (for anyone who stumbles across this dependency). ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 09:29:04AM -0400, Konrad Rzeszutek Wilk wrote: [...] > > > > Wouldn't it be just as easy, and more useful, to set a "has been > > livepatched" flag, and return errors for all gcov hypercalls if its' set? > > > > I would expect most users to want to build a single hypervisor that can > > be used for both gcov testing and live patching (under different > > circumstances). > > I actually would welcome livepatching and gcov to see if the test-cases I > wrote > cover most of the code. > I don't follow. Gcov doesn't give you a call graph -- if that's what you're after. > Adding in checking livepatch (common/livepatch.c: prepare_payload) to examine > the .gcov_info and see if it matches the hypervisor one, is fine too. > This then involves a non-trivial amount of work to figure out all the corner cases. It's better to defer that to a later stage. Wei. > > > > -George > > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On 12/10/16 14:26, George Dunlap wrote: > On 12/10/16 14:24, George Dunlap wrote: >> On 12/10/16 14:06, Wei Liu wrote: >>> On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: >>> On 11.10.16 at 12:31, wrote: > --- /dev/null > +++ b/xen/common/gcov/gcc_4_7.c > @@ -0,0 +1,205 @@ > +/* > + * This code provides functions to handle gcc's profiling data format > + * introduced with gcc 4.7. > + * > + * This file is based heavily on gcc_3_4.c file. > + * > + * For a better understanding, refer to gcc source: > + * gcc/gcov-io.h > + * libgcc/libgcov.c > + * > + * Uses gcc-internal data definitions. > + * > + * Imported from Linux and modified for Xen by > + *Wei Liu > + */ > + > +#include > + > +#include "gcov.h" > + > +#if GCC_VERSION < 40700 > +#error "Wrong version of GCC used to compile gcov" > +#endif > + > +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) > +#define GCOV_COUNTERS 10 > +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 > +#define GCOV_COUNTERS 9 > +#else > +#define GCOV_COUNTERS 8 > +#endif I'm sorry for not having pointed this out on v2 (I had noticed it, but then didn't finish analyzing the situation), but I'm afraid this together with ... > +struct gcov_info { > +unsigned int version; > +struct gcov_info *next; > +unsigned int stamp; > +const char *filename; > +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); > +unsigned int n_functions; > +struct gcov_fn_info **functions; > +}; ... this structure's trailing fields actually getting used by the code won't work well when changing compiler versions without cleaning the tree. I think instead you need thin gcc_5.c and gcc_4_9.c #define-ing their GCOV_COUNTERS and then #include-ing this shared source file. Plus btw, I don't think gcc 5.0.x (the development variant of 5.x) would use anything different from 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not normally be necessary anymore with gcc 5+. >>> Right. I will do something about this. Thanks for catching this. >>> And then - how is all of this supposed to be working in conjucntion with live patching, where the patch may have been created by yet another compiler version? >>> There is a version field in gcov_info, so we can compare that and reject >>> incompatible version. >>> >>> We need to use hooks in livepatching to call the constructor / >>> destructor when applying / reverting a live-patch. We might need to be >>> cautious about locks or whatever, but I'm sure it can be figured out. >>> >>> But I have no idea how useful it would be to use gcov and livepatching >>> together. For now the easiest thing to do is to >>> >>>depends on !LIVEPATCH >>> >>> in Kconfig. >> Wouldn't it be just as easy, and more useful, to set a "has been >> livepatched" flag, and return errors for all gcov hypercalls if its' set? >> >> I would expect most users to want to build a single hypervisor that can >> be used for both gcov testing and live patching (under different >> circumstances). > I mean software provider, not user, of course. That's what I would want > for CentOS, and I'm sure that's what the XenServer (and probably Oracle) > guys want as well. GCOV is majority invasive, both in terms of performance (every basic block needs to do a locked increment of a counter) and data size (metadata for all basic blocks). No software vendor is ever going to have it enabled in a production scenario. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
>>> On 12.10.16 at 15:26, wrote: > On 12/10/16 14:24, George Dunlap wrote: >> I would expect most users to want to build a single hypervisor that can >> be used for both gcov testing and live patching (under different >> circumstances). > > I mean software provider, not user, of course. That's what I would want > for CentOS, and I'm sure that's what the XenServer (and probably Oracle) > guys want as well. But gcov is explicitly a non-production feature, iirc. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 02:26:26PM +0100, George Dunlap wrote: [...] > >> > >> There is a version field in gcov_info, so we can compare that and reject > >> incompatible version. > >> > >> We need to use hooks in livepatching to call the constructor / > >> destructor when applying / reverting a live-patch. We might need to be > >> cautious about locks or whatever, but I'm sure it can be figured out. > >> > >> But I have no idea how useful it would be to use gcov and livepatching > >> together. For now the easiest thing to do is to > >> > >>depends on !LIVEPATCH > >> > >> in Kconfig. > > > > Wouldn't it be just as easy, and more useful, to set a "has been > > livepatched" flag, and return errors for all gcov hypercalls if its' set? > > > > I would expect most users to want to build a single hypervisor that can > > be used for both gcov testing and live patching (under different > > circumstances). > > I mean software provider, not user, of course. That's what I would want > for CentOS, and I'm sure that's what the XenServer (and probably Oracle) > guys want as well. > The usefulness of such build is not as big as you might think I'm afraid -- it wouldn't be useful until we figure out how to apply a livepatch generated with gcov support built in. Wei. > -George > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
>>> On 12.10.16 at 15:23, wrote: >> And then - how is all of this supposed to be working in conjucntion >> with live patching, where the patch may have been created by yet >> another compiler version? > > Uh, I hope one does not create a livepatch patch with another compiler > version! > > Let me put on the TODO to make livepatch-build-tools check gcc against > compile.h so that one does not try this. What's wrong with mixing compiler versions in general? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 02:24:51PM +0100, George Dunlap wrote: > On 12/10/16 14:06, Wei Liu wrote: > > On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: > > On 11.10.16 at 12:31, wrote: > >>> --- /dev/null > >>> +++ b/xen/common/gcov/gcc_4_7.c > >>> @@ -0,0 +1,205 @@ > >>> +/* > >>> + * This code provides functions to handle gcc's profiling data format > >>> + * introduced with gcc 4.7. > >>> + * > >>> + * This file is based heavily on gcc_3_4.c file. > >>> + * > >>> + * For a better understanding, refer to gcc source: > >>> + * gcc/gcov-io.h > >>> + * libgcc/libgcov.c > >>> + * > >>> + * Uses gcc-internal data definitions. > >>> + * > >>> + * Imported from Linux and modified for Xen by > >>> + *Wei Liu > >>> + */ > >>> + > >>> +#include > >>> + > >>> +#include "gcov.h" > >>> + > >>> +#if GCC_VERSION < 40700 > >>> +#error "Wrong version of GCC used to compile gcov" > >>> +#endif > >>> + > >>> +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) > >>> +#define GCOV_COUNTERS 10 > >>> +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 > >>> +#define GCOV_COUNTERS 9 > >>> +#else > >>> +#define GCOV_COUNTERS 8 > >>> +#endif > >> > >> I'm sorry for not having pointed this out on v2 (I had noticed it, > >> but then didn't finish analyzing the situation), but I'm afraid this > >> together with ... > >> > >>> +struct gcov_info { > >>> +unsigned int version; > >>> +struct gcov_info *next; > >>> +unsigned int stamp; > >>> +const char *filename; > >>> +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); > >>> +unsigned int n_functions; > >>> +struct gcov_fn_info **functions; > >>> +}; > >> > >> ... this structure's trailing fields actually getting used by the code > >> won't work well when changing compiler versions without cleaning > >> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c > >> #define-ing their GCOV_COUNTERS and then #include-ing this > >> shared source file. Plus btw, I don't think gcc 5.0.x (the > >> development variant of 5.x) would use anything different from > >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not > >> normally be necessary anymore with gcc 5+. > >> > > > > Right. I will do something about this. Thanks for catching this. > > > >> And then - how is all of this supposed to be working in conjucntion > >> with live patching, where the patch may have been created by yet > >> another compiler version? > >> > > > > There is a version field in gcov_info, so we can compare that and reject > > incompatible version. > > > > We need to use hooks in livepatching to call the constructor / > > destructor when applying / reverting a live-patch. We might need to be > > cautious about locks or whatever, but I'm sure it can be figured out. > > > > But I have no idea how useful it would be to use gcov and livepatching > > together. For now the easiest thing to do is to > > > >depends on !LIVEPATCH > > > > in Kconfig. > > Wouldn't it be just as easy, and more useful, to set a "has been > livepatched" flag, and return errors for all gcov hypercalls if its' set? > > I would expect most users to want to build a single hypervisor that can > be used for both gcov testing and live patching (under different > circumstances). I actually would welcome livepatching and gcov to see if the test-cases I wrote cover most of the code. Adding in checking livepatch (common/livepatch.c: prepare_payload) to examine the .gcov_info and see if it matches the hypervisor one, is fine too. > > -George > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On 12/10/16 14:24, George Dunlap wrote: > On 12/10/16 14:06, Wei Liu wrote: >> On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: >> On 11.10.16 at 12:31, wrote: --- /dev/null +++ b/xen/common/gcov/gcc_4_7.c @@ -0,0 +1,205 @@ +/* + * This code provides functions to handle gcc's profiling data format + * introduced with gcc 4.7. + * + * This file is based heavily on gcc_3_4.c file. + * + * For a better understanding, refer to gcc source: + * gcc/gcov-io.h + * libgcc/libgcov.c + * + * Uses gcc-internal data definitions. + * + * Imported from Linux and modified for Xen by + *Wei Liu + */ + +#include + +#include "gcov.h" + +#if GCC_VERSION < 40700 +#error "Wrong version of GCC used to compile gcov" +#endif + +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) +#define GCOV_COUNTERS 10 +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 +#define GCOV_COUNTERS 9 +#else +#define GCOV_COUNTERS 8 +#endif >>> >>> I'm sorry for not having pointed this out on v2 (I had noticed it, >>> but then didn't finish analyzing the situation), but I'm afraid this >>> together with ... >>> +struct gcov_info { +unsigned int version; +struct gcov_info *next; +unsigned int stamp; +const char *filename; +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); +unsigned int n_functions; +struct gcov_fn_info **functions; +}; >>> >>> ... this structure's trailing fields actually getting used by the code >>> won't work well when changing compiler versions without cleaning >>> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c >>> #define-ing their GCOV_COUNTERS and then #include-ing this >>> shared source file. Plus btw, I don't think gcc 5.0.x (the >>> development variant of 5.x) would use anything different from >>> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not >>> normally be necessary anymore with gcc 5+. >>> >> >> Right. I will do something about this. Thanks for catching this. >> >>> And then - how is all of this supposed to be working in conjucntion >>> with live patching, where the patch may have been created by yet >>> another compiler version? >>> >> >> There is a version field in gcov_info, so we can compare that and reject >> incompatible version. >> >> We need to use hooks in livepatching to call the constructor / >> destructor when applying / reverting a live-patch. We might need to be >> cautious about locks or whatever, but I'm sure it can be figured out. >> >> But I have no idea how useful it would be to use gcov and livepatching >> together. For now the easiest thing to do is to >> >>depends on !LIVEPATCH >> >> in Kconfig. > > Wouldn't it be just as easy, and more useful, to set a "has been > livepatched" flag, and return errors for all gcov hypercalls if its' set? > > I would expect most users to want to build a single hypervisor that can > be used for both gcov testing and live patching (under different > circumstances). I mean software provider, not user, of course. That's what I would want for CentOS, and I'm sure that's what the XenServer (and probably Oracle) guys want as well. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On 12/10/16 14:06, Wei Liu wrote: > On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: > On 11.10.16 at 12:31, wrote: >>> --- /dev/null >>> +++ b/xen/common/gcov/gcc_4_7.c >>> @@ -0,0 +1,205 @@ >>> +/* >>> + * This code provides functions to handle gcc's profiling data format >>> + * introduced with gcc 4.7. >>> + * >>> + * This file is based heavily on gcc_3_4.c file. >>> + * >>> + * For a better understanding, refer to gcc source: >>> + * gcc/gcov-io.h >>> + * libgcc/libgcov.c >>> + * >>> + * Uses gcc-internal data definitions. >>> + * >>> + * Imported from Linux and modified for Xen by >>> + *Wei Liu >>> + */ >>> + >>> +#include >>> + >>> +#include "gcov.h" >>> + >>> +#if GCC_VERSION < 40700 >>> +#error "Wrong version of GCC used to compile gcov" >>> +#endif >>> + >>> +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) >>> +#define GCOV_COUNTERS 10 >>> +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 >>> +#define GCOV_COUNTERS 9 >>> +#else >>> +#define GCOV_COUNTERS 8 >>> +#endif >> >> I'm sorry for not having pointed this out on v2 (I had noticed it, >> but then didn't finish analyzing the situation), but I'm afraid this >> together with ... >> >>> +struct gcov_info { >>> +unsigned int version; >>> +struct gcov_info *next; >>> +unsigned int stamp; >>> +const char *filename; >>> +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); >>> +unsigned int n_functions; >>> +struct gcov_fn_info **functions; >>> +}; >> >> ... this structure's trailing fields actually getting used by the code >> won't work well when changing compiler versions without cleaning >> the tree. I think instead you need thin gcc_5.c and gcc_4_9.c >> #define-ing their GCOV_COUNTERS and then #include-ing this >> shared source file. Plus btw, I don't think gcc 5.0.x (the >> development variant of 5.x) would use anything different from >> 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not >> normally be necessary anymore with gcc 5+. >> > > Right. I will do something about this. Thanks for catching this. > >> And then - how is all of this supposed to be working in conjucntion >> with live patching, where the patch may have been created by yet >> another compiler version? >> > > There is a version field in gcov_info, so we can compare that and reject > incompatible version. > > We need to use hooks in livepatching to call the constructor / > destructor when applying / reverting a live-patch. We might need to be > cautious about locks or whatever, but I'm sure it can be figured out. > > But I have no idea how useful it would be to use gcov and livepatching > together. For now the easiest thing to do is to > >depends on !LIVEPATCH > > in Kconfig. Wouldn't it be just as easy, and more useful, to set a "has been livepatched" flag, and return errors for all gcov hypercalls if its' set? I would expect most users to want to build a single hypervisor that can be used for both gcov testing and live patching (under different circumstances). -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
> And then - how is all of this supposed to be working in conjucntion > with live patching, where the patch may have been created by yet > another compiler version? Uh, I hope one does not create a livepatch patch with another compiler version! Let me put on the TODO to make livepatch-build-tools check gcc against compile.h so that one does not try this. > > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
>>> On 12.10.16 at 15:06, wrote: > But I have no idea how useful it would be to use gcov and livepatching > together. For now the easiest thing to do is to > >depends on !LIVEPATCH > > in Kconfig. I agree. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
On Wed, Oct 12, 2016 at 06:42:53AM -0600, Jan Beulich wrote: > >>> On 11.10.16 at 12:31, wrote: > > --- /dev/null > > +++ b/xen/common/gcov/gcc_4_7.c > > @@ -0,0 +1,205 @@ > > +/* > > + * This code provides functions to handle gcc's profiling data format > > + * introduced with gcc 4.7. > > + * > > + * This file is based heavily on gcc_3_4.c file. > > + * > > + * For a better understanding, refer to gcc source: > > + * gcc/gcov-io.h > > + * libgcc/libgcov.c > > + * > > + * Uses gcc-internal data definitions. > > + * > > + * Imported from Linux and modified for Xen by > > + *Wei Liu > > + */ > > + > > +#include > > + > > +#include "gcov.h" > > + > > +#if GCC_VERSION < 40700 > > +#error "Wrong version of GCC used to compile gcov" > > +#endif > > + > > +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) > > +#define GCOV_COUNTERS 10 > > +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 > > +#define GCOV_COUNTERS 9 > > +#else > > +#define GCOV_COUNTERS 8 > > +#endif > > I'm sorry for not having pointed this out on v2 (I had noticed it, > but then didn't finish analyzing the situation), but I'm afraid this > together with ... > > > +struct gcov_info { > > +unsigned int version; > > +struct gcov_info *next; > > +unsigned int stamp; > > +const char *filename; > > +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); > > +unsigned int n_functions; > > +struct gcov_fn_info **functions; > > +}; > > ... this structure's trailing fields actually getting used by the code > won't work well when changing compiler versions without cleaning > the tree. I think instead you need thin gcc_5.c and gcc_4_9.c > #define-ing their GCOV_COUNTERS and then #include-ing this > shared source file. Plus btw, I don't think gcc 5.0.x (the > development variant of 5.x) would use anything different from > 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not > normally be necessary anymore with gcc 5+. > Right. I will do something about this. Thanks for catching this. > And then - how is all of this supposed to be working in conjucntion > with live patching, where the patch may have been created by yet > another compiler version? > There is a version field in gcov_info, so we can compare that and reject incompatible version. We need to use hooks in livepatching to call the constructor / destructor when applying / reverting a live-patch. We might need to be cautious about locks or whatever, but I'm sure it can be figured out. But I have no idea how useful it would be to use gcov and livepatching together. For now the easiest thing to do is to depends on !LIVEPATCH in Kconfig. Wei. > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
>>> On 11.10.16 at 12:31, wrote: > --- /dev/null > +++ b/xen/common/gcov/gcc_4_7.c > @@ -0,0 +1,205 @@ > +/* > + * This code provides functions to handle gcc's profiling data format > + * introduced with gcc 4.7. > + * > + * This file is based heavily on gcc_3_4.c file. > + * > + * For a better understanding, refer to gcc source: > + * gcc/gcov-io.h > + * libgcc/libgcov.c > + * > + * Uses gcc-internal data definitions. > + * > + * Imported from Linux and modified for Xen by > + *Wei Liu > + */ > + > +#include > + > +#include "gcov.h" > + > +#if GCC_VERSION < 40700 > +#error "Wrong version of GCC used to compile gcov" > +#endif > + > +#if (__GNUC__ > 5) || (__GNUC__ == 5 && __GNUC_MINOR__ >= 1) > +#define GCOV_COUNTERS 10 > +#elif __GNUC__ == 4 && __GNUC_MINOR__ >= 9 > +#define GCOV_COUNTERS 9 > +#else > +#define GCOV_COUNTERS 8 > +#endif I'm sorry for not having pointed this out on v2 (I had noticed it, but then didn't finish analyzing the situation), but I'm afraid this together with ... > +struct gcov_info { > +unsigned int version; > +struct gcov_info *next; > +unsigned int stamp; > +const char *filename; > +void (*merge[GCOV_COUNTERS])(gcov_type *, unsigned int); > +unsigned int n_functions; > +struct gcov_fn_info **functions; > +}; ... this structure's trailing fields actually getting used by the code won't work well when changing compiler versions without cleaning the tree. I think instead you need thin gcc_5.c and gcc_4_9.c #define-ing their GCOV_COUNTERS and then #include-ing this shared source file. Plus btw, I don't think gcc 5.0.x (the development variant of 5.x) would use anything different from 5.1.x or 5.2.x; in fact use of __GNUC_MINOR__ should not normally be necessary anymore with gcc 5+. And then - how is all of this supposed to be working in conjucntion with live patching, where the patch may have been created by yet another compiler version? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3] gcov: add new interface and 3.4 and 4.7 format support
A new sysctl interface for passing gcov data back to userspace. The new interface uses a customised record file format. The new sysctl reuses original sysctl number but renames the op to gcov_op. Both gcc 3.4 and 4.7 format are supported. The code is rewritten so that a new format can be easily added in the future. Version specific code is grouped into different files. The format one needs to use can be picked via Kconfig. The default format is 4.7 format. Userspace programs to handle extracted data will come in a later patch. Signed-off-by: Wei Liu --- As always, v3 in my xenbits xen.git tree. Only sending this unacked patch. v3: 1. Check gcc version in gcc_*.c files. 2. Eliminate u32 / u64. 3. Mark __gcov_init as __init. 4. Fix a rebase error in v2. 5. Some other cosmetic fixes. v2: 1. Use tab in Kconfig and indent help text properly. 2. Bump XEN_SYSCTL_INTERFACE_VERSION. 3. Fix cosmetic issues. Cc: Andrew Cooper Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Konrad Rzeszutek Wilk Cc: Stefano Stabellini Cc: Tim Deegan Cc: Wei Liu --- xen/Kconfig.debug | 24 ++- xen/common/gcov/Makefile| 3 + xen/common/gcov/gcc_3_4.c | 367 xen/common/gcov/gcc_4_7.c | 205 + xen/common/gcov/gcov.c | 257 +++ xen/common/gcov/gcov.h | 40 + xen/common/gcov/gcov_base.c | 60 xen/common/sysctl.c | 8 + xen/include/public/sysctl.h | 39 - xen/include/xen/gcov.h | 9 ++ 10 files changed, 1005 insertions(+), 7 deletions(-) create mode 100644 xen/common/gcov/gcc_3_4.c create mode 100644 xen/common/gcov/gcc_4_7.c create mode 100644 xen/common/gcov/gcov.c create mode 100644 xen/common/gcov/gcov.h create mode 100644 xen/common/gcov/gcov_base.c create mode 100644 xen/include/xen/gcov.h diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug index ef863dc..c65e543 100644 --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -30,16 +30,30 @@ config FRAME_POINTER config GCOV bool "Gcov Support" - depends on BROKEN ---help--- Enable gcov (a test coverage program in GCC) support. - Currently the data structure and hypercall interface are tied - to GCC 3.4 gcov format. You need to have a version of GCC - that is compatible with that format to make gcov work. - If unsure, say N here. +choice + prompt "Specify Gcov format" + depends on GCOV + default GCOV_FORMAT_4_7 + ---help--- + The gcov format is determined by gcc version. + +config GCOV_FORMAT_4_7 + bool "GCC 4.7 format" + ---help--- + Select this option to use the format specified in GCC 4.7. + +config GCOV_FORMAT_3_4 + bool "GCC 3.4 format" + ---help--- + Select this option to use the format specified in GCC 3.4. + +endchoice + config LOCK_PROFILE bool "Lock Profiling" ---help--- diff --git a/xen/common/gcov/Makefile b/xen/common/gcov/Makefile index e69de29..03ac1e5 100644 --- a/xen/common/gcov/Makefile +++ b/xen/common/gcov/Makefile @@ -0,0 +1,3 @@ +obj-y += gcov_base.o gcov.o +obj-$(CONFIG_GCOV_FORMAT_3_4) += gcc_3_4.o +obj-$(CONFIG_GCOV_FORMAT_4_7) += gcc_4_7.o diff --git a/xen/common/gcov/gcc_3_4.c b/xen/common/gcov/gcc_3_4.c new file mode 100644 index 000..3631f4b --- /dev/null +++ b/xen/common/gcov/gcc_3_4.c @@ -0,0 +1,367 @@ +/* + * This code provides functions to handle gcc's profiling data format + * introduced with gcc 3.4. Future versions of gcc may change the gcov + * format (as happened before), so all format-specific information needs + * to be kept modular and easily exchangeable. + * + * This file is based on gcc-internal definitions. Functions and data + * structures are defined to be compatible with gcc counterparts. + * For a better understanding, refer to gcc source: gcc/gcov-io.h. + * + *Copyright IBM Corp. 2009 + *Author(s): Peter Oberparleiter + * + *Uses gcc-internal data definitions. + * + * Imported from Linux and modified for Xen by + *Wei Liu + */ + + +#include + +#include "gcov.h" + +#if !(GCC_VERSION >= 30400 && GCC_VERSION < 40700) +#error "Wrong version of GCC used to compile gcov" +#endif + +#define GCOV_COUNTERS 5 + +static struct gcov_info *gcov_info_head; + +/** + * struct gcov_fn_info - profiling meta data per function + * @ident: object file-unique function identifier + * @checksum: function checksum + * @n_ctrs: number of values per counter type belonging to this function + * + * This data is generated by gcc during compilation and doesn't change + * at run-time. + */ +struct gcov_fn_info +{ +unsigned int ident; +unsigned int checksum; +unsigned int n_ctrs[0]; +}; + +/** + * struct gcov_ctr_info - profiling data per counter type + * @num: number of counter values for this type + * @values: array of counter values for this type + * @merge: merge function for cou