Re: ELKS memcpy_fromfs() failing. Wrong variable offsets

Re: ELKS memcpy_fromfs() failing. Wrong variable offsets

[Robert de Bath]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1828 bytes --]

On Tue, 1 Jun 2004, Eduardo Pereira Habkost wrote:

> I am forwarding these messages to you, to check if you have any clue on
> why this happened, and why the function worked on older versions.
>
> I've changed the function to use [bp-2] for the 'ds' variable, and now
> the kernel works flawlessly.
>
> Do you have an explanation on how this function.off stuff works, and if
> this have changed on older versions?

Oh my, confusion reigns.

Okay first the si and di variables are usually defined as callee saves
unless you're compiling with the '-Mc' option.

The "memcpy_fromfs.off" stuff is generated for optimising the push/pop
of the si and di variables. It's a reasonably recent addition so all
accesses of si and si can be eliminated if they are unused. However,
it's only turned on if you use '-O'.

Ie: this is working as it should.

You have highlighted a bug though, the definition some of the 'set'
variables is actually wrong, they should have memcpy_fromfs.off added
when it might be non-zero.

BTW: You should really be using those variables to access C arguments
and locals, the '_memcpy_fromfs.ds' is for stack pointer relative the
'.memcpy_fromfs.ds' is for 'bp' relative.

I can fix the bug in one of two ways; either actually add the
'memcpy_fromfs.off' variable to the 'set' variables or simply
have the compiler assume that any '#asm' will use si and di.
(The asm("...") function will not however.)

Actually I think assuming that #asm uses si/di is best because as this
example has shown it is quite likely to be true, plus, it's the failsafe
option.

(PS: It's also very easy to do :-) see attached. )

So do you want me to make it like it was before? :-)

-- 
Rob.                          (Robert de Bath <robert$ @ debath.co.uk>)
                                       <http://www.cix.co.uk/~mayday>


[-- Attachment #2: Type: TEXT/PLAIN, Size: 545 bytes --]

diff -Nurd linux86.old/bcc/table.c linux86/bcc/table.c
--- linux86.old/bcc/table.c	Sun Jul 28 08:43:13 2002
+++ linux86/bcc/table.c	Wed Jun  2 09:37:56 2004
@@ -400,6 +400,11 @@
     register struct symstruct *symptr;
     int i;
 
+    if (framep && optimise && !callersaves) {
+       regfuse |= callee1mask;
+       outnstr("! Assuming #asm uses all callee saves registers");
+    }
+
     for (i = 0; i < HASHTABSIZE; ++i)
 	for (symptr = hashtab[i]; symptr != NULL; symptr = symptr->next)
 	    if (symptr->storage == LOCAL)

Re: ELKS memcpy_fromfs() failing. Wrong variable offsets

[Eduardo Pereira Habkost]

[-- Attachment #1: Type: text/plain, Size: 3158 bytes --]

On Wed, Jun 02, 2004 at 09:47:25AM +0100, Robert de Bath wrote:
<snip>
> 
> Oh my, confusion reigns.

I guess so.  8)

> 
> Okay first the si and di variables are usually defined as callee saves
> unless you're compiling with the '-Mc' option.
> 
> The "memcpy_fromfs.off" stuff is generated for optimising the push/pop
> of the si and di variables. It's a reasonably recent addition so all
> accesses of si and si can be eliminated if they are unused. However,
> it's only turned on if you use '-O'.

I've seen that this .off stuff was added between 0.16.0 and 0.16.15. That
is why the (wrong) ELKS code used to work.

> 
> Ie: this is working as it should.
> 
> You have highlighted a bug though, the definition some of the 'set'
> variables is actually wrong, they should have memcpy_fromfs.off added
> when it might be non-zero.
> 
> BTW: You should really be using those variables to access C arguments
> and locals, the '_memcpy_fromfs.ds' is for stack pointer relative the
> '.memcpy_fromfs.ds' is for 'bp' relative.

Hey, nice! I've never looked at that code before, and I didn't know what
the author have assumed about the offsets of the variables, but
these [._]<function>.<variable> macros are what I was looking for
fixing the code. Thanks. :)

BTW: these [._]<function>.<variable>macros are available since old
versions, right? I will change ELKS code to use them.

> 
> I can fix the bug in one of two ways; either actually add the
> 'memcpy_fromfs.off' variable to the 'set' variables or simply
> have the compiler assume that any '#asm' will use si and di.
> (The asm("...") function will not however.)
> 
> Actually I think assuming that #asm uses si/di is best because as this
> example has shown it is quite likely to be true, plus, it's the failsafe
> option.
> 
> (PS: It's also very easy to do :-) see attached. )
> 
> So do you want me to make it like it was before? :-)

No, I just wanted to understand what happened. Now I understand the
Right Way to access variables and parameters inside #asm. :)

I think that forcing the user to save DI and SI if he wants to use the
registers is ok, unless it would break some existing software. It's up
to you. Adding a way to make #asm tell the compiler which registers it
will use (and maybe assume by default that it will change all registers)
would be interesting, too.

> 
> -- 
> Rob.                          (Robert de Bath <robert$ @ debath.co.uk>)
>                                        <http://www.cix.co.uk/~mayday>
> 

> diff -Nurd linux86.old/bcc/table.c linux86/bcc/table.c
> --- linux86.old/bcc/table.c	Sun Jul 28 08:43:13 2002
> +++ linux86/bcc/table.c	Wed Jun  2 09:37:56 2004
> @@ -400,6 +400,11 @@
>      register struct symstruct *symptr;
>      int i;
>  
> +    if (framep && optimise && !callersaves) {
> +       regfuse |= callee1mask;
> +       outnstr("! Assuming #asm uses all callee saves registers");
> +    }
> +
>      for (i = 0; i < HASHTABSIZE; ++i)
>  	for (symptr = hashtab[i]; symptr != NULL; symptr = symptr->next)
>  	    if (symptr->storage == LOCAL)


-- 
Eduardo

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[bcc patch] Fix variable offset macros for #asm (was: Re: ELKS memcpy_fromfs() failing. Wrong variable offsets)

[Eduardo Pereira Habkost]

[-- Attachment #1: Type: text/plain, Size: 4342 bytes --]

On Wed, Jun 02, 2004 at 09:47:25AM +0100, Robert de Bath wrote:
<snip>
> 
> You have highlighted a bug though, the definition some of the 'set'
> variables is actually wrong, they should have memcpy_fromfs.off added
> when it might be non-zero.

I am sending a fix to this bug. I hope that I didn't anything wrong,
as I am not familiar with bcc code.

I guess that creating an outoffset(variable) function that uses
<function>.off when needed would be better, but that was just a quick fix.

> 
> BTW: You should really be using those variables to access C arguments
> and locals, the '_memcpy_fromfs.ds' is for stack pointer relative the
> '.memcpy_fromfs.ds' is for 'bp' relative.

As we have a fix for bcc >= 0.16.8, I will send a fix for ELKS code,
for using .<function>.<variable>, now.  :)

> 
> I can fix the bug in one of two ways; either actually add the
> 'memcpy_fromfs.off' variable to the 'set' variables or simply
> have the compiler assume that any '#asm' will use si and di.
> (The asm("...") function will not however.)
> 
> Actually I think assuming that #asm uses si/di is best because as this
> example has shown it is quite likely to be true, plus, it's the failsafe
> option.

I prefer the code as it is now: if some #asm will use some register,
then it should save it. Or maybe a better one (IMO): having a way to
tell the compiler which registers will be used.

And even if assuming that all #asm will use SI and DI, it is a little
safer fixing the offsets, too. Just in case someone make more changes
to the code, try to optmize #asm, and break things because there are
implict things (like: dumplocs() only work because when using #asm,
<function>.off is zero) they didn't see.

Which fix do you plan to include on the next releases of bcc?

