| Message ID | 20240410153212.127477-1-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | bug: Fix no-return-statement warning with !CONFIG_BUG | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
On Wed, 10 Apr 2024 at 21:02, Adrian Hunter <adrian.hunter@intel.com> wrote: > > BUG() does not return, and arch implementations of BUG() use unreachable() > or other non-returning code. However with !CONFIG_BUG, the default > implementation is often used instead, and that does not do that. x86 always > uses its own implementation, but powerpc with !CONFIG_BUG gives a build > error: > > kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: > kernel/time/timekeeping.c:286:1: error: no return statement in function > returning non-void [-Werror=return-type] > > Add unreachable() to default !CONFIG_BUG BUG() implementation. > > Fixes: e8e9d21a5df6 ("timekeeping: Refactor timekeeping helpers") > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > Closes: https://lore.kernel.org/all/CA+G9fYvjdZCW=7ZGxS6A_3bysjQ56YF7S-+PNLQ_8a4DKh1Bhg@mail.gmail.com/ > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> This patch applied on top of today's Linux next-20240410 tag and build test pass. Tested-by: Linux Kernel Functional Testing <lkft@linaro.org> > --- > include/asm-generic/bug.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) -- Linaro LKFT https://lkft.linaro.org
On Wed, Apr 10, 2024, at 17:32, Adrian Hunter wrote: > BUG() does not return, and arch implementations of BUG() use unreachable() > or other non-returning code. However with !CONFIG_BUG, the default > implementation is often used instead, and that does not do that. x86 always > uses its own implementation, but powerpc with !CONFIG_BUG gives a build > error: > > kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: > kernel/time/timekeeping.c:286:1: error: no return statement in function > returning non-void [-Werror=return-type] > > Add unreachable() to default !CONFIG_BUG BUG() implementation. I'm a bit worried about this patch, since we have had problems with unreachable() inside of BUG() in the past, and as far as I can remember, the current version was the only one that actually did the right thing on all compilers. One problem with an unreachable() annotation here is that if a compiler misanalyses the endless loop, it can decide to throw out the entire code path leading up to it and just run into undefined behavior instead of printing a BUG() message. Do you know which compiler version show the warning above? Arnd
On 11/04/24 10:04, Arnd Bergmann wrote: > On Wed, Apr 10, 2024, at 17:32, Adrian Hunter wrote: >> BUG() does not return, and arch implementations of BUG() use unreachable() >> or other non-returning code. However with !CONFIG_BUG, the default >> implementation is often used instead, and that does not do that. x86 always >> uses its own implementation, but powerpc with !CONFIG_BUG gives a build >> error: >> >> kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: >> kernel/time/timekeeping.c:286:1: error: no return statement in function >> returning non-void [-Werror=return-type] >> >> Add unreachable() to default !CONFIG_BUG BUG() implementation. > > I'm a bit worried about this patch, since we have had problems > with unreachable() inside of BUG() in the past, and as far as I > can remember, the current version was the only one that > actually did the right thing on all compilers. > > One problem with an unreachable() annotation here is that if > a compiler misanalyses the endless loop, it can decide to > throw out the entire code path leading up to it and just > run into undefined behavior instead of printing a BUG() > message. > > Do you know which compiler version show the warning above? Original report has a list https://lore.kernel.org/all/CA+G9fYvjdZCW=7ZGxS6A_3bysjQ56YF7S-+PNLQ_8a4DKh1Bhg@mail.gmail.com/
On Thu, Apr 11, 2024, at 09:16, Adrian Hunter wrote: > On 11/04/24 10:04, Arnd Bergmann wrote: >> On Wed, Apr 10, 2024, at 17:32, Adrian Hunter wrote: >>> BUG() does not return, and arch implementations of BUG() use unreachable() >>> or other non-returning code. However with !CONFIG_BUG, the default >>> implementation is often used instead, and that does not do that. x86 always >>> uses its own implementation, but powerpc with !CONFIG_BUG gives a build >>> error: >>> >>> kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: >>> kernel/time/timekeeping.c:286:1: error: no return statement in function >>> returning non-void [-Werror=return-type] >>> >>> Add unreachable() to default !CONFIG_BUG BUG() implementation. >> >> I'm a bit worried about this patch, since we have had problems >> with unreachable() inside of BUG() in the past, and as far as I >> can remember, the current version was the only one that >> actually did the right thing on all compilers. >> >> One problem with an unreachable() annotation here is that if >> a compiler misanalyses the endless loop, it can decide to >> throw out the entire code path leading up to it and just >> run into undefined behavior instead of printing a BUG() >> message. >> >> Do you know which compiler version show the warning above? > > Original report has a list > It looks like it's all versions of gcc, though no versions of clang show the warnings. I did a few more tests and could not find any differences on actual code generation, but I'd still feel more comfortable changing the caller than the BUG() macro. It's trivial to add a 'return 0' there. Another interesting observation is that clang-11 and earlier versions end up skipping the endless loop, both with and without the __builtin_unreachable, see https://godbolt.org/z/aqa9zqz8x clang-12 and above do work like gcc, so I guess that is good. Arnd
Le 11/04/2024 à 09:16, Adrian Hunter a écrit : > On 11/04/24 10:04, Arnd Bergmann wrote: >> On Wed, Apr 10, 2024, at 17:32, Adrian Hunter wrote: >>> BUG() does not return, and arch implementations of BUG() use unreachable() >>> or other non-returning code. However with !CONFIG_BUG, the default >>> implementation is often used instead, and that does not do that. x86 always >>> uses its own implementation, but powerpc with !CONFIG_BUG gives a build >>> error: >>> >>> kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: >>> kernel/time/timekeeping.c:286:1: error: no return statement in function >>> returning non-void [-Werror=return-type] >>> >>> Add unreachable() to default !CONFIG_BUG BUG() implementation. >> >> I'm a bit worried about this patch, since we have had problems >> with unreachable() inside of BUG() in the past, and as far as I >> can remember, the current version was the only one that >> actually did the right thing on all compilers. >> >> One problem with an unreachable() annotation here is that if >> a compiler misanalyses the endless loop, it can decide to >> throw out the entire code path leading up to it and just >> run into undefined behavior instead of printing a BUG() >> message. >> >> Do you know which compiler version show the warning above? > > Original report has a list > > https://lore.kernel.org/all/CA+G9fYvjdZCW=7ZGxS6A_3bysjQ56YF7S-+PNLQ_8a4DKh1Bhg@mail.gmail.com/ > Looking at the report, I think the correct fix should be to use BUILD_BUG() instead of BUG()
Le 11/04/2024 à 10:12, Christophe Leroy a écrit : > > > Le 11/04/2024 à 09:16, Adrian Hunter a écrit : >> On 11/04/24 10:04, Arnd Bergmann wrote: >>> On Wed, Apr 10, 2024, at 17:32, Adrian Hunter wrote: >>>> BUG() does not return, and arch implementations of BUG() use >>>> unreachable() >>>> or other non-returning code. However with !CONFIG_BUG, the default >>>> implementation is often used instead, and that does not do that. x86 >>>> always >>>> uses its own implementation, but powerpc with !CONFIG_BUG gives a build >>>> error: >>>> >>>> kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: >>>> kernel/time/timekeeping.c:286:1: error: no return statement in >>>> function >>>> returning non-void [-Werror=return-type] >>>> >>>> Add unreachable() to default !CONFIG_BUG BUG() implementation. >>> >>> I'm a bit worried about this patch, since we have had problems >>> with unreachable() inside of BUG() in the past, and as far as I >>> can remember, the current version was the only one that >>> actually did the right thing on all compilers. >>> >>> One problem with an unreachable() annotation here is that if >>> a compiler misanalyses the endless loop, it can decide to >>> throw out the entire code path leading up to it and just >>> run into undefined behavior instead of printing a BUG() >>> message. >>> >>> Do you know which compiler version show the warning above? >> >> Original report has a list >> >> https://lore.kernel.org/all/CA+G9fYvjdZCW=7ZGxS6A_3bysjQ56YF7S-+PNLQ_8a4DKh1Bhg@mail.gmail.com/ >> > > Looking at the report, I think the correct fix should be to use > BUILD_BUG() instead of BUG() I confirm the error goes away with the following change to next-20240411 on powerpc tinyconfig with gcc 13.2 diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 4e18db1819f8..3d5ac0cdd721 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -282,7 +282,7 @@ static inline void timekeeping_check_update(struct timekeeper *tk, u64 offset) } static inline u64 timekeeping_debug_get_ns(const struct tk_read_base *tkr) { - BUG(); + BUILD_BUG(); } #endif
On 11/04/24 10:56, Arnd Bergmann wrote: > On Thu, Apr 11, 2024, at 09:16, Adrian Hunter wrote: >> On 11/04/24 10:04, Arnd Bergmann wrote: >>> On Wed, Apr 10, 2024, at 17:32, Adrian Hunter wrote: >>>> BUG() does not return, and arch implementations of BUG() use unreachable() >>>> or other non-returning code. However with !CONFIG_BUG, the default >>>> implementation is often used instead, and that does not do that. x86 always >>>> uses its own implementation, but powerpc with !CONFIG_BUG gives a build >>>> error: >>>> >>>> kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: >>>> kernel/time/timekeeping.c:286:1: error: no return statement in function >>>> returning non-void [-Werror=return-type] >>>> >>>> Add unreachable() to default !CONFIG_BUG BUG() implementation. >>> >>> I'm a bit worried about this patch, since we have had problems >>> with unreachable() inside of BUG() in the past, and as far as I >>> can remember, the current version was the only one that >>> actually did the right thing on all compilers. >>> >>> One problem with an unreachable() annotation here is that if >>> a compiler misanalyses the endless loop, it can decide to >>> throw out the entire code path leading up to it and just >>> run into undefined behavior instead of printing a BUG() >>> message. >>> >>> Do you know which compiler version show the warning above? >> >> Original report has a list >> > > It looks like it's all versions of gcc, though no versions > of clang show the warnings. I did a few more tests and could > not find any differences on actual code generation, but > I'd still feel more comfortable changing the caller than > the BUG() macro. It's trivial to add a 'return 0' there. AFAICT every implementation of BUG() except this one has unreachable() or equivalent, so that inconsistency seems wrong. Could add 'return 0', but I do notice other cases where a function does not have a return value, such as cpus_have_final_boot_cap(), so there is already an expectation that that is OK. > Another interesting observation is that clang-11 and earlier > versions end up skipping the endless loop, both with and > without the __builtin_unreachable, see > https://godbolt.org/z/aqa9zqz8x Adding volatile asm("") to the loop would probably fix that, but it seems like a separate issue. > > clang-12 and above do work like gcc, so I guess that is good. > > Arnd
On 11/04/24 11:22, Christophe Leroy wrote: > > > Le 11/04/2024 à 10:12, Christophe Leroy a écrit : >> >> >> Le 11/04/2024 à 09:16, Adrian Hunter a écrit : >>> On 11/04/24 10:04, Arnd Bergmann wrote: >>>> On Wed, Apr 10, 2024, at 17:32, Adrian Hunter wrote: >>>>> BUG() does not return, and arch implementations of BUG() use >>>>> unreachable() >>>>> or other non-returning code. However with !CONFIG_BUG, the default >>>>> implementation is often used instead, and that does not do that. x86 >>>>> always >>>>> uses its own implementation, but powerpc with !CONFIG_BUG gives a build >>>>> error: >>>>> >>>>> kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: >>>>> kernel/time/timekeeping.c:286:1: error: no return statement in >>>>> function >>>>> returning non-void [-Werror=return-type] >>>>> >>>>> Add unreachable() to default !CONFIG_BUG BUG() implementation. >>>> >>>> I'm a bit worried about this patch, since we have had problems >>>> with unreachable() inside of BUG() in the past, and as far as I >>>> can remember, the current version was the only one that >>>> actually did the right thing on all compilers. >>>> >>>> One problem with an unreachable() annotation here is that if >>>> a compiler misanalyses the endless loop, it can decide to >>>> throw out the entire code path leading up to it and just >>>> run into undefined behavior instead of printing a BUG() >>>> message. >>>> >>>> Do you know which compiler version show the warning above? >>> >>> Original report has a list >>> >>> https://lore.kernel.org/all/CA+G9fYvjdZCW=7ZGxS6A_3bysjQ56YF7S-+PNLQ_8a4DKh1Bhg@mail.gmail.com/ >>> >> >> Looking at the report, I think the correct fix should be to use >> BUILD_BUG() instead of BUG() > > I confirm the error goes away with the following change to next-20240411 > on powerpc tinyconfig with gcc 13.2 > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c > index 4e18db1819f8..3d5ac0cdd721 100644 > --- a/kernel/time/timekeeping.c > +++ b/kernel/time/timekeeping.c > @@ -282,7 +282,7 @@ static inline void timekeeping_check_update(struct > timekeeper *tk, u64 offset) > } > static inline u64 timekeeping_debug_get_ns(const struct tk_read_base *tkr) > { > - BUG(); > + BUILD_BUG(); > } > #endif > That is fragile because it depends on defined(__OPTIMIZE__), so it should still be: BUILD_BUG(); return 0;
From: Adrian Hunter > Sent: 11 April 2024 10:04 > > On 11/04/24 10:56, Arnd Bergmann wrote: > > On Thu, Apr 11, 2024, at 09:16, Adrian Hunter wrote: > >> On 11/04/24 10:04, Arnd Bergmann wrote: > >>> On Wed, Apr 10, 2024, at 17:32, Adrian Hunter wrote: > >>>> BUG() does not return, and arch implementations of BUG() use unreachable() > >>>> or other non-returning code. However with !CONFIG_BUG, the default > >>>> implementation is often used instead, and that does not do that. x86 always > >>>> uses its own implementation, but powerpc with !CONFIG_BUG gives a build > >>>> error: > >>>> > >>>> kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: > >>>> kernel/time/timekeeping.c:286:1: error: no return statement in function > >>>> returning non-void [-Werror=return-type] > >>>> > >>>> Add unreachable() to default !CONFIG_BUG BUG() implementation. > >>> > >>> I'm a bit worried about this patch, since we have had problems > >>> with unreachable() inside of BUG() in the past, and as far as I > >>> can remember, the current version was the only one that > >>> actually did the right thing on all compilers. > >>> > >>> One problem with an unreachable() annotation here is that if > >>> a compiler misanalyses the endless loop, it can decide to > >>> throw out the entire code path leading up to it and just > >>> run into undefined behavior instead of printing a BUG() > >>> message. > >>> > >>> Do you know which compiler version show the warning above? > >> > >> Original report has a list > >> > > > > It looks like it's all versions of gcc, though no versions > > of clang show the warnings. I did a few more tests and could > > not find any differences on actual code generation, but > > I'd still feel more comfortable changing the caller than > > the BUG() macro. It's trivial to add a 'return 0' there. > > AFAICT every implementation of BUG() except this one has > unreachable() or equivalent, so that inconsistency seems > wrong. > > Could add 'return 0', but I do notice other cases > where a function does not have a return value, such as > cpus_have_final_boot_cap(), so there is already an expectation > that that is OK. Isn't that likely to generate an 'unreachable code' warning? I any case the compiler can generate better code for the non-BUG() path if it knows BUG() doesn't return. (And confuse stack back-trace code in the process.) > > > Another interesting observation is that clang-11 and earlier > > versions end up skipping the endless loop, both with and > > without the __builtin_unreachable, see > > https://godbolt.org/z/aqa9zqz8x > > Adding volatile asm("") to the loop would probably fix that, > but it seems like a separate issue. I'd guess barrier() would be better. It might be needed before the loopstop for other reasons. The compiler might be just moving code below the loop and then discarding it as unreachable. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Apr 11, 2024, at 11:27, Adrian Hunter wrote: > On 11/04/24 11:22, Christophe Leroy wrote: >> Le 11/04/2024 à 10:12, Christophe Leroy a écrit : >>> >>> Looking at the report, I think the correct fix should be to use >>> BUILD_BUG() instead of BUG() >> >> I confirm the error goes away with the following change to next-20240411 >> on powerpc tinyconfig with gcc 13.2 >> >> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c >> index 4e18db1819f8..3d5ac0cdd721 100644 >> --- a/kernel/time/timekeeping.c >> +++ b/kernel/time/timekeeping.c >> @@ -282,7 +282,7 @@ static inline void timekeeping_check_update(struct >> timekeeper *tk, u64 offset) >> } >> static inline u64 timekeeping_debug_get_ns(const struct tk_read_base *tkr) >> { >> - BUG(); >> + BUILD_BUG(); >> } >> #endif >> > > 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. Before e8e9d21a5df6 ("timekeeping: Refactor timekeeping helpers"), the #ifdef made some sense, but now the #else is not really that useful. Ideally we would make timekeeping_debug_get_delta() and timekeeping_check_update() just return in case of !IS_ENABLED(CONFIG_DEBUG_TIMEKEEPING), but unfortunately the code uses some struct members that are undefined then. The patch below moves the #ifdef check into these functions, which is not great, but it avoids defining useless functions. Maybe there is a better way here. How about just removing the BUG()? Arnd diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 4e18db1819f8..16c6dba64dd6 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -195,12 +195,11 @@ static inline u64 tk_clock_read(const struct tk_read_base *tkr) return clock->read(clock); } -#ifdef CONFIG_DEBUG_TIMEKEEPING #define WARNING_FREQ (HZ*300) /* 5 minute rate-limiting */ static void timekeeping_check_update(struct timekeeper *tk, u64 offset) { - +#ifdef CONFIG_DEBUG_TIMEKEEPING u64 max_cycles = tk->tkr_mono.clock->max_cycles; const char *name = tk->tkr_mono.clock->name; @@ -235,12 +234,19 @@ static void timekeeping_check_update(struct timekeeper *tk, u64 offset) } tk->overflow_seen = 0; } +#endif } static inline u64 timekeeping_cycles_to_ns(const struct tk_read_base *tkr, u64 cycles); -static inline u64 timekeeping_debug_get_ns(const struct tk_read_base *tkr) +static u64 __timekeeping_get_ns(const struct tk_read_base *tkr) +{ + return timekeeping_cycles_to_ns(tkr, tk_clock_read(tkr)); +} + +static inline u64 timekeeping_get_ns(const struct tk_read_base *tkr) { +#ifdef CONFIG_DEBUG_TIMEKEEPING struct timekeeper *tk = &tk_core.timekeeper; u64 now, last, mask, max, delta; unsigned int seq; @@ -275,16 +281,10 @@ static inline u64 timekeeping_debug_get_ns(const struct tk_read_base *tkr) /* timekeeping_cycles_to_ns() handles both under and overflow */ return timekeeping_cycles_to_ns(tkr, now); -} #else -static inline void timekeeping_check_update(struct timekeeper *tk, u64 offset) -{ -} -static inline u64 timekeeping_debug_get_ns(const struct tk_read_base *tkr) -{ - BUG(); -} + return __timekeeping_get_ns(tkr); #endif +} /** * tk_setup_internals - Set up internals to use clocksource clock. @@ -390,19 +390,6 @@ static inline u64 timekeeping_cycles_to_ns(const struct tk_read_base *tkr, u64 c return ((delta * tkr->mult) + tkr->xtime_nsec) >> tkr->shift; } -static __always_inline u64 __timekeeping_get_ns(const struct tk_read_base *tkr) -{ - return timekeeping_cycles_to_ns(tkr, tk_clock_read(tkr)); -} - -static inline u64 timekeeping_get_ns(const struct tk_read_base *tkr) -{ - if (IS_ENABLED(CONFIG_DEBUG_TIMEKEEPING)) - return timekeeping_debug_get_ns(tkr); - - return __timekeeping_get_ns(tkr); -} - /** * update_fast_timekeeper - Update the fast and NMI safe monotonic timekeeper. * @tkr: Timekeeping readout base from which we take the update
"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: >>> Le 11/04/2024 à 10:12, Christophe Leroy a écrit : >>>> >>>> Looking at the report, I think the correct fix should be to use >>>> BUILD_BUG() instead of BUG() >>> >>> I confirm the error goes away with the following change to next-20240411 >>> on powerpc tinyconfig with gcc 13.2 >>> >>> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c >>> index 4e18db1819f8..3d5ac0cdd721 100644 >>> --- a/kernel/time/timekeeping.c >>> +++ b/kernel/time/timekeeping.c >>> @@ -282,7 +282,7 @@ static inline void timekeeping_check_update(struct >>> timekeeper *tk, u64 offset) >>> } >>> static inline u64 timekeeping_debug_get_ns(const struct tk_read_base *tkr) >>> { >>> - BUG(); >>> + BUILD_BUG(); >>> } >>> #endif >>> >> >> 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. 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. cheers
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. Arnd
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
On Mon, Apr 15, 2024, at 19:07, Christophe Leroy wrote: > Le 15/04/2024 à 17:35, Arnd Bergmann a écrit : >> >> 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 ? Sure, this version looks fine to me, and passes a simple build test without CONFIG_DEBUG_TIMEKEEPING. Arnd diff --git a/include/linux/timekeeper_internal.h b/include/linux/timekeeper_internal.h index 84ff2844df2a..485677a98b0b 100644 --- a/include/linux/timekeeper_internal.h +++ b/include/linux/timekeeper_internal.h @@ -124,7 +124,6 @@ struct timekeeper { u32 ntp_err_mult; /* Flag used to avoid updating NTP twice with same second */ u32 skip_second_overflow; -#ifdef CONFIG_DEBUG_TIMEKEEPING long last_warning; /* * These simple flag variables are managed @@ -135,7 +134,6 @@ struct timekeeper { */ int underflow_seen; int overflow_seen; -#endif }; #ifdef CONFIG_GENERIC_TIME_VSYSCALL diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 4e18db1819f8..17f7aed807e1 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -195,7 +195,6 @@ static inline u64 tk_clock_read(const struct tk_read_base *tkr) return clock->read(clock); } -#ifdef CONFIG_DEBUG_TIMEKEEPING #define WARNING_FREQ (HZ*300) /* 5 minute rate-limiting */ static void timekeeping_check_update(struct timekeeper *tk, u64 offset) @@ -276,15 +275,6 @@ static inline u64 timekeeping_debug_get_ns(const struct tk_read_base *tkr) /* timekeeping_cycles_to_ns() handles both under and overflow */ return timekeeping_cycles_to_ns(tkr, now); } -#else -static inline void timekeeping_check_update(struct timekeeper *tk, u64 offset) -{ -} -static inline u64 timekeeping_debug_get_ns(const struct tk_read_base *tkr) -{ - BUG(); -} -#endif /** * tk_setup_internals - Set up internals to use clocksource clock. @@ -2173,7 +2163,8 @@ static bool timekeeping_advance(enum timekeeping_adv_mode mode) goto out; /* Do some additional sanity checking */ - timekeeping_check_update(tk, offset); + if (IS_ENABLED(CONFIG_DEBUG_TIMEKEEPING)) + timekeeping_check_update(tk, offset); /* * With NO_HZ we may have to accumulate many cycle_intervals
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 6e794420bd39..b7de3a4eade1 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -156,7 +156,10 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...); #else /* !CONFIG_BUG */ #ifndef HAVE_ARCH_BUG -#define BUG() do {} while (1) +#define BUG() do { \ + do {} while (1); \ + unreachable(); \ +} while (0) #endif #ifndef HAVE_ARCH_BUG_ON
BUG() does not return, and arch implementations of BUG() use unreachable() or other non-returning code. However with !CONFIG_BUG, the default implementation is often used instead, and that does not do that. x86 always uses its own implementation, but powerpc with !CONFIG_BUG gives a build error: kernel/time/timekeeping.c: In function ‘timekeeping_debug_get_ns’: kernel/time/timekeeping.c:286:1: error: no return statement in function returning non-void [-Werror=return-type] Add unreachable() to default !CONFIG_BUG BUG() implementation. Fixes: e8e9d21a5df6 ("timekeeping: Refactor timekeeping helpers") Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> Closes: https://lore.kernel.org/all/CA+G9fYvjdZCW=7ZGxS6A_3bysjQ56YF7S-+PNLQ_8a4DKh1Bhg@mail.gmail.com/ Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- include/asm-generic/bug.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)