Re: [OpenBIOS] [PATCH 10/10] Adding filll Forth primitive

[Andreas Färber]

Am 16.08.2011 um 04:04 schrieb William Hahne:
> On 8/15/11, Andreas Färber  wrote:
> > Am 09.08.2011 um 23:55 schrieb William Hahne:
> > > This is a forth primitive that is required by BootX. It just fills
> > > some specified memory address and length with longs.
> > > Index: kernel/forth.c
> > >
> > > --- kernel/forth.c	(revision 1041)
> > > +++ kernel/forth.c	(working copy)
> > > @@ -1610,6 +1616,20 @@
> > >    memset(src, value, count);
> > > }
> > >
> > > +/*
> > > filll       ( addr len byte -- )
> > > */
> > >
> > > +static void filll(void)
> > > +{
> > > ucell value = POP();
> > > ucell count = POP();
> > > ucell *dest = (ucell *)cell2pointer(POP());
> > > int i;
> > > for (i = 0; i <= count / 4; i++) {
> > >    dest[i] = value;	
> > > }
> > >
> > > +}
> >
> > This word puzzles me: The extra l in filll seems to be for "long"
> > according to the patch description. But there's a hardcoded  
> > arithmetic
> > with 4 and the parameter is documented as byte yet is being written
> > ucell-wide. That strikes me as inconsistent.
>
> Honestly, my problem is I was unable to find documentation on filll.
> This suggests that it may be an non-standard addition by Apple. You
> are right about the inconsistency. I will make a revised version of
> the patch to address this.
> > Either 4 needs to be replaced with sizeof(ucell) to fit sparc64, or
> > the division by 4 dropped and uint8_t* written len times.
>
> It seems likely that it is supposed to fill with 32bit integers, but
> this would have to be checked on a 64bit platform which I do not have
> access to.

I have access to both Apple and IBM ppc64 machines and could check if  
you provide paste'able Forth code. :)
> This may not be applicable to sparc at all but again
> someone with access to the hardware would have to confirm.

In its present form, the patch seems in no way limited to ppc - you  
might want to look at the [IFDEF word or so if that's your intent.

Also note, your division by 4 leaves up to three trailing bytes un- 
filled for unaligned lengths. If intentional, you should add a comment  
saying so.

Further, it was suggested we use QEMU coding style for new  
contributions - the above function uses an inconsistent indentation.

Andreas

[PATCH 10/10] Adding filll Forth primitive

[Segher Boessenkool]

> I have access to both Apple and IBM ppc64 machines and could check  
> if you provide paste'able Forth code. :)

Apple's OF for 64-bit machines (version 5.x.x, on G5 machines) is a
32-bit OF.
> > This may not be applicable to sparc at all but again
> > someone with access to the hardware would have to confirm.
>
> In its present form, the patch seems in no way limited to ppc - you  
> might want to look at the [IFDEF word or so if that's your intent.

I would recommend all these words for BootX support go in a bootx.fs  
file,
which is only used where needed.  You do not want to always include all
these weird apple-isms :-)

Segher