Skip to content

Conversation

@rofl0r
Copy link

@rofl0r rofl0r commented Jun 28, 2014

the code was written in very unidiomatic C++ - basically 99% C with usage of less than a handfull C++ features. turning it entirely into C gives better portability (for example embedded or new systems without C++ compilers), and much better compilation speed.

rofl0r added 30 commits May 25, 2014 00:13
the header was copied from an old glibc version for some reason
which conflicts with the modern posix conformant version in
the libc include dir.

In file included from TestEmu.cpp:35:0:
../getopt.h:115:20: error: declaration of C function 'int getopt()' conflicts with
In file included from /usr/include/zconf.h:452:0,
                 from /usr/include/zlib.h:34,
                 from ../unzip.h:68,
                 from ../System.h:23,
                 from ../GBA.h:23,
                 from TestEmu.cpp:28:
/usr/include/unistd.h:124:5: error: previous declaration 'int getopt(int, char* const*, const char*)' here
with the update to png 1.4 some structs were made private and new
accessor functions added.

../Util.cpp: In function 'bool utilWritePNGFile(const char*, int, int, u8*)':
../Util.cpp:90:20: error: invalid use of incomplete type 'png_struct {aka struct png_struct_def}'
In file included from ../Util.cpp:25:0:
/include/libpng15/png.h:869:16: error: forward declaration of 'png_struct {aka struct png_struct_def}'
../Util.cpp: In function 'gzFile_s* utilGzOpen(const char*, const char*)':
../Util.cpp:995:73: error: invalid conversion from 'int (*)(void*, void*, unsigned int)'
to 'int (*)(gzFile, voidp, unsigned int) {aka int (*)(gzFile_s*, void*, unsigned int)}' [-fpermissive]
the profiling code does not compile currently, and uses some glibc
specific profiling infrastructure that's not 64bit safe.

prof.cpp: In function 'void profCleanup()':
prof.cpp:274:44: error: cast from 'char*' to 'u32 {aka unsigned int}' loses precision [-fpermissive]
prof.cpp:305:29: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
etc
we aim to compile them with the C compiler which is much faster.
only the win32 and gtk files use C++ constructs.
this involves changes to the build system as well as getting rid of some
C++ features that were used

- function overloading - split into funcs with different names
- unnamed unused arguments - give them a name
- global variables initialized from other global vars - use constant
  macros to initialize them

also the lex/yacc files were regenerated.
this file is not built in the default config so its C++isms slipped through.
these are the only headers that are conditionally included depending
on configure checks in the entire source tree, so we remove it for
consistency and just include them unconditionally.
this saves us from writing configure checks and implicit declaration
of inet_ntoa if the right HAVE_ define is not passed.
with C compilers getting better and better, there's hardly a reason
anymore to carry around decade old handwritten asm for a few selected
CPUs, plus the respective build system logic.
this one i had to double-check like 10x. should have added some
code initializing memory to 0, let the old code run and generate
table from difference.
it was worth it though, it's now crystal-clear which regs are
0xff-initialized, and which aren't.
gcc silently used gbSgbSetPalette instead of gbSgbSetPaletteB
due to lack of void inside the empty parameter list, so this
conversion error from C++ to C went unnoticed.
it was only detected by trying the file via cproc.
in order to prevent such mistakes in the future, add void
wherever no arguments are expected.

while at it, added static where possible and removed line-end
whitespace noise.
in my initial C port, i missed changing the prototype of the
no-argument function to use void in the parameter list, so
instead of warning about the use of the wrong function, GCC
silently called sdlReadPreferences instead of sdlReadPreferencesF.
the compiler does it automatically on high optimization levels,
or with -funroll-loops
system memcpy is usually heavily optimized with asm, 128-bit stores
and what have you, so it should be a lot faster than the manual
filling loop.
but same source. this allows to build both versions without
`make clean` or manually deleting SDL.o for each version-
SDL1 version uses SDL1.o, and SDL2 version SDL2.o.

the build now produces target bins VisualBoyAdvanceSDL1/2
respectively.
this gratuitous 2* here caused the pix memory to be overwritten
by far.
i actually wrote a test function to make sure this is invalid:

static void *guard_alloc(size_t n) {
        size_t ps = PAGE_SIZE, pages = 1+n/ps;
        char *mem = mmap(0, (pages+1)*ps, PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_$
        if(mem == MAP_FAILED) return 0;
        if(mprotect(mem + pages*ps, ps, PROT_NONE)) {
                munmap(mem, (pages+1)*ps);
                return 0;
        }
        return mem + pages*ps - n;
}

and allocated "pix" within GBA.c using this function - and indeed
ifbFunction crashed on first execution.
upon visual inspection, the IFB is applied to the entire graphics
buffer, so this fix is 100% correct.

this was broken since the very first commit that introduced IFB
in 2003 (d9a5dff), so apparently
nobody ever tested it with 32 system bit depth - or nobody noticed
the resulting memory corruption.
IFB operates on the input image at 1x scale, so there's nothing
preventing its use with SDL2's hardware scalers.
this seems to have been done previously by the autoconf scripts.
at least in case of SDL2 - SDL1 only supports the pull model.
currently there's a timing issue in the push model code, which
causes the game from "running away" from the sound - i.e. with
every minute that passes in-game the sound is lagging behind
more and more.
the push api call returns 0 on success not non-0 as
i assumed, therefore the SDL_Delay line was never
triggered.
@condret
Copy link

condret commented Mar 12, 2025

nice work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants