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 <[email protected]>
Closes: https://lore.kernel.org/all/CA+G9fYvjdZCW=7ZGxS6A_3bysjQ56YF7S-+PNLQ_8a4DKh1Bhg@mail.gmail.com/
Signed-off-by: Adrian Hunter <[email protected]>
---
include/asm-generic/bug.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
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
--
2.34.1
On Wed, 10 Apr 2024 at 21:02, Adrian Hunter <[email protected]> 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 <[email protected]>
> Closes: https://lore.kernel.org/all/CA+G9fYvjdZCW=7ZGxS6A_3bysjQ56YF7S-+PNLQ_8a4DKh1Bhg@mail.gmail.com/
> Signed-off-by: Adrian Hunter <[email protected]>
This patch applied on top of today's Linux next-20240410 tag and
build test pass.
Tested-by: Linux Kernel Functional Testing <[email protected]>
> ---
> include/asm-generic/bug.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
--
Linaro LKFT
https://lkft.linaro.org
The following commit has been merged into the timers/urgent branch of tip:
Commit-ID: 5284984a4fbacb0883bfebe905902cdda2891a07
Gitweb: https://git.kernel.org/tip/5284984a4fbacb0883bfebe905902cdda2891a07
Author: Adrian Hunter <[email protected]>
AuthorDate: Wed, 10 Apr 2024 18:32:12 +03:00
Committer: Thomas Gleixner <[email protected]>
CommitterDate: Wed, 10 Apr 2024 22:01:35 +02:00
bug: Fix no-return-statement warning with !CONFIG_BUG
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 <[email protected]>
Signed-off-by: Adrian Hunter <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Tested-by: Linux Kernel Functional Testing <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Closes: https://lore.kernel.org/all/CA+G9fYvjdZCW=7ZGxS6A_3bysjQ56YF7S-+PNLQ_8a4DKh1Bhg@mail.gmail.com/
---
include/asm-generic/bug.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 6e79442..b7de3a4 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
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" <[email protected]> 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" <[email protected]> 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" <[email protected]> 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