| Message ID | ae31ccf1-7334-cdf9-9b90-edac7ca4e148@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] lib: drop (replace) debug_build() | expand |
On 23/12/2020 16:05, Jan Beulich wrote: > Its expansion shouldn't be tied to NDEBUG - down the road we may want to > allow enabling assertions independently of CONFIG_DEBUG. Replace the few > uses by a new xen_build_info() helper, subsuming gcov_string at the same > time (while replacing the stale CONFIG_GCOV used there) and also adding > CONFIG_UBSAN indication. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although... > --- > v2: Introduce xen_build_info() including also gcov and ubsan info. > > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -175,14 +175,14 @@ static void print_xen_info(void) > { > char taint_str[TAINT_STRING_MAX_LEN]; > > - printk("----[ Xen-%d.%d%s %s debug=%c " gcov_string " %s ]----\n", > + printk("----[ Xen-%d.%d%s %s %s %s ]----\n", > xen_major_version(), xen_minor_version(), xen_extra_version(), > #ifdef CONFIG_ARM_32 > "arm32", > #else > "arm64", > #endif > - debug_build() ? 'y' : 'n', print_tainted(taint_str)); > + xen_build_info(), print_tainted(taint_str)); > } > > #ifdef CONFIG_ARM_32 > --- a/xen/arch/x86/x86_64/traps.c > +++ b/xen/arch/x86/x86_64/traps.c > @@ -29,9 +29,9 @@ static void print_xen_info(void) > { > char taint_str[TAINT_STRING_MAX_LEN]; > > - printk("----[ Xen-%d.%d%s x86_64 debug=%c " gcov_string " %s ]----\n", > + printk("----[ Xen-%d.%d%s x86_64 %s %s ]----\n", > xen_major_version(), xen_minor_version(), xen_extra_version(), > - debug_build() ? 'y' : 'n', print_tainted(taint_str)); > + xen_build_info(), print_tainted(taint_str)); > } > > enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest }; > --- a/xen/common/version.c > +++ b/xen/common/version.c > @@ -70,6 +70,30 @@ const char *xen_deny(void) > return "<denied>"; > } > > +static const char build_info[] = > + "debug=" > +#ifdef CONFIG_DEBUG > + "y" > +#else > + "n" > +#endif > +#ifdef CONFIG_COVERAGE > +# ifdef __clang__ > + " llvmcov=y" > +# else > + " gcov=y" > +# endif > +#endif > +#ifdef CONFIG_UBSAN > + " ubsan=y" > +#endif > + ""; > + > +const char *xen_build_info(void) > +{ > + return build_info; > +} ... do we really need a function here? Wouldn't an extern const char build_info[] do? ~Andrew > + > static const void *build_id_p __read_mostly; > static unsigned int build_id_len __read_mostly; > > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -1002,10 +1002,10 @@ void __init console_init_preirq(void) > spin_lock(&console_lock); > __putstr(xen_banner()); > spin_unlock(&console_lock); > - printk("Xen version %d.%d%s (%s@%s) (%s) debug=%c " gcov_string " %s\n", > + printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n", > xen_major_version(), xen_minor_version(), xen_extra_version(), > - xen_compile_by(), xen_compile_domain(), > - xen_compiler(), debug_build() ? 'y' : 'n', xen_compile_date()); > + xen_compile_by(), xen_compile_domain(), xen_compiler(), > + xen_build_info(), xen_compile_date()); > printk("Latest ChangeSet: %s\n", xen_changeset()); > > /* Locate and print the buildid, if applicable. */ > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -48,21 +48,13 @@ > #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond)) > #endif > > -#ifdef CONFIG_GCOV > -#define gcov_string "gcov=y" > -#else > -#define gcov_string "" > -#endif > - > #ifndef NDEBUG > #define ASSERT(p) \ > do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) > #define ASSERT_UNREACHABLE() assert_failed("unreachable") > -#define debug_build() 1 > #else > #define ASSERT(p) do { if ( 0 && (p) ) {} } while (0) > #define ASSERT_UNREACHABLE() do { } while (0) > -#define debug_build() 0 > #endif > > #define ABS(_x) ({ \ > --- a/xen/include/xen/version.h > +++ b/xen/include/xen/version.h > @@ -16,6 +16,7 @@ const char *xen_extra_version(void); > const char *xen_changeset(void); > const char *xen_banner(void); > const char *xen_deny(void); > +const char *xen_build_info(void); > int xen_build_id(const void **p, unsigned int *len); > > #ifdef BUILD_ID
On 23.12.2020 17:53, Andrew Cooper wrote: > On 23/12/2020 16:05, Jan Beulich wrote: >> Its expansion shouldn't be tied to NDEBUG - down the road we may want to >> allow enabling assertions independently of CONFIG_DEBUG. Replace the few >> uses by a new xen_build_info() helper, subsuming gcov_string at the same >> time (while replacing the stale CONFIG_GCOV used there) and also adding >> CONFIG_UBSAN indication. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, Thanks. >> --- a/xen/common/version.c >> +++ b/xen/common/version.c >> @@ -70,6 +70,30 @@ const char *xen_deny(void) >> return "<denied>"; >> } >> >> +static const char build_info[] = >> + "debug=" >> +#ifdef CONFIG_DEBUG >> + "y" >> +#else >> + "n" >> +#endif >> +#ifdef CONFIG_COVERAGE >> +# ifdef __clang__ >> + " llvmcov=y" >> +# else >> + " gcov=y" >> +# endif >> +#endif >> +#ifdef CONFIG_UBSAN >> + " ubsan=y" >> +#endif >> + ""; >> + >> +const char *xen_build_info(void) >> +{ >> + return build_info; >> +} > > ... do we really need a function here? > > Wouldn't an extern const char build_info[] do? It probably would, but I wanted things to remain consistent with the siblings, many of which also return string literals (or effectively plain numbers). Jan
On 23/12/2020 16:59, Jan Beulich wrote: > On 23.12.2020 17:53, Andrew Cooper wrote: >> On 23/12/2020 16:05, Jan Beulich wrote: >>> Its expansion shouldn't be tied to NDEBUG - down the road we may want to >>> allow enabling assertions independently of CONFIG_DEBUG. Replace the few >>> uses by a new xen_build_info() helper, subsuming gcov_string at the same >>> time (while replacing the stale CONFIG_GCOV used there) and also adding >>> CONFIG_UBSAN indication. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, > Thanks. > >>> --- a/xen/common/version.c >>> +++ b/xen/common/version.c >>> @@ -70,6 +70,30 @@ const char *xen_deny(void) >>> return "<denied>"; >>> } >>> >>> +static const char build_info[] = >>> + "debug=" >>> +#ifdef CONFIG_DEBUG >>> + "y" >>> +#else >>> + "n" >>> +#endif >>> +#ifdef CONFIG_COVERAGE >>> +# ifdef __clang__ >>> + " llvmcov=y" >>> +# else >>> + " gcov=y" >>> +# endif >>> +#endif >>> +#ifdef CONFIG_UBSAN >>> + " ubsan=y" >>> +#endif >>> + ""; >>> + >>> +const char *xen_build_info(void) >>> +{ >>> + return build_info; >>> +} >> ... do we really need a function here? >> >> Wouldn't an extern const char build_info[] do? > It probably would, but I wanted things to remain consistent with > the siblings, many of which also return string literals (or > effectively plain numbers). The only reason they are still functions is because there was an argument over breaking the livepatch testing on older versions of Xen, and I got bored arguing. I, however, don't consider this a valid reason to block improvements. ~Andrew
Hi Jan, On 23/12/2020 16:05, Jan Beulich wrote: > Its expansion shouldn't be tied to NDEBUG - down the road we may want to > allow enabling assertions independently of CONFIG_DEBUG. Replace the few > uses by a new xen_build_info() helper, subsuming gcov_string at the same > time (while replacing the stale CONFIG_GCOV used there) and also adding > CONFIG_UBSAN indication. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> For the Arm code: Acked-by: Julien Grall <jgrall@amazon.com> Cheers, > --- > v2: Introduce xen_build_info() including also gcov and ubsan info. > > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -175,14 +175,14 @@ static void print_xen_info(void) > { > char taint_str[TAINT_STRING_MAX_LEN]; > > - printk("----[ Xen-%d.%d%s %s debug=%c " gcov_string " %s ]----\n", > + printk("----[ Xen-%d.%d%s %s %s %s ]----\n", > xen_major_version(), xen_minor_version(), xen_extra_version(), > #ifdef CONFIG_ARM_32 > "arm32", > #else > "arm64", > #endif > - debug_build() ? 'y' : 'n', print_tainted(taint_str)); > + xen_build_info(), print_tainted(taint_str)); > } > > #ifdef CONFIG_ARM_32 > --- a/xen/arch/x86/x86_64/traps.c > +++ b/xen/arch/x86/x86_64/traps.c > @@ -29,9 +29,9 @@ static void print_xen_info(void) > { > char taint_str[TAINT_STRING_MAX_LEN]; > > - printk("----[ Xen-%d.%d%s x86_64 debug=%c " gcov_string " %s ]----\n", > + printk("----[ Xen-%d.%d%s x86_64 %s %s ]----\n", > xen_major_version(), xen_minor_version(), xen_extra_version(), > - debug_build() ? 'y' : 'n', print_tainted(taint_str)); > + xen_build_info(), print_tainted(taint_str)); > } > > enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest }; > --- a/xen/common/version.c > +++ b/xen/common/version.c > @@ -70,6 +70,30 @@ const char *xen_deny(void) > return "<denied>"; > } > > +static const char build_info[] = > + "debug=" > +#ifdef CONFIG_DEBUG > + "y" > +#else > + "n" > +#endif > +#ifdef CONFIG_COVERAGE > +# ifdef __clang__ > + " llvmcov=y" > +# else > + " gcov=y" > +# endif > +#endif > +#ifdef CONFIG_UBSAN > + " ubsan=y" > +#endif > + ""; > + > +const char *xen_build_info(void) > +{ > + return build_info; > +} > + > static const void *build_id_p __read_mostly; > static unsigned int build_id_len __read_mostly; > > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -1002,10 +1002,10 @@ void __init console_init_preirq(void) > spin_lock(&console_lock); > __putstr(xen_banner()); > spin_unlock(&console_lock); > - printk("Xen version %d.%d%s (%s@%s) (%s) debug=%c " gcov_string " %s\n", > + printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n", > xen_major_version(), xen_minor_version(), xen_extra_version(), > - xen_compile_by(), xen_compile_domain(), > - xen_compiler(), debug_build() ? 'y' : 'n', xen_compile_date()); > + xen_compile_by(), xen_compile_domain(), xen_compiler(), > + xen_build_info(), xen_compile_date()); > printk("Latest ChangeSet: %s\n", xen_changeset()); > > /* Locate and print the buildid, if applicable. */ > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -48,21 +48,13 @@ > #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond)) > #endif > > -#ifdef CONFIG_GCOV > -#define gcov_string "gcov=y" > -#else > -#define gcov_string "" > -#endif > - > #ifndef NDEBUG > #define ASSERT(p) \ > do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) > #define ASSERT_UNREACHABLE() assert_failed("unreachable") > -#define debug_build() 1 > #else > #define ASSERT(p) do { if ( 0 && (p) ) {} } while (0) > #define ASSERT_UNREACHABLE() do { } while (0) > -#define debug_build() 0 > #endif > > #define ABS(_x) ({ \ > --- a/xen/include/xen/version.h > +++ b/xen/include/xen/version.h > @@ -16,6 +16,7 @@ const char *xen_extra_version(void); > const char *xen_changeset(void); > const char *xen_banner(void); > const char *xen_deny(void); > +const char *xen_build_info(void); > int xen_build_id(const void **p, unsigned int *len); > > #ifdef BUILD_ID >
--- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -175,14 +175,14 @@ static void print_xen_info(void) { char taint_str[TAINT_STRING_MAX_LEN]; - printk("----[ Xen-%d.%d%s %s debug=%c " gcov_string " %s ]----\n", + printk("----[ Xen-%d.%d%s %s %s %s ]----\n", xen_major_version(), xen_minor_version(), xen_extra_version(), #ifdef CONFIG_ARM_32 "arm32", #else "arm64", #endif - debug_build() ? 'y' : 'n', print_tainted(taint_str)); + xen_build_info(), print_tainted(taint_str)); } #ifdef CONFIG_ARM_32 --- a/xen/arch/x86/x86_64/traps.c +++ b/xen/arch/x86/x86_64/traps.c @@ -29,9 +29,9 @@ static void print_xen_info(void) { char taint_str[TAINT_STRING_MAX_LEN]; - printk("----[ Xen-%d.%d%s x86_64 debug=%c " gcov_string " %s ]----\n", + printk("----[ Xen-%d.%d%s x86_64 %s %s ]----\n", xen_major_version(), xen_minor_version(), xen_extra_version(), - debug_build() ? 'y' : 'n', print_tainted(taint_str)); + xen_build_info(), print_tainted(taint_str)); } enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest }; --- a/xen/common/version.c +++ b/xen/common/version.c @@ -70,6 +70,30 @@ const char *xen_deny(void) return "<denied>"; } +static const char build_info[] = + "debug=" +#ifdef CONFIG_DEBUG + "y" +#else + "n" +#endif +#ifdef CONFIG_COVERAGE +# ifdef __clang__ + " llvmcov=y" +# else + " gcov=y" +# endif +#endif +#ifdef CONFIG_UBSAN + " ubsan=y" +#endif + ""; + +const char *xen_build_info(void) +{ + return build_info; +} + static const void *build_id_p __read_mostly; static unsigned int build_id_len __read_mostly; --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -1002,10 +1002,10 @@ void __init console_init_preirq(void) spin_lock(&console_lock); __putstr(xen_banner()); spin_unlock(&console_lock); - printk("Xen version %d.%d%s (%s@%s) (%s) debug=%c " gcov_string " %s\n", + printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n", xen_major_version(), xen_minor_version(), xen_extra_version(), - xen_compile_by(), xen_compile_domain(), - xen_compiler(), debug_build() ? 'y' : 'n', xen_compile_date()); + xen_compile_by(), xen_compile_domain(), xen_compiler(), + xen_build_info(), xen_compile_date()); printk("Latest ChangeSet: %s\n", xen_changeset()); /* Locate and print the buildid, if applicable. */ --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -48,21 +48,13 @@ #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond)) #endif -#ifdef CONFIG_GCOV -#define gcov_string "gcov=y" -#else -#define gcov_string "" -#endif - #ifndef NDEBUG #define ASSERT(p) \ do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0) #define ASSERT_UNREACHABLE() assert_failed("unreachable") -#define debug_build() 1 #else #define ASSERT(p) do { if ( 0 && (p) ) {} } while (0) #define ASSERT_UNREACHABLE() do { } while (0) -#define debug_build() 0 #endif #define ABS(_x) ({ \ --- a/xen/include/xen/version.h +++ b/xen/include/xen/version.h @@ -16,6 +16,7 @@ const char *xen_extra_version(void); const char *xen_changeset(void); const char *xen_banner(void); const char *xen_deny(void); +const char *xen_build_info(void); int xen_build_id(const void **p, unsigned int *len); #ifdef BUILD_ID
Its expansion shouldn't be tied to NDEBUG - down the road we may want to allow enabling assertions independently of CONFIG_DEBUG. Replace the few uses by a new xen_build_info() helper, subsuming gcov_string at the same time (while replacing the stale CONFIG_GCOV used there) and also adding CONFIG_UBSAN indication. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: Introduce xen_build_info() including also gcov and ubsan info.