Patchwork [PATCH-0.9.33] common/pread_write.c: unbreak on archs without __NR_pread64

login
register
mail settings
Submitter Peter Korsgaard
Date Oct. 4, 2013, 7:41 p.m.
Message ID <1380915680-27206-1-git-send-email-peter@korsgaard.com>
Download mbox | patch
Permalink /patch/280721/
State New
Headers show

Comments

Peter Korsgaard - Oct. 4, 2013, 7:41 p.m.
Some archs (avr32 in particular) still doesn't define __NR_pread64, so
we should fall back to __NR_pread if it isn't available.

The code nicely checks for it, but then ends up hard coding the syscall
to use __NR_pread64 afterwards, rendering the check useless. Fix it by
using the result of the test instead.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
Noticed when adding the pending patches for 0.9.33.3 to Buildroot:
http://jenkins.free-electrons.com/job/buildroot/config=atstk100x_defconfig/116/console

 libc/sysdeps/linux/common/pread_write.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Peter Korsgaard - Oct. 4, 2013, 9:45 p.m.
>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

Hi,

 Peter> Some archs (avr32 in particular) still doesn't define __NR_pread64, so
 Peter> we should fall back to __NR_pread if it isn't available.

 Peter> The code nicely checks for it, but then ends up hard coding the syscall
 Peter> to use __NR_pread64 afterwards, rendering the check useless. Fix it by
 Peter> using the result of the test instead.

I noticed another critical issue on ARM EABI. The use of
__LONG_LONG_PAIR for the offset doesn't take alignment requirement of
64bit parameters on EABI into consideration, so the offset is off by one
register :/

https://lkml.org/lkml/2006/1/12/175

How should that be handled?
Peter Korsgaard - Oct. 15, 2013, 9:52 a.m.
>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 Peter> Some archs (avr32 in particular) still doesn't define __NR_pread64, so
 Peter> we should fall back to __NR_pread if it isn't available.

 Peter> The code nicely checks for it, but then ends up hard coding the syscall
 Peter> to use __NR_pread64 afterwards, rendering the check useless. Fix it by
 Peter> using the result of the test instead.

 Peter> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
 Peter> ---
 Peter> Noticed when adding the pending patches for 0.9.33.3 to Buildroot:
 Peter> http://jenkins.free-electrons.com/job/buildroot/config=atstk100x_defconfig/116/console

Ping?

 Peter>  libc/sysdeps/linux/common/pread_write.c |    2 +-
 Peter>  1 file changed, 1 insertion(+), 1 deletion(-)

 Peter> diff --git a/libc/sysdeps/linux/common/pread_write.c b/libc/sysdeps/linux/common/pread_write.c
 Peter> index b13de66..8562ab4 100644
 Peter> --- a/libc/sysdeps/linux/common/pread_write.c
 Peter> +++ b/libc/sysdeps/linux/common/pread_write.c
 Peter> @@ -42,7 +42,7 @@ extern __typeof(pwrite64) __libc_pwrite64;
 
 Peter>  #include <bits/kernel_types.h>
 
 Peter> -# define __NR___syscall_pread __NR_pread64
 Peter> +# define __NR___syscall_pread __NR_pread
 Peter>  static __inline__ _syscall5(ssize_t, __syscall_pread, int, fd, void *, buf,
 Peter>  		size_t, count, off_t, offset_hi, off_t, offset_lo)
 
 Peter> -- 
 Peter> 1.7.10.4
Peter Korsgaard - Oct. 15, 2013, 9:53 a.m.
>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

Hi,

 Peter> Some archs (avr32 in particular) still doesn't define __NR_pread64, so
 Peter> we should fall back to __NR_pread if it isn't available.

 Peter> The code nicely checks for it, but then ends up hard coding the syscall
 Peter> to use __NR_pread64 afterwards, rendering the check useless. Fix it by
 Peter> using the result of the test instead.

 Peter> I noticed another critical issue on ARM EABI. The use of
 Peter> __LONG_LONG_PAIR for the offset doesn't take alignment requirement of
 Peter> 64bit parameters on EABI into consideration, so the offset is off by one
 Peter> register :/

 Peter> https://lkml.org/lkml/2006/1/12/175

 Peter> How should that be handled?

