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@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