| Message ID | 1380915680-27206-1-git-send-email-peter@korsgaard.com |
|---|---|
| State | New |
| Headers | show |
>>>>> "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" == 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" == 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?
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
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
>>>>> "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?
>>>>> "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?
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
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
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
>>>>> "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.
>>>>> "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.
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
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)
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(-)