Anybody?
Mike Frysinger - Oct. 15, 2013, 8:10 p.m.
On Friday 04 October 2013 17:45:20 Peter Korsgaard wrote:
> >>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:
>  Peter> Some archs (avr32 in particular) still doesn't define __NR_pread64,
> so Peter> we should fall back to __NR_pread if it isn't available.
> 
>  Peter> The code nicely checks for it, but then ends up hard coding the
> syscall Peter> to use __NR_pread64 afterwards, rendering the check
> useless. Fix it by Peter> using the result of the test instead.
> 
> I noticed another critical issue on ARM EABI. The use of
> __LONG_LONG_PAIR for the offset doesn't take alignment requirement of
> 64bit parameters on EABI into consideration, so the offset is off by one
> register :/
> 
> https://lkml.org/lkml/2006/1/12/175
> 
> How should that be handled?

i introduced __UCLIBC_SYSCALL_ALIGN_64BIT__ to handle this case.  and the 
pread/pwrite logic takes that into account.  do you have information to 
indicate it isn't working ?

your e-mail client still sucks btw
-mike
Mike Frysinger - Oct. 15, 2013, 8:11 p.m.
On Friday 04 October 2013 15:41:20 Peter Korsgaard wrote:
> Some archs (avr32 in particular) still doesn't define __NR_pread64, so
> we should fall back to __NR_pread if it isn't available.
> 
> The code nicely checks for it, but then ends up hard coding the syscall
> to use __NR_pread64 afterwards, rendering the check useless. Fix it by
> using the result of the test instead.

i think you should look at all the pread/pwrite changes in master.  afaik, all 
issues are addressed there.
-mike
Peter Korsgaard - Oct. 15, 2013, 8:37 p.m.
>>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:

Hi,

 >> I noticed another critical issue on ARM EABI. The use of
 >> __LONG_LONG_PAIR for the offset doesn't take alignment requirement of
 >> 64bit parameters on EABI into consideration, so the offset is off by one
 >> register :/
 >> 
 >> https://lkml.org/lkml/2006/1/12/175
 >> 
 >> How should that be handled?

 Mike> i introduced __UCLIBC_SYSCALL_ALIGN_64BIT__ to handle this case.
 Mike> and the pread/pwrite logic takes that into account.  do you have
 Mike> information to indicate it isn't working ?

Well, as the subject indicates this is for the 0.9.33 branch, where
there isn't any ALIGN_64BIT. I haven't tried master yet (will do now),
but it looks like this should get backported before 0.9.33.3.

 Mike> your e-mail client still sucks btw

Sorry, in what way?
Peter Korsgaard - Oct. 15, 2013, 8:41 p.m.
>>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:

Hi,

 >> The code nicely checks for it, but then ends up hard coding the syscall
 >> to use __NR_pread64 afterwards, rendering the check useless. Fix it by
 >> using the result of the test instead.

 Mike> i think you should look at all the pread/pwrite changes in
 Mike> master.  afaik, all issues are addressed there.

Yes, possible. I'm trying to test the 0.9.33 branch to hopefully speed
up the 0.9.33.3 release as there's quite some fixes pending, but it
looks like some more stuff should get backported.

Anybody else testing the branch?
Khem Raj - Oct. 15, 2013, 11:04 p.m.
On Oct 15, 2013, at 1:41 PM, Peter Korsgaard <peter@korsgaard.com> wrote:

>>>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:
> 
> Hi,
> 
>>> The code nicely checks for it, but then ends up hard coding the syscall
>>> to use __NR_pread64 afterwards, rendering the check useless. Fix it by
>>> using the result of the test instead.
> 
> Mike> i think you should look at all the pread/pwrite changes in
> Mike> master.  afaik, all issues are addressed there.
> 
> Yes, possible. I'm trying to test the 0.9.33 branch to hopefully speed
> up the 0.9.33.3 release as there's quite some fixes pending, but it
> looks like some more stuff should get backported.
> 
> Anybody else testing the branch?

I would be interested if you try out latest master.


> 
> -- 
> Bye, Peter Korsgaard
> _______________________________________________
> uClibc mailing list
> uClibc@uclibc.org
> http://lists.busybox.net/mailman/listinfo/uclibc
Mike Frysinger - Oct. 15, 2013, 11:44 p.m.
On Tuesday 15 October 2013 19:04:12 Khem Raj wrote:
> On Oct 15, 2013, at 1:41 PM, Peter Korsgaard <peter@korsgaard.com> wrote:
> >>>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:
> > Hi,
> > 
> >>> The code nicely checks for it, but then ends up hard coding the syscall
> >>> to use __NR_pread64 afterwards, rendering the check useless. Fix it by
> >>> using the result of the test instead.
> > 
> > Mike> i think you should look at all the pread/pwrite changes in
> > Mike> master.  afaik, all issues are addressed there.
> > 
> > Yes, possible. I'm trying to test the 0.9.33 branch to hopefully speed
> > up the 0.9.33.3 release as there's quite some fixes pending, but it
> > looks like some more stuff should get backported.
> > 
> > Anybody else testing the branch?
> 
> I would be interested if you try out latest master.

