Message ID | 20140819191115.GA28883@waldemar-brodkorb.de |
---|---|
State | Accepted |
Headers | show |
On 19 August 2014 21:11, Waldemar Brodkorb <wbx@openadk.org> wrote: > When using something like this: > $ echo foo|grep foo|wc -l > with mksh shell, you get an runtime error. > > Glibc and klibc does not do these extra check. > After removing this check using double pipes work fine. > Tested with Qemu 2.1.0. Applied, thanks!
On Wed, Aug 20, 2014 at 10:10:30AM +0200, Bernhard Reutner-Fischer wrote: > On 19 August 2014 21:11, Waldemar Brodkorb <wbx@openadk.org> wrote: > > When using something like this: > > $ echo foo|grep foo|wc -l > > with mksh shell, you get an runtime error. > > > > Glibc and klibc does not do these extra check. > > After removing this check using double pipes work fine. > > Tested with Qemu 2.1.0. > > Applied, thanks! Is there any explanation for what the patch does and why it's correct? I suspect it is, but it would be nice to see (in both the patch proposal and the commit message) an indication of why the change is deemed correct rather than just "problem symptom observed before, goes away after patch". Rich
Hi Rich, Rich Felker wrote, > On Wed, Aug 20, 2014 at 10:10:30AM +0200, Bernhard Reutner-Fischer wrote: > > On 19 August 2014 21:11, Waldemar Brodkorb <wbx@openadk.org> wrote: > > > When using something like this: > > > $ echo foo|grep foo|wc -l > > > with mksh shell, you get an runtime error. > > > > > > Glibc and klibc does not do these extra check. > > > After removing this check using double pipes work fine. > > > Tested with Qemu 2.1.0. > > > > Applied, thanks! > > Is there any explanation for what the patch does and why it's correct? > I suspect it is, but it would be nice to see (in both the patch > proposal and the commit message) an indication of why the change is > deemed correct rather than just "problem symptom observed before, goes > away after patch". Sorry, I wanted to add the original author to CC, but have forgotten it. I can not add any more explanation to the problem. I have done my best to produce a simple testcase and to run the perl based mksh testsuite in Qemu and provide Thorsten a backtrace, some disassembly and a static binary, as I know he has access to some real sparc hardware. He analyzed it and suggested this change and I tested it again and was happy that my system now boots up fine without issues. At this point I just could had kept the change for myself. But I think better we have a fix for the problem. I don't know exactly why the code was added. In a perfect world the programmer had added some comments while adding assembly code, which usage might not be obvious for skilled people like you. best regards Waldemar
On Wed, Aug 20, 2014 at 05:50:08PM +0200, Waldemar Brodkorb wrote: > Hi Rich, > Rich Felker wrote, > > > On Wed, Aug 20, 2014 at 10:10:30AM +0200, Bernhard Reutner-Fischer wrote: > > > On 19 August 2014 21:11, Waldemar Brodkorb <wbx@openadk.org> wrote: > > > > When using something like this: > > > > $ echo foo|grep foo|wc -l > > > > with mksh shell, you get an runtime error. > > > > > > > > Glibc and klibc does not do these extra check. > > > > After removing this check using double pipes work fine. > > > > Tested with Qemu 2.1.0. > > > > > > Applied, thanks! > > > > Is there any explanation for what the patch does and why it's correct? > > I suspect it is, but it would be nice to see (in both the patch > > proposal and the commit message) an indication of why the change is > > deemed correct rather than just "problem symptom observed before, goes > > away after patch". > > Sorry, I wanted to add the original author to CC, but have forgotten > it. I can not add any more explanation to the problem. I have done > my best to produce a simple testcase and to run the perl based mksh > testsuite in Qemu and provide Thorsten a backtrace, some > disassembly and a static binary, as I know he has access to some > real sparc hardware. > He analyzed it and suggested this change and I tested it again and > was happy that my system now boots up fine without issues. > At this point I just could had kept the change for myself. > But I think better we have a fix for the problem. I don't know exactly why > the code was added. In a perfect world the programmer had added some > comments while adding assembly code, which usage might not be > obvious for skilled people like you. No problem. As far as I can tell, the new version is equivalent to what's in glibc now, and despite the old version supposedly being copied from glibc, I can't find it in the glibc history. I am interested in what the incorrect check was doing though, and why it was wrong. Rich
On 20 August 2014 21:48:33 CEST, Rich Felker <dalias@libc.org> wrote: >No problem. As far as I can tell, the new version is equivalent to >what's in glibc now, and despite the old version supposedly being >copied from glibc, I can't find it in the glibc history. I am Austin, where's that orcc coming from? >interested in what the incorrect check was doing though, and why it >was wrong. orcc %i1,%g0,%o1 or %i1 with 0 and store it to o1. mov %i2,%o0 Put input arg #2 into the first syscall arg. So we now effectively lose the first input arg, no? What am I missing?
diff --git a/libc/sysdeps/linux/sparc/pipe.S b/libc/sysdeps/linux/sparc/pipe.S index 09ef322..b085faf 100644 --- a/libc/sysdeps/linux/sparc/pipe.S +++ b/libc/sysdeps/linux/sparc/pipe.S @@ -35,8 +35,6 @@ pipe: /* sanity check arguments */ tst %i0 be .Lerror - orcc %i1,%g0,%o1 - be .Lerror mov %i2,%o0 /* Do the system call */