Re: [PATCH] bug: Fix no-return-statement warning with !CONFIG_BUG

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 15/04/2024 à 17:35, Arnd Bergmann a écrit :
> On Mon, Apr 15, 2024, at 04:19, Michael Ellerman wrote:
>> "Arnd Bergmann" <arnd@arndb.de> writes:
>>> On Thu, Apr 11, 2024, at 11:27, Adrian Hunter wrote:
>>>> On 11/04/24 11:22, Christophe Leroy wrote:
>>>>
>>>> That is fragile because it depends on defined(__OPTIMIZE__),
>>>> so it should still be:
>>>
>>> If there is a function that is defined but that must never be
>>> called, I think we are doing something wrong.
>>
>> It's a pretty inevitable result of using IS_ENABLED(), which the docs
>> encourage people to use.
> 
> Using IS_ENABLED() is usually a good idea, as it helps avoid
> adding extra #ifdef checks and just drops static functions as
> dead code, or lets you call extern functions that are conditionally
> defined in a different file.
> 
> The thing is that here it does not do either of those and
> adds more complexity than it avoids.
> 
>> In this case it could easily be turned into a build error by just making
>> it an extern rather than a static inline.
>>
>> But I think Christophe's solution is actually better, because it's more
>> explicit, ie. this function should not be called and if it is that's a
>> build time error.
> 
> I haven't seen a good solution here. Ideally we'd just define
> the functions unconditionally and have IS_ENABLED() take care
> of letting the compiler drop them silently, but that doesn't
> build because of missing struct members.
> 
> I won't object to either an 'extern' declaration or the
> 'BUILD_BUG_ON()' if you and others prefer that, both are better
> than BUG() here. I still think my suggestion would be a little
> simpler.

The advantage of the BUILD_BUG() against the extern is that the error 
gets detected at buildtime. With the extern it gets detected only at 
link-time.

But agree with you, the missing struct members defeats the advantages of 
IS_ENABLED().

At the end, how many instances of struct timekeeper do we have in the 
system ? With a quick look I see only two instances: tkcore.timekeeper 
and shadow_timekeeper. If I'm correct, wouldn't it just be simpler to 
have the three debug struct members defined at all time ?

Christophe