if master works, then we can cherry pick back the patches.  but i'd like to 
know before we start that work :).
-mike
Mike Frysinger - Oct. 16, 2013, 6:58 a.m.
On Tuesday 15 October 2013 16:37:32 Peter Korsgaard wrote:
>  Mike> your e-mail client still sucks btw
> 
> Sorry, in what way?

the quoting style unreasonably mangles things
-mike
Peter Korsgaard - Oct. 16, 2013, 9:06 a.m.
>>>>> "Mike" == Mike Frysinger <vapier@gentoo.org> writes:

Hi,

 > On Tuesday 15 October 2013 16:37:32 Peter Korsgaard wrote:
 Mike> your e-mail client still sucks btw
 >> 
 >> Sorry, in what way?

 > the quoting style unreasonably mangles things

The supercite 'name>' thing? I kind of like it, but I've turned it off
for @uclibc.org.
Peter Korsgaard - Nov. 21, 2013, 11:41 p.m.
>>>>> "Khem" == Khem Raj <raj.khem@gmail.com> writes:

Hi,

 >>>> The code nicely checks for it, but then ends up hard coding the
 >>>> syscall to use __NR_pread64 afterwards, rendering the check
 >>>> useless. Fix it by using the result of the test instead.
 >> 
 Mike> i think you should look at all the pread/pwrite changes in
 Mike> master.  afaik, all issues are addressed there.
 >> 
 >> Yes, possible. I'm trying to test the 0.9.33 branch to hopefully speed
 >> up the 0.9.33.3 release as there's quite some fixes pending, but it
 >> looks like some more stuff should get backported.
 >> 
 >> Anybody else testing the branch?

 > I would be interested if you try out latest master.

Sorry for the slow response - I only now found time to do so. I'm happy
to say that the pread issue ISN'T present on todays snapshot of master.

So these pread/pwrite changes imho should get backported to to the
0.9.33 branch if we ever plan on doing a bugfix release from it.
Anthony Basile - Nov. 22, 2013, 9:23 p.m.
On 11/21/2013 06:41 PM, Peter Korsgaard wrote:
>>>>>> "Khem" == Khem Raj <raj.khem@gmail.com> writes:
>
> Hi,
>
>   >>>> The code nicely checks for it, but then ends up hard coding the
>   >>>> syscall to use __NR_pread64 afterwards, rendering the check
>   >>>> useless. Fix it by using the result of the test instead.
>   >>
>   Mike> i think you should look at all the pread/pwrite changes in
>   Mike> master.  afaik, all issues are addressed there.
>   >>
>   >> Yes, possible. I'm trying to test the 0.9.33 branch to hopefully speed
>   >> up the 0.9.33.3 release as there's quite some fixes pending, but it
>   >> looks like some more stuff should get backported.
>   >>
>   >> Anybody else testing the branch?
>
>   > I would be interested if you try out latest master.
>
> Sorry for the slow response - I only now found time to do so. I'm happy
> to say that the pread issue ISN'T present on todays snapshot of master.
>
> So these pread/pwrite changes imho should get backported to to the
> 0.9.33 branch if we ever plan on doing a bugfix release from it.
>

I hit a nasty race condition in git 1.8 because pread/pwrite are not 
atomic in 0.9.33.2 [1].  I've been meaning to backport Mike's fixes for 
Gentoo, but really we should push out another release.  The linux kernel 
has provided pread/pwrite for a long time now, no need to simulate them 
with lseek().

Ref
[1] https://bugs.gentoo.org/show_bug.cgi?id=475920

Patch

diff --git a/libc/sysdeps/linux/common/pread_write.c b/libc/sysdeps/linux/common/pread_write.c
index b13de66..8562ab4 100644
--- a/libc/sysdeps/linux/common/pread_write.c
+++ b/libc/sysdeps/linux/common/pread_write.c
@@ -42,7 +42,7 @@  extern __typeof(pwrite64) __libc_pwrite64;
 
 #include <bits/kernel_types.h>
 
-# define __NR___syscall_pread __NR_pread64
+# define __NR___syscall_pread __NR_pread
 static __inline__ _syscall5(ssize_t, __syscall_pread, int, fd, void *, buf,
 		size_t, count, off_t, offset_hi, off_t, offset_lo)