issue with preadv/pwritev and gcc on armel/armhf

[Jérémy Lal]

Hi,

as discussed in
https://github.com/libuv/libuv/issues/4678

and associated build failures
https://buildd.debian.org/status/package.php?p=libuv1&suite=experimental

It seems that gcc is doing something wrong with the offset parameter of
preadv, pwritev.
The same problem happens with optimization levels 0, 1, 2.

The call is made at
https://salsa.debian.org/debian/libuv1/-/blob/debian/sid/src/unix/fs.c?ref_type=heads#L494

To reproduce, just dpkg-buildpackage from
dget -xu
https://deb.debian.org/debian/pool/main/libu/libuv1/libuv1_1.49.2-1.dsc

I'd be surprised if it concerns only libuv !
Please have a look, because the libuv maintainers already tried their best.

Jérémy

[Arnd Bergmann]

Post by Jérémy Lal
Hi,
as discussed in
https://github.com/libuv/libuv/issues/4678
and associated build failures
https://buildd.debian.org/status/package.php?p=libuv1&suite=experimental
It seems that gcc is doing something wrong with the offset parameter of
preadv, pwritev.
The same problem happens with optimization levels 0, 1, 2.
The call is made at
https://salsa.debian.org/debian/libuv1/-/blob/debian/sid/src/unix/fs.c?ref_type=heads#L494
To reproduce, just dpkg-buildpackage from
dget -xu https://deb.debian.org/debian/pool/main/libu/libuv1/libuv1_1.49.2-1.dsc
I'd be surprised if it concerns only libuv !
Please have a look, because the libuv maintainers already tried their best.

My strong guess is that this is caused by the implied 64-bit
off_t after the time64 conversion. On i386, the file is
likely still built with a 32-bit off_t.

In the source line

p = dlsym(RTLD_DEFAULT, is_pread ? "preadv" : "pwritev");

the uv__preadv_or_pwritev() function looks up the preadv() or
pwritev() symbols in glibc, but those expect a 32-bit ("long")
off_t, unlike preadv64() and pwritev64().

It appears that https://github.com/libuv/libuv/issues/4532
attempted to address the issue, but completely misunderstood
the problem, as this was a bug in libuv, not in gcc.

Ideally the library should avoid using dlsym() here, since
that makes it unportable. If it can't be avoided, it could
try to do something like

