Re: [Xenomai-core] [RFC] fix XENO_OPT_DEBUG bugs.

[Philippe Gerum <rpm@xenomai.org>]

On Mon, 2010-04-19 at 18:14 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Mon, 2010-04-19 at 17:58 +0200, Jan Kiszka wrote:
> >> Gilles Chanteperdrix wrote:
> >>> Jan Kiszka wrote:
> >>>> Gilles Chanteperdrix wrote:
> >>>>> Jan Kiszka wrote:
> >>>>>> Gilles Chanteperdrix wrote:
> >>>>>>> Jan Kiszka wrote:
> >>>>>>>> Gilles Chanteperdrix wrote:
> >>>>>>>>> Hi,
> >>>>>>>>>
> >>>>>>>>> I found some code which was referencing directly some
> >>>>>>>>> CONFIG_XENO_OPT_DEBUG_ variables with things like:
> >>>>>>>>>
> >>>>>>>>> #ifdef CONFIG_XENO_OPT_DEBUG_FOO
> >>>>>>>>>
> >>>>>>>>> This usage is incompatible with the pre-requisites of the assert.h
> >>>>>>>>> header that CONFIG_XENO_OPT_DEBUG_FOO should be defined at all times.
> >>>>>>>>> While grepping for CONFIG_XENO_OPT_DEBUG_, I found that we also have
> >>>>>>>>> many duplicates of construction like:
> >>>>>>>>> #ifndef CONFIG_XENO_OPT_DEBUG_FOO
> >>>>>>>>> #define CONFIG_XENO_OPT_DEBUG_FOO 0
> >>>>>>>>> #endif /* CONFIG_XENO_OPT_DEBUG_FOO */
> >>>>>>>>>
> >>>>>>>>> So, a patch follows which:
> >>>>>>>>> - replace the #ifdef with some #if XENO_DEBUG(FOO)
> >>>>>>>> Should probably come as a separate patch.
> >>>>>>> Come on, the patch is simple, one patch for this is enough.
> >>>>>> One part is an obvious fix, the other a refactoring under discussion.
> >>>>>>
> >>>>>>>>> - move all the initializations to assert.h
> >>>>>>>>>
> >>>>>>>>> This will make any reference to CONFIG_XENO_OPT_DEBUG_FOO outside of
> >>>>>>>>> assert.h suspicious, and easy to detect.
> >>>>>>>> How many duplicates did you find?
> >>>>>>> A lot, especially for CONFIG_XENO_OPT_DEBUG_NUCLEUS
> >>>>>> That's because the CONFIG_XENO_OPT_DEBUG_NUCLEUS defines were misplaced.
> >>>>>> Besides that we had one CONFIG_XENO_OPT_DEBUG_QUEUE duplicate, but tons
> >>>>>> of proper users. I don't see such an immediate need.
> >>>>>>
> >>>>>> When adding this kind of switching, it's till more handy to have things
> >>>>>> locally in your subsystem. That also makes reviewing easier when you
> >>>>>> only find changes in files that belong to a subsystem.
> >>>>> That is not the main point, the main point is that putting all these
> >>>>> defines in one files allow to detect easily direct references to the
> >>>>> symbols.
> >>>>>
> >>>>>>>> Generally, I'm more a fan of decentralized management here (e.g. this
> >>>>>>>> avoids needless patch conflicts in central files).
> >>>>>>> If we maintain the list in alphabetical order (which I have done), we 
> >>>>>>> reduce the likeliness for conflicts. The aim of doing this is also that 
> >>>>>>> I can check that the sources are clean with:
> >>>>>>>
> >>>>>>> find xenomai-2.5 ! -name 'assert.h' -name '*.[ch]' | xargs grep CONFIG_XENO_OPT_DEBUG_
> >>>>>>>
> >>>>>>> And that I can add this test to the automated build test.
> >>>>>>>
> >>>>>>> Note that forgetting to add a #define to the list yields an immediate
> >>>>>>> compilation error. So, the patch makes things completely safe.
> >>>>>> What did you change to make this happen (for new users of XENO_DEBUG)?
> >>>>> Nothing. If you forget to add the define, and do not enable the debug
> >>>>> option (which everybody does most of the time), you get a compilation
> >>>>> error.
> >>>> The alternative (and decentralized) approach for fixing this consists of
> >>>> Kconfig magic for generating the value:
> >>>>
> >>>> config XENO_OPT_DEBUG_FOO
> >>>> 	bool "..."
> >>>>
> >>>> config XENO_OPT_DEBUG_FOO_P
> >>>> 	int
> >>>> 	default "1" if XENO_OPT_DEBUG_FOO
> >>>> 	default "0"
> >>>>
> >>>> and XENO_DEBUG() could be extended to test for
> >>>> CONFIG_XENO_OPT_DEBUG_FOO_P when given "FOO". I'm just not sure if this
> >>>> can be expressed for legacy 2.4 kernels, so it might have to wait for
> >>>> Xenomai 3.
> > 
> > Well, actually, I would not merge this in Xenomai 3. I find this rather
> > overkill; mainline first I mean, and mainline, i.e. the Xenomai code
> > base only requires a simple and straightforward way to get debug
> > switches right. Having to make Kconfig a kitchen sink for some unknown
> > out of tree modules to be happy is not really my preferred approach in
> > this particular case.
> > 
> > Don't get me wrong, I'm not opposed to a more decentralized approach on
> > the paper, it's just that I only care about the mainline tree here.
> 
> The point is not out-of-tree but robustness. Neither the current
> decentralized #ifdef-#define nor its centralized brother meet this
> criteria. An approach like the above which forces you to provide all
> required bits before any of the cases (disabled/enabled) starts to work
> does so.

Flexibility and robustness have to be combined. Explicit declaration in
Kconfig is against flexibility, because this is one external thing more
to take care of, for adding something as simple as a debug switch. So if
decentralized is not important to you, then let's stop discussing about
this, and find a centralized way that includes robustness, to avoid
untested paths. IIRC, this was precisely the essence of Gilles's initial
proposal.

> 
> Jan
> 


-- 
Philippe.