| Message ID | 20230105000847.151464-1-hws@denx.de |
|---|---|
| State | Accepted |
| Commit | ea3d28ec31d64cd059683947746ac71c8a90a1f9 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Revert "time: add weak annotation to timer_read_counter declaration" | expand |
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
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!
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)
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(-)