[PATCH i-g-t] lib/igt_core: Use suffix for casting to long to avoid overflow

[Kamil Konieczny]

Hi Lucas,
On 2024-12-17 at 12:03:49 -0600, Lucas De Marchi wrote:
> On Tue, Dec 17, 2024 at 05:47:44PM +0100, Kamil Konieczny wrote:
> > Hi Zbigniew,
> > On 2024-12-17 at 11:58:07 +0100, Zbigniew Kempczyński wrote:
> > 
> > please improve subject, imho better something like:
> > 
> > Avoid overflow in micro/nano-seconds calculations
> > or
> > Fix overflow in micro/nano-seconds calculations
> > 
> > or propose yourself one line description.
> > 
> > > Multiplication without using casting to long (long) for at least
> > > one argument in expression may cause overflow especially when we're
> > > using this for calculating timeouts in nanoseconds precision.
> > > 
> > > Ensure we're picking valid ul/ull suffix for base definitions for
> > > all dependents.
> > > 
> > > Reported-by: Ilia Levi <ilia.levi at intel.com>
> > > Closes: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/163
> > > 
> > 
> > I agree here with Lucas, remove this empty line.
> > 
> > > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > Cc: Ilia Levi <ilia.levi at intel.com>
> > > ---
> > >  lib/igt_core.h | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/igt_core.h b/lib/igt_core.h
> > > index 90f57402f0..1b6e70182b 100644
> > > --- a/lib/igt_core.h
> > > +++ b/lib/igt_core.h
> > > @@ -1517,11 +1517,17 @@ void igt_kmsg(const char *format, ...);
> > >  #define le32_to_cpu(x)  (x)
> > >  #endif
> > > 
> > > -#define MSEC_PER_SEC (1000u)
> > > +#if __WORDSIZE == 64
> > 
> > I am not sure if this is defined on all platforms where igt
> > is compiled, on musl and on non-Linux like FreeBSD,
> > why not make #ifndef/#define/#endif for __WORDSIZE ?
> 
> because if it's not defined, how are you going to decide which side of
> them to use?
> 
> from the tools/  include in kernel, one could do
> 
> #ifndef __WORDSIZE
> #define __WORDSIZE (__SIZEOF_LONG__ * 8)
> #endif
> 
> another possible solution is checking ULONG_MAX, like in dnf:
> 
> https://github.com/rpm-software-management/libdnf/pull/1159/files
> 
> but I think the kernel one is cleaner.
> 
> Lucas De Marchi
> 

Why not just 'sizeof(1ul) == 8' ?
Btw we still have problem here when sizeof(1ull) == 4
(I hope such architectures are out of our interest though)

Regards,
Kamil

> > 
> > BTW is it just sizeof(1ul) == 8 ?
> > 
> > With this fixed and subject improved
> > Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > 
> > Regards,
> > Kamil
> > 
> > > +#define MSEC_PER_SEC (1000ul)
> > > +#define USEC_PER_MSEC (1000ul)
> > > +#else
> > > +#define MSEC_PER_SEC (1000ull)
> > > +#define USEC_PER_MSEC (1000ull)
> > > +#endif
> > > +
> > >  #define USEC_PER_SEC (1000u * MSEC_PER_SEC)
> > >  #define USEC_PER_DECISEC (100u * MSEC_PER_SEC)
> > >  #define NSEC_PER_SEC (1000u * USEC_PER_SEC)
> > > -#define USEC_PER_MSEC (1000u)
> > >  #define NSEC_PER_MSEC (1000u * USEC_PER_MSEC)
> > > 
> > >  #define for_if(expr__) if (!(expr__)) {} else
> > > --
> > > 2.34.1
> > >