statx struct's stx_size pointer compatibility with uint64_t/size_t

statx struct's stx_size pointer compatibility with uint64_t/size_t

[Dominique Martinet]

I originally asked this on libc-help@, Florian redirected me to
linux-api@ and libc-alpha@; resending my first mail and quoting his
reply at the end.


A coworker ran into an incompatible-pointer-type compiler warning when
trying to pass &statxbuf.stx_size to a function expecting a size_t *,
which boils down to this:
-------------
#define _GNU_SOURCE
#include <stdint.h>
#include <sys/stat.h>

static void test(uint64_t *size) {   
    (void)size;
}

int main() {   
    struct statx statxbuf;

    test(&statxbuf.stx_size);
    return 0;
}
-------------
Giving this warning:
-------------
t.c: In function ‘main’:
t.c:12:10: warning: passing argument 1 of ‘test’ from incompatible
pointer type [-Wincompatible-pointer-types]
   12 |     test(&statxbuf.stx_size);
      |          ^~~~~~~~~~~~~~~~~~
      |          |
      |          __u64 * {aka long long unsigned int *}
t.c:5:28: note: expected ‘uint64_t *’ {aka ‘long unsigned int *’} but
argument is of type ‘__u64 *’ {aka ‘long long unsigned int *’}
    5 | static void test(uint64_t *size) {
      |                  ~~~~~~~~~~^~~~
-------------
(same happens with size_t)

The final warning is probably some standard defining long and long long
cannot be treated as compatible even on archs where it is, but it's a
bit of a shame that manipulating __u64 and uint64_t yield such errors
when passing pointers around -- the types pretty much guarantee they
have to be compatible and it is just an arbitrary choice that made one
be long and the other long long on x86_64 unless I misunderstood
something?


I'm a bit at loss of what to advise here.
We need to pass the value as a pointer because it can be updated, our
use case is that the symlink size can be wrong in /proc/x/fd/ and we
will want the correct value back in a statx struct here[1].

What would be the "recommended" way of doing this?
Any chance the field could change to be uint64_t-compatible in the
future? Not sure what that implies regarding e.g. backwards
compatibility though...


[1] https://github.com/cea-hpc/robinhood/blob/1ed74893c088d78783acd2e25e8009a483510ff7/src/backends/posix.c#L248


Florian Weimer wrote on Tue, Dec 17, 2019:
> * Dominique Martinet:
> > What would be the "recommended" way of doing this?
> > Any chance the field could change to be uint64_t-compatible in the
> > future? Not sure what that implies regarding e.g. backwards
> > compatibility though...
> 
> This is a tricky subject.  We already have a copy of the type with
> uint64_t fields in the installed glibc headers, but this is only used
> if the kernel definition is not available.
> 
> We do not want to duplicate kernel headers too much because it causes
> problems if both glibc headers and kernel headers are included in the
> same translation unit.  That in turn makes it difficult to use new
> kernel features by only updating the kernel headers.
> 
> I do not know why the kernel definition of __u64 does not follow that
> of uint64_t in <stdint.h> (or why we even have that __u64 type), and
> whether the kernel definition can be changed at this point.  We can
> fix this issue with preprocessor magic, but I am not entirely sure if
> this is a good idea.


Thanks,
-- 
Dominique

Re: statx struct's stx_size pointer compatibility with uint64_t/size_t

[Dominique Martinet]

Dominique Martinet wrote on Tue, Dec 17, 2019:
> Florian Weimer wrote on Tue, Dec 17, 2019:
> > I do not know why the kernel definition of __u64 does not follow that
> > of uint64_t in <stdint.h> (or why we even have that __u64 type), and
> > whether the kernel definition can be changed at this point.  We can
> > fix this issue with preprocessor magic, but I am not entirely sure if
> > this is a good idea.

Looking at this from a kernel's point of view, it looks like there
really was a will to simplify 64-bit ints handling over all arches and
have them all define 64-bit ints as long long a few years back.

See for example linux commit 0c79a8e29 ("asm/types.h: Remove
include/asm-generic/int-l64.h")[1] that describes the removal of '64bit
ints as long' there.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0c79a8e29b5fcbcbfd611daf9d500cfad8370fcf


This makes sense to me to avoid multiplying header files for the
different arches, so if anything I would be tempted to ask 'why is
stdint.h uint64_t defined with just long'? -- although from what I see,
musl and uClibc both also define it as just long so there must also be
some logic in using the smallest possible type that fits?

If someone happens to know why then perhaps the same reason could also
make sense with the kernel, I don't know. Tricky, as you pointed out...

(size_t is another issue and I agree it's best not to rely on it being
64 bits long anyway)



Thanks,
-- 
Dominique

Re: statx struct's stx_size pointer compatibility with uint64_t/size_t

[David Howells]

Dominique Martinet <asmadeus@codewreck.org> wrote:

> Looking at this from a kernel's point of view, it looks like there
> really was a will to simplify 64-bit ints handling over all arches and
> have them all define 64-bit ints as long long a few years back.

Think printk() too.  Do you use "%lu", "%Lu" or "%llu"?  It's a lot easier if
__u64 is consistently unsigned long long - then it's always "%llu".

The problem with defining it as "unsigned long" on some platforms and
"unsigned long long" on others is that you're guaranteed warnings on one arch
or another.

David

Re: statx struct's stx_size pointer compatibility with uint64_t/size_t

[Florian Weimer]

* Dominique Martinet:

> This makes sense to me to avoid multiplying header files for the
> different arches, so if anything I would be tempted to ask 'why is
> stdint.h uint64_t defined with just long'?

It's not a compiler-provided header.  When it was added to glibc in the
90s, I don't think long long support was universal among 64-bit
compilers, and you could not just drop the type (which might have been
acceptable on 32-bit architectures).

Anyway, looking at this, it looks like we should define struct statx
with unsigned long long int in our copy instead of uint64_t.  I filed
bug 25292 to track this.  I guess it's just another thing to keep in
mind when adding system call support to glibc headers.

Thanks,
Florian