[OpenBIOS] [PATCH 18/24] aout: implement load/init-program as per IEEE-1275 specification

[BALATON Zoltan]

On Tue, 6 Sep 2016, Mark Cave-Ayland wrote:
> On 06/09/16 10:26, BALATON Zoltan wrote:
>> On Mon, 5 Sep 2016, Mark Cave-Ayland wrote:
>>> load should place the file at load-base, whilst init-program should parse
>>> the memory at load-base and set up the context accordingly.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland at ilande.co.uk>
>>> ---
[...]
>>> @@ -171,6 +158,21 @@ out:
>>> void
>>> aout_init_program(void)
>>> {
>>> -    // Currently not implemented
>>> -    feval("0 state-valid !");
>>> +    ucell start, size;
>>> +
>>> +    // Relocate a.out text down from load-base to load-base - header.
>>> This
>>> +    // is similar to what OBP and is needed for NextStep.
>>> +    fword("load-base");
>>> +    start = POP();
>>> +    feval("load-state >ls.file-size @");
>>> +    size = POP();
>>> +
>>> +    memcpy((char *)start - sizeof(struct exec), (char *)start, size);
>>
>> Shouldn't this be memmove() to handle overlapping regions correctly? (I
>> assume size is often bigger than sizeof(struct exec).) Maybe it works
>> with the current memcpy implementation in OpenBIOS but in case it's ever
>> replaced memmove may be better.
>
> I'm not sure that would have an effect here, since the copy in memory is
> always downwards from higher memory locations to lower memory locations

It wouldn't in this particular case because the implementation of memcpy 
in OpenBIOS will work but that's only coincidence.

> (and I can't see how anyone would implement memcpy() backwards as it
> would be extremely cache unfriendly). But feel free to point out sources
> that suggest otherwise :)

I can only point to documentation (e.g. man pages) that say that if memory 
regions are overlapping memcpy's behaviour is undefined but memmove is 
guaranteed to work so it's safer to use that for known overlapping 
regions. As in OpenBIOS we are using the built in implementations one can 
rely on the memcpy implementation but I think it's cleaner to use memmove 
here and not rely on something not guaranteed even if it happens to work.

So this works both ways here just looks better with memmove IMO. This is 
not a real problem with this patch just a suggestion.

Regards,
BALATON Zoltan