-
Notifications
You must be signed in to change notification settings - Fork 22
Description
Splitting from #84: libxmp currently requires a stack size of (rounded up) 32768 or larger. This is easy to manually set with the stack command before launching xmp, but there are ways to do this in code. From least annoying to most annoying, roughly:
- Workbench 3.2, AmigaOS 4 shells check for a "stack cookie" in the binary in the form
$STACK: [size in decimal]. This can be static or global, I made it global in the below patch to work around old compilers lacking__attribute__((used)). (IIRC they also have a minimum stack size of 32k, and are unaffected.) Clib2 checks for a global named__stack_sizeduring CRT init.- libnix checks for a global named
__stackduring CRT init if the binary objectswapstack.ois included. No mechanism is provided to automatically include this object, meaning xmp would have to locate it in configury. Workbench 3.x builds apparently have a workaround to pull it in, but as far as I can tell, the libnix variants packaged with these toolchains are for Workbench versions 1.3, 2.0, and 4? The adtools toolchain (haven't checked bebbo) has-mstackexpandto allow for a dynamic stack size. libxmp and xmp both need to be built with this to work.- AROS has the function
NewStackSwapto safely swap stacks in C code (again, they increased the default to 32k, I think?). MorphOS has the equivalentNewPPCStackSwap, and its stack size needs to be queried via a different function. - Workbench 2.04 through 3.1 have the older
StackSwapfunction, which is safe to use through inline assembly. This needs to be disabled forWorkbench 1.3 andAmigaOS 4, etc. builds if it is included.
This can't be fixed easily in libxmp: the two main culprits are the 16k buffers in MD5 calculation and external depacking, which can be reduced to lower the required stack value, but the loaders (or possibly elsewhere) would all have to be audited for stack usage. I tested the XM loader primarily, which needs 8k stack. Getting everything comfortably under 4k is probably not reasonably achievable.
(Seems like a lot of developer headache could have been avoided by someone having added something like -mstacksize=32768 to GCC to handle all of this automatically in CRT init.)
Ah, forgot the stack size can be a problem because I didn't need to use the stack command on aros.
And didn't know about the stack cookie thing either.
We have this piece of code in our amiga-specific sources (amigaos3, aros, morhpos -- don't know if its suitable for amigaos4, though), maybe if it'd be useful here? https://github.com/sezero/uhexen2/blob/m68k-amigaos/engine/hexen2/sys_amiga.c#L38-L70 And the aros-specific part: https://github.com/sezero/uhexen2/blob/master/oslibs/aros-x86/misc/include/incstack.h
This fixes the crash on my end, likely needs a lot more testing (whichever libnix/WB version the default is with the old adtools m68k toolchain):
diff --git a/src/main.c b/src/main.c index f1694dfe..dcaf8de9 100644 --- a/src/main.c +++ b/src/main.c @@ -18,6 +18,7 @@ #include <sys/time.h> #endif #if defined(XMP_AMIGA) +#include <proto/exec.h> #include <proto/timer.h> #include <time.h> #endif @@ -45,6 +46,25 @@ #include <windows.h> #endif + +/* AmigaOS and derivatives require manually setting the stack size. */ +#if defined(XMP_AMIGA) +#define MIN_STACK_SIZE 65536 + +/* AmigaOS 3.2, 4.x */ +extern const char stack_cookie[]; +const char stack_cookie[] = "$STACK:65536"; + +#if defined(__CLIB2__) +extern int __stack_size; +int __stack_size = MIN_STACK_SIZE; +#else +extern ULONG __stack; +ULONG __stack = MIN_STACK_SIZE; +#endif +#endif + + static const struct sound_driver *sound; static unsigned int foreground_in, foreground_out; static int refresh_status; @@ -234,7 +254,7 @@ static void load_error(const char *name, const char *filename, int val) } -int main(int argc, char **argv) +static int real_main(int argc, char **argv) { xmp_context xc; struct xmp_module_info mi; @@ -655,3 +675,36 @@ int main(int argc, char **argv) return 0; } + +int main(int argc, char **argv) +{ + /* Do not use any stack variables in this function. */ + static int ret; + +#if defined(XMP_AMIGA) + static struct StackSwapStruct sw; + static struct Task *mytask; + static long stack_size; + + mytask = FindTask(NULL); + stack_size = (UBYTE *)mytask->tc_SPUpper - (UBYTE *)mytask->tc_SPLower; + if (stack_size < MIN_STACK_SIZE) { + sw.stk_Lower = AllocVec(MIN_STACK_SIZE, MEMF_PUBLIC); + sw.stk_Upper = (ULONG)sw.stk_Lower + MIN_STACK_SIZE; + sw.stk_Pointer = (APTR)sw.stk_Upper; + StackSwap(&sw); + } +#endif + + ret = real_main(argc, argv); + +#if defined(XMP_AMIGA) + /* Restore original stack. */ + if (stack_size < MIN_STACK_SIZE) { + StackSwap(&sw); + FreeVec(sw.stk_Lower); + } +#endif + + return ret; +}Also, I'm not sure that this usage is safe, even when avoiding stack variables in this manner.
(16384 results in a crash with a basic test XM, 32768 works fine, I doubled it for safety. The main culprit seems to be the libxmp load.c:45 stack-allocated buffer for calculating MD5s. There's another 16k stack buffer in the depackers, too, but they should never co-exist. Lowering these both to 2048 makes a stack size of 8192 work for this test module, but not the default 4096.)
Also, setting the stack to 32768+ also fixes my bebbo toolchain builds.