debuginfod/debuginfod-client.c: correct string format on 32bit arches with 64bit time_t

  From: Alexander Kanavin <alex.kanavin@gmail.com>

Use intmax_t to print time_t

time_t is platform dependent and some of architectures e.g.
x32, riscv32, arc use 64bit time_t even while they are 32bit
architectures, therefore directly using integer printf formats will not
work portably, use intmax_t to typecast time_t into printf family of
functions.

musl 1.2.0 and above uses time64 on all platforms; glibc 2.34 has added support for
time64 (which might be enabled by distro-wide CFLAGS), with possibly
2.35 or 2.36 making it enabled by default.

Use a plain int for scanning cache_config, as that's what the function
returns.

Signed-off-by: Alexander Kanavin <alex.kanavin@gmail.com>
Signed-off-by: Khem Raj <raj.khem@gmail.com>
---
 debuginfod/debuginfod-client.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
  

Hi -

> Use intmax_t to print time_t
> [...]
> -      if (dprintf(fd, "%ld", cache_config_default_s) < 0)
> +      if (dprintf(fd, "%jd", (intmax_t)cache_config_default_s) < 0)
> [...]

I'm not a compatibility specialist, but note that elfutils is sometimes
built on decade-old toolchains, where use of %lld and (long long) would
certainly work.  We have such in the code base already, whereas this would
be the first %j.

- FChE
  

According to 'man 3 printf' %j appeared in glibc 2.1 and seems to be C99
feature. That's decidedly ancient.

And may I be allowed to suggest, unless elfutils is tested in upstream CI
with decade-old toolchains, you should not be considering its compatibility
with them.

Alex

On Thu, 25 Nov 2021 at 18:51, Frank Ch. Eigler <fche@redhat.com> wrote:

> Hi -
>
> > Use intmax_t to print time_t
> > [...]
> > -      if (dprintf(fd, "%ld", cache_config_default_s) < 0)
> > +      if (dprintf(fd, "%jd", (intmax_t)cache_config_default_s) < 0)
> > [...]
>
> I'm not a compatibility specialist, but note that elfutils is sometimes
> built on decade-old toolchains, where use of %lld and (long long) would
> certainly work.  We have such in the code base already, whereas this would
> be the first %j.
>
> - FChE
>
>
  

Hi -

> According to 'man 3 printf' %j appeared in glibc 2.1 and seems to be C99
> feature. That's decidedly ancient.

OK. 

> And may I be allowed to suggest, unless elfutils is tested in upstream CI
> with decade-old toolchains, you should not be considering its compatibility
> with them.

Deployment of elfutils is not limited to platforms that happen to be
included in upstream CI that you see.  RHEL7 uses glibc 2.17 from
2012; RHEL6 years before that.  We do not wish to unnecessarily break
them.

- FChE
  

Hi,

On Sun, Nov 21, 2021 at 11:14:11PM +0100, Alexander Kanavin via Elfutils-devel wrote:
> From: Alexander Kanavin <alex.kanavin@gmail.com>
> 
> Use intmax_t to print time_t
> 
> time_t is platform dependent and some of architectures e.g.
> x32, riscv32, arc use 64bit time_t even while they are 32bit
> architectures, therefore directly using integer printf formats will not
> work portably, use intmax_t to typecast time_t into printf family of
> functions.

OK, so there were some questions about whether using intmax_t and %jd
are portable enough, but I think it is clear that everything these
days support that. So that is good.

> musl 1.2.0 and above uses time64 on all platforms; glibc 2.34 has added support for
> time64 (which might be enabled by distro-wide CFLAGS), with possibly
> 2.35 or 2.36 making it enabled by default.
> 
> Use a plain int for scanning cache_config, as that's what the function
> returns.

So I think you are correct that printing/scanning/casting the time_t
around is somewhat unfortunate because the size of time_t is not
stable across architectures. Thanks for looking into this. I had no
idea time_t was such a problematic data type.

What makes it even more curious is that debuginfod_config_cache takes
a long, not a time_t, while it is always called with a time_t
argument. And then it returns an int... The result is interpreted as a
negative errno number or is cast back to a time_t if it is positive.

