[fbtest] Initialize ticks in benchmark()

Commit Message

Philipp Zabel June 29, 2023, 8:28 a.m. UTC

Fixes a build error with GCC 13:

  util.c: In function 'benchmark':
  util.c:177:17: error: 'ticks' may be used uninitialized [-Werror=maybe-uninitialized]
  util.c:161:14: note: 'ticks' was declared here

Signed-off-by: Philipp Zabel <[e-mail]>
---
 util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Geert Uytterhoeven July 5, 2023, 9:23 a.m. UTC | #1

Hi Philipp,

Thanks for your patch!

On Thu, Jun 29, 2023 at 10:28 AM Philipp Zabel <[e-mail]> wrote:
> Fixes a build error with GCC 13:
>
>   util.c: In function 'benchmark':
>   util.c:177:17: error: 'ticks' may be used uninitialized [-Werror=maybe-uninitialized]
>   util.c:161:14: note: 'ticks' was declared here

I believe this is a false positive?
(unless your "long" type has only a single bit ;-)

But as this is built with -Werror, I agree it is better to play it safe,
and fix this.

>
> Signed-off-by: Philipp Zabel <[e-mail]>

> --- a/util.c
> +++ b/util.c
> @@ -158,7 +158,7 @@ static uint64_t get_ticks(void)
>
>  double benchmark(void (*func)(unsigned long n, void *data), void *data)
>  {
> -    uint64_t ticks;
> +    uint64_t ticks = 0;

Wouldn't it be better to preinitialize this to 1 instead?
Else an overzealous compiler might detect possible UB in the division
below, and more hell might break loose...

    return 1e6*n/ticks;

>      unsigned long n = 1;
>
>      printf("Benchmarking... ");

Gr{oetje,eeting}s,

                        Geert

Philipp Zabel July 5, 2023, 9:32 a.m. UTC | #2

Hi Geert,

On Mi, 2023-07-05 at 11:23 +0200, Geert Uytterhoeven wrote:
> Hi Philipp,
> 
> Thanks for your patch!
> 
> On Thu, Jun 29, 2023 at 10:28 AM Philipp Zabel <[e-mail]> wrote:
> > Fixes a build error with GCC 13:
> > 
> >   util.c: In function 'benchmark':
> >   util.c:177:17: error: 'ticks' may be used uninitialized [-Werror=maybe-uninitialized]
> >   util.c:161:14: note: 'ticks' was declared here
> 
> I believe this is a false positive?
> (unless your "long" type has only a single bit ;-)

Yes. I've clarified the commit message in v2.

> But as this is built with -Werror, I agree it is better to play it safe,
> and fix this.
> 
> > 
> > Signed-off-by: Philipp Zabel <[e-mail]>
> 
> > --- a/util.c
> > +++ b/util.c
> > @@ -158,7 +158,7 @@ static uint64_t get_ticks(void)
> > 
> >  double benchmark(void (*func)(unsigned long n, void *data), void *data)
> >  {
> > -    uint64_t ticks;
> > +    uint64_t ticks = 0;
> 
> Wouldn't it be better to preinitialize this to 1 instead?
> Else an overzealous compiler might detect possible UB in the division
> below, and more hell might break loose...

Thanks, since we already are working around a confused compiler, this
seems like a good idea. Fixed in v2.

regards
Philipp

Patch

diff --git a/util.c b/util.c
index cdf89b38618a..1193523990e7 100644
--- a/util.c
+++ b/util.c
@@ -158,7 +158,7 @@  static uint64_t get_ticks(void)
 
 double benchmark(void (*func)(unsigned long n, void *data), void *data)
 {
-    uint64_t ticks;
+    uint64_t ticks = 0;
     unsigned long n = 1;
 
     printf("Benchmarking... ");