diff mbox

fix for pipe() on sparc

Message ID 20140819191115.GA28883@waldemar-brodkorb.de
State Accepted
Headers show

Commit Message

Waldemar Brodkorb Aug. 19, 2014, 7:11 p.m. UTC
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.

Signed-off-by: Waldemar Brodkorb <wbx@openadk.org>
Signed-off-by: Thorsten Glaser <tg@mirbsd.org>
---
 libc/sysdeps/linux/sparc/pipe.S | 2 --
 1 file changed, 2 deletions(-)

Comments

Bernhard Reutner-Fischer Aug. 20, 2014, 8:10 a.m. UTC | #1
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!
Rich Felker Aug. 20, 2014, 2:23 p.m. UTC | #2
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
Waldemar Brodkorb Aug. 20, 2014, 3:50 p.m. UTC | #3
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
Rich Felker Aug. 20, 2014, 7:48 p.m. UTC | #4
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
Bernhard Reutner-Fischer Aug. 20, 2014, 8:13 p.m. UTC | #5
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 mbox

Patch

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 */