Re: [hatari-devel] defaulting to SMALL_MEM

[Nicolas Pomarède <npomarede@xxxxxxxxxxxx>]

Le 16/02/2022 à 22:02, Eero Tamminen a écrit :
> Hi,
>
> On 15.2.2022 21.01, Thomas Huth wrote:
> > Am Sun, 13 Feb 2022 19:10:44 +0200
> > schrieb Eero Tamminen <oak@xxxxxxxxxxxxxx>:
> > > I.e. if screen pointer would be in such RAM area, should the other
> > > video related counters still change or not?
> >
> > Well, likely yes, but in this case I wonder if that'd buy us much. It
> > certainly would make the current ST-low rendering code (which is
> > already quite complex) way more complicated (and likely slower) if we
> > add checks there all over the place. And for what? As long as we don't
> > know of any program that relies on such weird behavior, it does not
> > have any benefit for us, does it?
>
> For the mono function it's trivial, but I hadn't realized how much 
> things would need to be changed or the color one before actually looking 
> into it.
>
> Attached is patch handling all the memcpy() cases,
> which is already quite large.  However, that is missing all the 
> additional word accesses through pVideoRasterEndLine pointer, used for 
> STE special cases, which would be even trickier to handle.
>
> => I concur that it's not really workable for Video_CopyScreenLineColor().
>
> (That function really is one scary monster. Would be nice if somebody 
> were able to split it into more manageable smaller functions.)

Hi

I agree this function is rather complicated in the "color" case due to 
all the possible overscan tricks to handle. I'm planning to rewrite it 
differently one day to handle cases that are even not supported today 
(such as mixing low res and med res on the same line).

In the meantime, I'm not sure it would be wise to apply your patch, it 
makes it even harder to read the code.

Regarding this possibility of reading after end of ram when converting 
screen pixels, maybe a simpler solution could be to add a "security 
margin" buffer after end of STRam in case we compile with SMALL_MEM ?

That is, instead of allocating STRamEnd bytes for STRam[], we allocate 
STRamEnd + SCREENBYTES_LINE*10 bytes and fill this extra space of 
SCREENBYTES_LINE*10 bytes with 0.

This way we can remove the tests like "> &STRam[STRamEnd]" + memset to 0 
when above the limit and happily copy whatever pVideoRaster points too.

I agree this may not be as clean as really checking all accesses are not 
above the limit, but in term of optimisation it would be much more 
efficient, considering the Video_CopyScreenLineXXX functions are called 
rather often ; as long as this extra security margin is documented when 
allocating RAM at start and in every Video_CopyScreenLineXXX functions, 
this would look like a good trade off to me.

Nicolas