search

libretro / libretro-cap32

caprice32 4.2.0 libretro
21 stars 34 forks source link

[WIP] Misaligned access fix #139

Closed madcock closed 2 months ago

madcock commented 7 months ago

Fix misaligned access that caused problems on SF2000 platform.

madcock commented 7 months ago

Context from the data_frog_sf2000 discord at https://discord.com/channels/741895796315914271/1099465777825972347/1176215621025939457

image

DSkywalk commented 7 months ago

Is something related with big/little endianess, isn't it? What is the endianess of your CPU? maybe we could use a gcc flag to detect when you compile with your MIPS devkit.

Check the endianness file and use your exclusive compiler flag: https://github.com/libretro/libretro-cap32/blob/master/cap32/retro_endianness.h

Thanks @madcock for your PR :)

bnister commented 7 months ago

It's not endianess. RendPos is uint32_t * but it points to a byte-aligned address, not a word-aligned one https://github.com/libretro/libretro-cap32/blob/e727310c86ef1dc1d1c3ffa2e7fa73b3c8dd0d0d/cap32/crtc.c#L1243 This upsets some platforms; x86 and ARM are immune, while MIPS is not, and needs a (costly) exception processing each time a write is issued. The fix is far from perfect (lowest CRTC horizontal position bits would become trimmed) so I vote not merging. And since it's actually relevant, the rendering should be done in byte or halfword quantities, esp. seeing something like this https://github.com/libretro/libretro-cap32/blob/e727310c86ef1dc1d1c3ffa2e7fa73b3c8dd0d0d/cap32/crtc.c#L1013

madcock commented 7 months ago

Also, since it's not obvious here, bnister == osaka in thread above. I asked for an explanation since I knew it would be better than any I could give. ;)

(So sounds like it makes sense to reject this PR, and instead investigate the source of the alignment issues, which is the way rendering is currently done.)

Thanks everyone!

DSkywalk commented 7 months ago

Ook, and thank you very much for the great explanation @bnister!

@madcock when you have the patch, just update this PR and I will be happy to merge it.

Thank you both! :+1:

LibretroAdmin commented 2 months ago

Hi @DSkywalk , should we merge this or not?

DSkywalk commented 2 months ago

No, as the developer wrote it is a very early fix that needed tweaks and also affects the correct emulation of the screen (crtc), as there has been no news step to close it and if the author wants to recover it in the future there is no problem to revisit it. Thanks!! :+1: