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

[Jan Kiszka <jan.kiszka@domain.hid>]

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.
> 
> 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