From mboxrd@z Thu Jan 1 00:00:00 1970 From: Philippe Gerum In-Reply-To: <4BCC7D88.4070403@domain.hid> References: <4BCC619E.2@domain.hid> <4BCC6CEE.70907@domain.hid> <4BCC6E3C.3090301@domain.hid> <4BCC7092.1030809@domain.hid> <4BCC71AF.4030903@domain.hid> <4BCC77BD.9040900@domain.hid> <4BCC78D5.3010405@domain.hid> <4BCC7D88.4070403@domain.hid> Content-Type: text/plain; charset="UTF-8" Date: Mon, 19 Apr 2010 18:10:11 +0200 Message-ID: <1271693411.16659.128.camel@domain.hid> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai-core] [RFC] fix XENO_OPT_DEBUG bugs. List-Id: Xenomai life and development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: xenomai-core 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. > > We can do something like that. But I still find this more complicated > > than simply moving three lines to assert.h. > > It requires one more line but provides the safety the current approach > lacks - not worth it? > > > > > What is your problem exactly? Do you have some customer code which > > defines some more debug symbols? In that case there is no problem, you > > can keep the code as is. I plan to do the debug check part of the build > > test, not part of the makefiles. > > I have no such problems or concerns. I just find the centralization an > unclean workaround for the actual issue. > > Jan > > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux > > _______________________________________________ > Xenomai-core mailing list > Xenomai-core@domain.hid > https://mail.gna.org/listinfo/xenomai-core -- Philippe.