if ((sizeof(long)< sizeof(off_t))
p = dlsym(RTLD_DEFAULT, is_pread ? "preadv64" : "pwritev64");
else
p = dlsym(RTLD_DEFAULT, is_pread ? "preadv" : "pwritev");

This in turn makes it less portable to non-glibc
implementations like musl.

Arnd

[Jérémy Lal]

Post by Jérémy Lal

Post by Jérémy Lal
Hi,
as discussed in
https://github.com/libuv/libuv/issues/4678
and associated build failures
https://buildd.debian.org/status/package.php?p=libuv1&suite=experimental
It seems that gcc is doing something wrong with the offset parameter of
preadv, pwritev.
The same problem happens with optimization levels 0, 1, 2.
The call is made at

https://salsa.debian.org/debian/libuv1/-/blob/debian/sid/src/unix/fs.c?ref_type=heads#L494

Post by Jérémy Lal
To reproduce, just dpkg-buildpackage from
dget -xu

https://deb.debian.org/debian/pool/main/libu/libuv1/libuv1_1.49.2-1.dsc

Post by Jérémy Lal
I'd be surprised if it concerns only libuv !
Please have a look, because the libuv maintainers already tried their

best.
My strong guess is that this is caused by the implied 64-bit
off_t after the time64 conversion. On i386, the file is
likely still built with a 32-bit off_t.
In the source line
p = dlsym(RTLD_DEFAULT, is_pread ? "preadv" : "pwritev");
the uv__preadv_or_pwritev() function looks up the preadv() or
pwritev() symbols in glibc, but those expect a 32-bit ("long")
off_t, unlike preadv64() and pwritev64().
It appears that https://github.com/libuv/libuv/issues/4532
attempted to address the issue, but completely misunderstood
the problem, as this was a bug in libuv, not in gcc.
Ideally the library should avoid using dlsym() here, since
that makes it unportable. If it can't be avoided, it could
try to do something like
if ((sizeof(long)< sizeof(off_t))
p = dlsym(RTLD_DEFAULT, is_pread ? "preadv64" : "pwritev64");
else
p = dlsym(RTLD_DEFAULT, is_pread ? "preadv" : "pwritev");

Thank you, it works with that.
I linked your reply in the upstream issue.

This in turn makes it less portable to non-glibc

Post by Jérémy Lal
implementations like musl.

I suppose that unless upstream rewrites some parts, we will just add a patch
in the Debian package for now.

[Arnd Bergmann]

Post by Jérémy Lal

Post by Arnd Bergmann
if ((sizeof(long)< sizeof(off_t))
p = dlsym(RTLD_DEFAULT, is_pread ? "preadv64" : "pwritev64");
else
p = dlsym(RTLD_DEFAULT, is_pread ? "preadv" : "pwritev");

Thank you, it works with that.
I linked your reply in the upstream issue.

It appears that the proposed workaround at

https://github.com/libuv/libuv/pull/4683/commits/45bf09e2567b6480962d932946608454fab31b87

now just always tries to find the preadv64()/pwritev64() symbols,
which is still nonportable because this doesn't work on targets
that use a 32-bit off_t.

If i386-debian was working before this change, it is now broken.

The bit I don't understand is why libuv was ever getting built
without largefile support. It probably makes sense to change that
for all architectures regardless of time64 support, but this
is likely an ABI break and requires rebuilding all libuv users
in turn.

Arnd

[Jérémy Lal]

Post by Arnd Bergmann

Post by Jérémy Lal

Post by Arnd Bergmann
if ((sizeof(long)< sizeof(off_t))

"pwritev64");

Post by Jérémy Lal

Post by Arnd Bergmann
else
p = dlsym(RTLD_DEFAULT, is_pread ? "preadv" : "pwritev");

Thank you, it works with that.
I linked your reply in the upstream issue.

It appears that the proposed workaround at
https://github.com/libuv/libuv/pull/4683/commits/45bf09e2567b6480962d932946608454fab31b87
now just always tries to find the preadv64()/pwritev64() symbols,
which is still nonportable because this doesn't work on targets
that use a 32-bit off_t.
If i386-debian was working before this change, it is now broken.
The bit I don't understand is why libuv was ever getting built
without largefile support. It probably makes sense to change that
for all architectures regardless of time64 support, but this
is likely an ABI break and requires rebuilding all libuv users
in turn.

It builds fine in a i386 chroot on barriere.d.o with
https://github.com/libuv/libuv/pull/4683.patch

[Arnd Bergmann]

Post by Jérémy Lal

Post by Arnd Bergmann
The bit I don't understand is why libuv was ever getting built
without largefile support. It probably makes sense to change that
for all architectures regardless of time64 support, but this
is likely an ABI break and requires rebuilding all libuv users
in turn.

It builds fine in a i386 chroot on barriere.d.o with
https://github.com/libuv/libuv/pull/4683.patch

But does it successfully run the test case? I can't
see how the behavior on i386 is correct both before
and after the patch: either the bug existed on all
32-bit architectures because they enabled largefile
support, or changing i386 over to use the largefile
version of preadv/writev is wrong.

Can you debug into this file on i386 to see which symbol
it picks up in the end, and whether off_t is defined
as a 32-bit or 64-bit type?

It's possible that for some reason the testcase fails
to report a bug on i386 despite the code being wrong
either before or after your patch.

Arnd

[Jérémy Lal]

Post by Arnd Bergmann

Post by Jérémy Lal

Post by Arnd Bergmann
The bit I don't understand is why libuv was ever getting built
without largefile support. It probably makes sense to change that
for all architectures regardless of time64 support, but this
is likely an ABI break and requires rebuilding all libuv users
in turn.

It builds fine in a i386 chroot on barriere.d.o with
https://github.com/libuv/libuv/pull/4683.patch

But does it successfully run the test case?

Yes

I can't

Post by Arnd Bergmann
see how the behavior on i386 is correct both before
and after the patch: either the bug existed on all
32-bit architectures because they enabled largefile
support, or changing i386 over to use the largefile
version of preadv/writev is wrong.
Can you debug into this file on i386 to see which symbol
it picks up in the end, and whether off_t is defined
as a 32-bit or 64-bit type?

After patch, preadv64.
In both cases,
(gdb) whatis off
type = off_t
(gdb) whatis off_t
type = __off64_t

Post by Arnd Bergmann
It's possible that for some reason the testcase fails
to report a bug on i386 despite the code being wrong
either before or after your patch.

[Arnd Bergmann]

Post by Jérémy Lal

Post by Arnd Bergmann
Can you debug into this file on i386 to see which symbol
it picks up in the end, and whether off_t is defined
as a 32-bit or 64-bit type?

After patch, preadv64.
In both cases,
(gdb) whatis off
type = off_t
(gdb) whatis off_t
type = __off64_t

Post by Arnd Bergmann
It's possible that for some reason the testcase fails
to report a bug on i386 despite the code being wrong
either before or after your patch.

Ok, good, so most likely your new version just works and
it was broken on all 32-bit targets including i386 before
your patch. That leaves the question why the testcase
didn't catch that bug on i386.

What I think is going on here is that i386 calling
conventions for 64-bit function arguments happen to
make it work correctly, as they are passed on the stack
as five 32-bit words in preadv64()

<fd> <iov> <iovcnt> <off_low> <off_high>

and preadv() takes exactly the same four arguments
but ignores the last one.

On ARM EABI, the arguments are passed in six registers
instead, with the 64-bit argument in an aligned pair
of registers

<fd> <iov> <iovcnt> <_PAD_> <off_low> <off_high>

which in turn means that calling preadv() by accident
with the preadv64 arguments puts uninitialized stack
data into the 32-bit offset argument slow. The same thing
happens on mips, powerpc, xtensa, arc, nios2, openrisc
sh and riscv. On top of this, big-endian architectures
also typically flip the low and high word for the
64-bit argument, so you pass the wrong word on m68k,
parisc, s390, sparc and openrisc despite those not
inserting the padding argument.

To catch these bugs better, I think a testcase could
be added that passes and argument that has valid
32-bit offset, but garbage in the high bits, e.g.
0xffffffff00000008. On correctly working targets this
should return errno=ENXIO, while on broken i386
systems it would succeed reading from file offset 8.

Arnd