(BTW, I really missed a BCC_VERSION macro or equivalent. It would be
very useful in cases like this one)

--
Eduardo


--- dev86-0.16.15/bcc/codefrag.c	2002-08-03 13:38:27.000000000 -0300
+++ dev86-current/bcc/codefrag.c	2004-06-02 12:38:28.233060704 -0300
@@ -105,7 +105,6 @@
 # define outlswitch() (outload(), outstr(ireg0str), outncregname(DREG))
 # define outnc1() outnstr(",*1")
 # define outsbc() outop3str("sbb\t")
-# define outset() outstr ("\tset\t")
 # define outsl() outop2str("shl\t")
 # define outsr() outop2str("sar\t")
 # define outtransfer() outload()
@@ -134,6 +133,10 @@
     outstr(acclostr);
     outncregname(BREG);
 }
+PUBLIC void outset()
+{
+    outstr ("\tset\t");
+}
 PUBLIC void comment()
 {
     outstr("! ");
@@ -417,7 +420,6 @@
 # define outpil2switch() outnop2str("LDD\tD,X")
 # define outrolhi() outnop1str("ROLA");
 # define outsbc() outop2str("SBC")
-# define outset() outstr ("\tSET\t")
 # define outsl() outop1str("LSL")
 # define outtransfer() outop2str("TFR\t")
 # define reclaimfactor() outnstr ("++")	/* discard factor from stack */
@@ -434,6 +436,10 @@
 # define tfrhilo() outnop2str("TFR\tA,B")
 # define tfrlohi() outnop2str("TFR\tB,A")
 # define usr1() (outnop1str("LSRA"), outnop1str("RORB"))
+PUBLIC void outset()
+{
+    outstr ("\tSET\t");
+}
 PUBLIC void clrBreg()
 {
     outnop1str("CLRB");
--- dev86-0.16.15/bcc/table.c	2002-07-28 04:43:13.000000000 -0300
+++ dev86-current/bcc/table.c	2004-06-02 12:48:12.176287840 -0300
@@ -402,8 +402,38 @@
 
     for (i = 0; i < HASHTABSIZE; ++i)
 	for (symptr = hashtab[i]; symptr != NULL; symptr = symptr->next)
-	    if (symptr->storage == LOCAL)
-		set(symptr->name.namea, symptr->offset.offi - sp);
+	    if (symptr->storage == LOCAL) {
+		/* Variable offset relative to sp */
+		outccname(funcname);
+		outbyte(LOCALSTARTCHAR);
+		outstr(symptr->name.namea);
+		outset();
+		outshex(symptr->offset.offi - sp);
+		outnl();
+
+#ifdef FRAMEPOINTER
+		/* Variable offset relative to bp */
+		if (framep) 
+		{
+		    outbyte(LOCALSTARTCHAR);
+		    outstr(funcname);
+		    outbyte(LOCALSTARTCHAR);
+		    outstr(symptr->name.namea);
+		    outset();
+		    outshex(symptr->offset.offi - framep);
+#ifdef I8088
+#ifndef NO_DEL_PUSH
+		    if (optimise && !callersaves && symptr->offset.offi < framep) {
+			outbyte('+');
+			outstr(funcname);
+			outstr(".off");
+		    }
+#endif
+#endif
+		    outnl();
+		}
+#endif
+	    }
 }
 
 #ifdef HOLDSTRINGS

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [bcc patch] Fix variable offset macros for #asm (was: Re: ELKS memcpy_fromfs() failing. Wrong variable offsets)

[Miguel Bolanos]

I can't apply this patch either because is for dev86, so will forward it
to Bruce Evans.

thanks

Mike

On Wed, 2004-06-02 at 10:10, Eduardo Pereira Habkost wrote:
> On Wed, Jun 02, 2004 at 09:47:25AM +0100, Robert de Bath wrote:
> <snip>
> > 
> > You have highlighted a bug though, the definition some of the 'set'
> > variables is actually wrong, they should have memcpy_fromfs.off added
> > when it might be non-zero.
> 
> I am sending a fix to this bug. I hope that I didn't anything wrong,
> as I am not familiar with bcc code.
> 
> I guess that creating an outoffset(variable) function that uses
> <function>.off when needed would be better, but that was just a quick fix.
> 
> > 
> > BTW: You should really be using those variables to access C arguments
> > and locals, the '_memcpy_fromfs.ds' is for stack pointer relative the
> > '.memcpy_fromfs.ds' is for 'bp' relative.
> 
> As we have a fix for bcc >= 0.16.8, I will send a fix for ELKS code,
> for using .<function>.<variable>, now.  :)
> 
> > 
> > I can fix the bug in one of two ways; either actually add the
> > 'memcpy_fromfs.off' variable to the 'set' variables or simply
> > have the compiler assume that any '#asm' will use si and di.
> > (The asm("...") function will not however.)
> > 
> > Actually I think assuming that #asm uses si/di is best because as this
> > example has shown it is quite likely to be true, plus, it's the failsafe
> > option.
> 
> I prefer the code as it is now: if some #asm will use some register,
> then it should save it. Or maybe a better one (IMO): having a way to
> tell the compiler which registers will be used.
> 
> And even if assuming that all #asm will use SI and DI, it is a little
> safer fixing the offsets, too. Just in case someone make more changes
> to the code, try to optmize #asm, and break things because there are
> implict things (like: dumplocs() only work because when using #asm,
> <function>.off is zero) they didn't see.
> 
> Which fix do you plan to include on the next releases of bcc?
> 
> (BTW, I really missed a BCC_VERSION macro or equivalent. It would be
> very useful in cases like this one)
> 
> --
> Eduardo
> 
> 
> --- dev86-0.16.15/bcc/codefrag.c	2002-08-03 13:38:27.000000000 -0300
> +++ dev86-current/bcc/codefrag.c	2004-06-02 12:38:28.233060704 -0300
> @@ -105,7 +105,6 @@
>  # define outlswitch() (outload(), outstr(ireg0str), outncregname(DREG))
>  # define outnc1() outnstr(",*1")
>  # define outsbc() outop3str("sbb\t")
> -# define outset() outstr ("\tset\t")
>  # define outsl() outop2str("shl\t")
>  # define outsr() outop2str("sar\t")
>  # define outtransfer() outload()
> @@ -134,6 +133,10 @@
>      outstr(acclostr);
>      outncregname(BREG);
>  }
> +PUBLIC void outset()
> +{
> +    outstr ("\tset\t");
> +}
>  PUBLIC void comment()
>  {
>      outstr("! ");
> @@ -417,7 +420,6 @@
>  # define outpil2switch() outnop2str("LDD\tD,X")
>  # define outrolhi() outnop1str("ROLA");
>  # define outsbc() outop2str("SBC")
> -# define outset() outstr ("\tSET\t")
>  # define outsl() outop1str("LSL")
>  # define outtransfer() outop2str("TFR\t")
>  # define reclaimfactor() outnstr ("++")	/* discard factor from stack */
> @@ -434,6 +436,10 @@
>  # define tfrhilo() outnop2str("TFR\tA,B")
>  # define tfrlohi() outnop2str("TFR\tB,A")
>  # define usr1() (outnop1str("LSRA"), outnop1str("RORB"))
> +PUBLIC void outset()
> +{
> +    outstr ("\tSET\t");
> +}
>  PUBLIC void clrBreg()
>  {
>      outnop1str("CLRB");
> --- dev86-0.16.15/bcc/table.c	2002-07-28 04:43:13.000000000 -0300
> +++ dev86-current/bcc/table.c	2004-06-02 12:48:12.176287840 -0300
> @@ -402,8 +402,38 @@
>  
>      for (i = 0; i < HASHTABSIZE; ++i)
>  	for (symptr = hashtab[i]; symptr != NULL; symptr = symptr->next)
> -	    if (symptr->storage == LOCAL)
> -		set(symptr->name.namea, symptr->offset.offi - sp);
> +	    if (symptr->storage == LOCAL) {
> +		/* Variable offset relative to sp */
> +		outccname(funcname);
> +		outbyte(LOCALSTARTCHAR);
> +		outstr(symptr->name.namea);
> +		outset();
> +		outshex(symptr->offset.offi - sp);
> +		outnl();
> +
> +#ifdef FRAMEPOINTER
> +		/* Variable offset relative to bp */
> +		if (framep) 
> +		{
> +		    outbyte(LOCALSTARTCHAR);
> +		    outstr(funcname);
> +		    outbyte(LOCALSTARTCHAR);
> +		    outstr(symptr->name.namea);
> +		    outset();
> +		    outshex(symptr->offset.offi - framep);
> +#ifdef I8088
> +#ifndef NO_DEL_PUSH
> +		    if (optimise && !callersaves && symptr->offset.offi < framep) {
> +			outbyte('+');
> +			outstr(funcname);
> +			outstr(".off");
> +		    }
> +#endif
> +#endif
> +		    outnl();
> +		}
> +#endif
> +	    }
>  }
>  
>  #ifdef HOLDSTRINGS
-- 
--------------------miguel bolanos, systems administrator, linuxlabs
... ........ ..... ....                 230 peachtree st nw ste 2701
the original linux labs                          atlanta.ga.us 30303
       -since 1995                          http://www.linuxlabs.com
                                office 404.577.7747 fax 404.577.7743
--------------------------------------------------------------------

Re: ELKS memcpy_fromfs() failing. Wrong variable offsets

[Miguel Bolanos]

I can't apply this patch as this is for dev86.. this should go to Bruce
Evans...

I will forward it to him.

Thanks

Mike

On Wed, 2004-06-02 at 02:47, Robert de Bath wrote:
> On Tue, 1 Jun 2004, Eduardo Pereira Habkost wrote:
> 
> > I am forwarding these messages to you, to check if you have any clue on
> > why this happened, and why the function worked on older versions.
> >
> > I've changed the function to use [bp-2] for the 'ds' variable, and now
> > the kernel works flawlessly.
> >
> > Do you have an explanation on how this function.off stuff works, and if
> > this have changed on older versions?
> 
> Oh my, confusion reigns.
> 
> Okay first the si and di variables are usually defined as callee saves
> unless you're compiling with the '-Mc' option.
> 
> The "memcpy_fromfs.off" stuff is generated for optimising the push/pop
> of the si and di variables. It's a reasonably recent addition so all
> accesses of si and si can be eliminated if they are unused. However,
> it's only turned on if you use '-O'.
> 
> Ie: this is working as it should.
> 
> You have highlighted a bug though, the definition some of the 'set'
> variables is actually wrong, they should have memcpy_fromfs.off added
> when it might be non-zero.
> 
> BTW: You should really be using those variables to access C arguments
> and locals, the '_memcpy_fromfs.ds' is for stack pointer relative the
> '.memcpy_fromfs.ds' is for 'bp' relative.
> 
> I can fix the bug in one of two ways; either actually add the
> 'memcpy_fromfs.off' variable to the 'set' variables or simply
> have the compiler assume that any '#asm' will use si and di.
> (The asm("...") function will not however.)
> 
> Actually I think assuming that #asm uses si/di is best because as this
> example has shown it is quite likely to be true, plus, it's the failsafe
> option.
> 
> (PS: It's also very easy to do :-) see attached. )
> 
> So do you want me to make it like it was before? :-)
-- 
--------------------miguel bolanos, systems administrator, linuxlabs
... ........ ..... ....                 230 peachtree st nw ste 2701
the original linux labs                          atlanta.ga.us 30303
       -since 1995                          http://www.linuxlabs.com
                                office 404.577.7747 fax 404.577.7743
--------------------------------------------------------------------