| Message ID | 1463893295-2610-2-git-send-email-cardoe@cardoe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5/22/16 12:01 AM, Doug Goldstein wrote: > Enabling debug will disable NDEBUG which will result in more debug > prints. There are a number of debugging options for Xen so place the > debug option under a menu for different debugging options to have a way > to group them all together. > > Signed-off-by: Doug Goldstein <cardoe@cardoe.com> ... snip ... > diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug > new file mode 100644 > index 0000000..b446027 > --- /dev/null > +++ b/xen/Kconfig.debug > @@ -0,0 +1,13 @@ > + > +menu "Debugging Options" > + > +config DEBUG > + bool "Developer Checks" Add the following when committing: default y if you want debug to be on by default. > + ---help--- > + Enables developer checks such as asserts and extra printks, this > + option is intended for development purposes only, and not for > + production use. > + > + You probably want to say 'N' here.
>>> On 22.05.16 at 21:04, <cardoe@cardoe.com> wrote: > On 5/22/16 12:01 AM, Doug Goldstein wrote: >> --- /dev/null >> +++ b/xen/Kconfig.debug >> @@ -0,0 +1,13 @@ >> + >> +menu "Debugging Options" >> + >> +config DEBUG >> + bool "Developer Checks" > > Add the following when committing: > > default y > > if you want debug to be on by default. Indeed we definitely want this in other than stable (or late -rc) trees. Jan
>>> On 22.05.16 at 07:01, <cardoe@cardoe.com> wrote: > --- /dev/null > +++ b/xen/Kconfig.debug > @@ -0,0 +1,13 @@ > + > +menu "Debugging Options" > + > +config DEBUG > + bool "Developer Checks" > + ---help--- > + Enables developer checks such as asserts and extra printks, this > + option is intended for development purposes only, and not for > + production use. I'm not a native speaker, but it seems to me that either this wants to start "Enabling ..." or the first comma wants to become a stop. > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -20,13 +20,14 @@ include $(XEN_ROOT)/Config.mk > ifeq ($(debug),y) This being left in place suggests ... > verbose := y > frame_pointer := y > -else > -CFLAGS += -DNDEBUG > endif > ifeq ($(perfc_arrays),y) > perfc := y > endif > > +ifeq ($(origin debug),command line) > +$(error "You must use 'make menuconfig' to enable/disable debug now.") > +endif ... that this is at least premature. "At least" because: Didn't you or someone else point out that this is then still used for controlling the tool stack part of the build? Jan
On 23/05/16 13:58, Jan Beulich wrote: >>>> On 22.05.16 at 07:01, <cardoe@cardoe.com> wrote: >> --- /dev/null >> +++ b/xen/Kconfig.debug >> @@ -0,0 +1,13 @@ >> + >> +menu "Debugging Options" >> + >> +config DEBUG >> + bool "Developer Checks" >> + ---help--- >> + Enables developer checks such as asserts and extra printks, this >> + option is intended for development purposes only, and not for >> + production use. > I'm not a native speaker, but it seems to me that either this wants to > start "Enabling ..." or the first comma wants to become a stop. The first comma should become a full stop. Changing Enables to Enabling still results in a particularly odd sentence. ~Andrew
On 5/23/16 7:58 AM, Jan Beulich wrote: >>>> On 22.05.16 at 07:01, <cardoe@cardoe.com> wrote: >> verbose := y >> frame_pointer := y >> -else >> -CFLAGS += -DNDEBUG >> endif >> ifeq ($(perfc_arrays),y) >> perfc := y >> endif >> >> +ifeq ($(origin debug),command line) >> +$(error "You must use 'make menuconfig' to enable/disable debug now.") >> +endif > > ... that this is at least premature. "At least" because: Didn't you or > someone else point out that this is then still used for controlling the > tool stack part of the build? > > Jan > I purposefully just check for "command line" in this case. So if someone does "make -C xen debug=[y|n]" or "cd xen && make debug=[y|n]" then that should be the only time they get that message. The other cases you'll see the check is different and would catch the debug value from the top level. If that's not what's intended please let me know and I can strip this bit out. You had mentioned the 'debug' option in http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg01027.html which is why I tried to come up with a message that I thought would be more limited and then hopefully not premature.
On 5/23/16 8:19 AM, Andrew Cooper wrote: > On 23/05/16 13:58, Jan Beulich wrote: >>>>> On 22.05.16 at 07:01, <cardoe@cardoe.com> wrote: >>> --- /dev/null >>> +++ b/xen/Kconfig.debug >>> @@ -0,0 +1,13 @@ >>> + >>> +menu "Debugging Options" >>> + >>> +config DEBUG >>> + bool "Developer Checks" >>> + ---help--- >>> + Enables developer checks such as asserts and extra printks, this >>> + option is intended for development purposes only, and not for >>> + production use. >> I'm not a native speaker, but it seems to me that either this wants to >> start "Enabling ..." or the first comma wants to become a stop. > > The first comma should become a full stop. > > Changing Enables to Enabling still results in a particularly odd sentence. > > ~Andrew > Looks like I glued together a few suggestions between the different reposts and never really read it. This is what I've got now: If you say Y here this will enable developer checks such as asserts and extra printks. This option is intended for development purposes only, and not for production use. You probably want to say 'N' here.
On 5/23/16 3:39 AM, Jan Beulich wrote: >>>> On 22.05.16 at 21:04, <cardoe@cardoe.com> wrote: >> On 5/22/16 12:01 AM, Doug Goldstein wrote: >>> --- /dev/null >>> +++ b/xen/Kconfig.debug >>> @@ -0,0 +1,13 @@ >>> + >>> +menu "Debugging Options" >>> + >>> +config DEBUG >>> + bool "Developer Checks" >> >> Add the following when committing: >> >> default y >> >> if you want debug to be on by default. > > Indeed we definitely want this in other than stable (or late -rc) trees. > > Jan > Since we're now for sure sticking this into 4.8, I've squashed that in.
>>> On 24.05.16 at 03:03, <cardoe@cardoe.com> wrote: > On 5/23/16 7:58 AM, Jan Beulich wrote: >>>>> On 22.05.16 at 07:01, <cardoe@cardoe.com> wrote: > >>> verbose := y >>> frame_pointer := y >>> -else >>> -CFLAGS += -DNDEBUG >>> endif >>> ifeq ($(perfc_arrays),y) >>> perfc := y >>> endif >>> >>> +ifeq ($(origin debug),command line) >>> +$(error "You must use 'make menuconfig' to enable/disable debug now.") >>> +endif >> >> ... that this is at least premature. "At least" because: Didn't you or >> someone else point out that this is then still used for controlling the >> tool stack part of the build? > > I purposefully just check for "command line" in this case. So if someone > does "make -C xen debug=[y|n]" or "cd xen && make debug=[y|n]" then that > should be the only time they get that message. But doesn't a "debug=[y|n]" on a top level make command line trickle down as a command line variable too? In which case your check would prevent someone forcing that setting to a specific value for the tools (and other?) subtree(s). > The other cases you'll see the check is different and would catch the > debug value from the top level. > > If that's not what's intended please let me know and I can strip this > bit out. You had mentioned the 'debug' option in > http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg01027.html > which is why I tried to come up with a message that I thought would be > more limited and then hopefully not premature. Well, yes, I did mention debug there, but iirc the discussion about debug having a meaning outside of xen/ was on another subthread, and needs to be taken into account here. Bottom line - I'd be fine with the check left in place if indeed it won't harm any possible top level make invocation. Otherwise I think this would at least need to be relaxed to $(warning ...). Jan
diff --git a/xen/Kconfig b/xen/Kconfig index fa8b27c..0fe7a1a 100644 --- a/xen/Kconfig +++ b/xen/Kconfig @@ -26,3 +26,5 @@ config DEFCONFIG_LIST config EXPERT string option env="XEN_CONFIG_EXPERT" + +source "Kconfig.debug" diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug new file mode 100644 index 0000000..b446027 --- /dev/null +++ b/xen/Kconfig.debug @@ -0,0 +1,13 @@ + +menu "Debugging Options" + +config DEBUG + bool "Developer Checks" + ---help--- + Enables developer checks such as asserts and extra printks, this + option is intended for development purposes only, and not for + production use. + + You probably want to say 'N' here. + +endmenu diff --git a/xen/Rules.mk b/xen/Rules.mk index 961d533..86c1e0d 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -20,13 +20,14 @@ include $(XEN_ROOT)/Config.mk ifeq ($(debug),y) verbose := y frame_pointer := y -else -CFLAGS += -DNDEBUG endif ifeq ($(perfc_arrays),y) perfc := y endif +ifeq ($(origin debug),command line) +$(error "You must use 'make menuconfig' to enable/disable debug now.") +endif ifneq ($(origin kexec),undefined) $(error "You must use 'make menuconfig' to enable/disable kexec now.") endif diff --git a/xen/include/xen/config.h b/xen/include/xen/config.h index ef6e5ee..473c5e8 100644 --- a/xen/include/xen/config.h +++ b/xen/include/xen/config.h @@ -81,4 +81,8 @@ /* allow existing code to work with Kconfig variable */ #define NR_CPUS CONFIG_NR_CPUS +#ifndef CONFIG_DEBUG +#define NDEBUG +#endif + #endif /* __XEN_CONFIG_H__ */
Enabling debug will disable NDEBUG which will result in more debug prints. There are a number of debugging options for Xen so place the debug option under a menu for different debugging options to have a way to group them all together. Signed-off-by: Doug Goldstein <cardoe@cardoe.com> --- CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Tim Deegan <tim@xen.org> CC: Wei Liu <wei.liu2@citrix.com> --- xen/Kconfig | 2 ++ xen/Kconfig.debug | 13 +++++++++++++ xen/Rules.mk | 5 +++-- xen/include/xen/config.h | 4 ++++ 4 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 xen/Kconfig.debug