Re: [hatari-devel] defaulting to SMALL_MEM

[ Thread Index | Date Index | More Archives ]

Le 16/02/2022 à 22:02, Eero Tamminen a écrit :

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.)


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.


Mail converted by MHonArc 2.6.19+