[PATCH] Revert "time: add weak annotation to timer_read_counter declaration"

[PATCH] Revert "time: add weak annotation to timer_read_counter declaration"

[Harald Seiler]

This reverts commit 65ba7add0d609bbd035b8d42fafdaf428ac24751.

A weak extern is a nasty sight to behold: If the symbol is never
defined, on ARM, the linker will replace the function call with a NOP.
This behavior isn't well documented but there are at least some hints
to it [1].

When timer_read_counter() is not defined, this obviously does the wrong
thing here and it does so silently.  The consequence is that a board
without timer_read_counter() will sleep for random amounts and generally
have erratic get_ticks() values.

Drop the __weak annotation of the extern so a linker error is raised
when timer_read_counter() is not defined.  This is okay, the original
reason for the reverted change - breaking the sandbox build - no longer
applies.

Final sidenote:  This was the only weak extern in the entire tree at
this time as far as I can tell.  I guess we should avoid introduction of
them again as they are obviously a very big footgun.

[1]: https://stackoverflow.com/questions/31203402/gcc-behavior-for-unresolved-weak-functions

Fixes: 65ba7add0d60 ("time: add weak annotation to timer_read_counter declaration")
Reported-by: Serge Bazanski <q3k@q3k.org>
Signed-off-by: Harald Seiler <hws@denx.de>
---
 lib/time.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/time.c b/lib/time.c
index f3aaf472d1..1e24b1b03c 100644
--- a/lib/time.c
+++ b/lib/time.c
@@ -63,7 +63,7 @@ ulong timer_get_boot_us(void)
 }
 
 #else
-extern unsigned long __weak timer_read_counter(void);
+extern unsigned long timer_read_counter(void);
 #endif
 
 #if CONFIG_IS_ENABLED(TIMER)
-- 
2.39.0

Re: [PATCH] Revert "time: add weak annotation to timer_read_counter declaration"

[Rob Herring]

On Wed, Jan 4, 2023 at 6:09 PM Harald Seiler <hws@denx.de> wrote:
>
> This reverts commit 65ba7add0d609bbd035b8d42fafdaf428ac24751.
>
> A weak extern is a nasty sight to behold: If the symbol is never
> defined, on ARM, the linker will replace the function call with a NOP.
> This behavior isn't well documented but there are at least some hints
> to it [1].
>
> When timer_read_counter() is not defined, this obviously does the wrong
> thing here and it does so silently.  The consequence is that a board
> without timer_read_counter() will sleep for random amounts and generally
> have erratic get_ticks() values.
>
> Drop the __weak annotation of the extern so a linker error is raised
> when timer_read_counter() is not defined.  This is okay, the original
> reason for the reverted change - breaking the sandbox build - no longer
> applies.
>
> Final sidenote:  This was the only weak extern in the entire tree at
> this time as far as I can tell.  I guess we should avoid introduction of
> them again as they are obviously a very big footgun.
>
> [1]: https://stackoverflow.com/questions/31203402/gcc-behavior-for-unresolved-weak-functions
>
> Fixes: 65ba7add0d60 ("time: add weak annotation to timer_read_counter declaration")

I don't agree that this is a fix to the above commit. Applying it at
any point after commit 65ba7add0d60 would not work in all cases. It
may not be needed any more, but that is probably due to the addition
of the timer class 2 years later, not that the commit was wrong/broken
at the time.

Rob

Re: [PATCH] Revert "time: add weak annotation to timer_read_counter declaration"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]

On Thu, Jan 05, 2023 at 01:08:47AM +0100, Harald Seiler wrote:

> This reverts commit 65ba7add0d609bbd035b8d42fafdaf428ac24751.
> 
> A weak extern is a nasty sight to behold: If the symbol is never
> defined, on ARM, the linker will replace the function call with a NOP.
> This behavior isn't well documented but there are at least some hints
> to it [1].
> 
> When timer_read_counter() is not defined, this obviously does the wrong
> thing here and it does so silently.  The consequence is that a board
> without timer_read_counter() will sleep for random amounts and generally
> have erratic get_ticks() values.
> 
> Drop the __weak annotation of the extern so a linker error is raised
> when timer_read_counter() is not defined.  This is okay, the original
> reason for the reverted change - breaking the sandbox build - no longer
> applies.
> 
> Final sidenote:  This was the only weak extern in the entire tree at
> this time as far as I can tell.  I guess we should avoid introduction of
> them again as they are obviously a very big footgun.
> 
> [1]: https://stackoverflow.com/questions/31203402/gcc-behavior-for-unresolved-weak-functions
> 
> Fixes: 65ba7add0d60 ("time: add weak annotation to timer_read_counter declaration")
> Reported-by: Serge Bazanski <q3k@q3k.org>
> Signed-off-by: Harald Seiler <hws@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]