With this patch we write out the interval values "as big as possible"
(intmax_t), but read it back in "as small as possible" (int). Would it
make sense to also try to read it back in as intmax_t and then cast
down to int? Or simply always cast down to int before writing it out,
so reading and writing use the same data type?

We seem to not expect these intervals to be much bigger than a week,
so an int should always be big enough (even when stretched up to a
whole year).

Thanks,

Mark
  

I'm not sure; the point of this patch is simply to ensure debuginfod builds
everywhere without errors. Making the types consistent is perhaps better
done as a followup?

Alex

On Sun, 5 Dec 2021 at 21:31, Mark Wielaard <mark@klomp.org> wrote:

> Hi,
>
> On Sun, Nov 21, 2021 at 11:14:11PM +0100, Alexander Kanavin via
> Elfutils-devel wrote:
> > From: Alexander Kanavin <alex.kanavin@gmail.com>
> >
> > Use intmax_t to print time_t
> >
> > time_t is platform dependent and some of architectures e.g.
> > x32, riscv32, arc use 64bit time_t even while they are 32bit
> > architectures, therefore directly using integer printf formats will not
> > work portably, use intmax_t to typecast time_t into printf family of
> > functions.
>
> OK, so there were some questions about whether using intmax_t and %jd
> are portable enough, but I think it is clear that everything these
> days support that. So that is good.
>
> > musl 1.2.0 and above uses time64 on all platforms; glibc 2.34 has added
> support for
> > time64 (which might be enabled by distro-wide CFLAGS), with possibly
> > 2.35 or 2.36 making it enabled by default.
> >
> > Use a plain int for scanning cache_config, as that's what the function
> > returns.
>
> So I think you are correct that printing/scanning/casting the time_t
> around is somewhat unfortunate because the size of time_t is not
> stable across architectures. Thanks for looking into this. I had no
> idea time_t was such a problematic data type.
>
> What makes it even more curious is that debuginfod_config_cache takes
> a long, not a time_t, while it is always called with a time_t
> argument. And then it returns an int... The result is interpreted as a
> negative errno number or is cast back to a time_t if it is positive.
>
> With this patch we write out the interval values "as big as possible"
> (intmax_t), but read it back in "as small as possible" (int). Would it
> make sense to also try to read it back in as intmax_t and then cast
> down to int? Or simply always cast down to int before writing it out,
> so reading and writing use the same data type?
>
> We seem to not expect these intervals to be much bigger than a week,
> so an int should always be big enough (even when stretched up to a
> whole year).
>
> Thanks,
>
> Mark
>
  

Hi Alexander,

On Sun, 2021-12-05 at 21:45 +0100, Alexander Kanavin wrote:
> I'm not sure; the point of this patch is simply to ensure debuginfod builds
> everywhere without errors. Making the types consistent is perhaps better
> done as a followup?

I think the issue of the code not compiling in some environments is
because of the inconsistent use of types. So I rather fix the build
errors by fixing the used types than to simply cast the errors away.

Cheers,

Mark
  

Hi -

> [...]
> We seem to not expect these intervals to be much bigger than a week,
> so an int should always be big enough (even when stretched up to a
> whole year).

Yes, ints are fine for these humane-number-of-seconds kinds of values
in the cache configuration.  There's no need for maximum length
integers or even longs for purposes of storage/parse.  In the later
interval arithmetic related to time_t values, we can widen them to
(time_t) then and there.

- FChE
  

Alright, I just sent a patch that replaces time_t with long instead. I
tested it on x86, x86-x32, x86-64.

Alex

On Wed, 8 Dec 2021 at 16:31, Mark Wielaard <mark@klomp.org> wrote:

> Hi Alexander,
>
> On Sun, 2021-12-05 at 21:45 +0100, Alexander Kanavin wrote:
> > I'm not sure; the point of this patch is simply to ensure debuginfod
> builds
> > everywhere without errors. Making the types consistent is perhaps better
> > done as a followup?
>
> I think the issue of the code not compiling in some environments is
> because of the inconsistent use of types. So I rather fix the build
> errors by fixing the used types than to simply cast the errors away.
>
> Cheers,
>
> Mark
>