z88dk / z88dk

The development kit for over a hundred z80 family machines - c compiler, assembler, linker, libraries.
https://www.z88dk.org
Other
907 stars 171 forks source link

classic: zsdcc incompatible with qsort #127

Closed aralbrec closed 7 years ago

aralbrec commented 7 years ago

This will affect any assembly function that accepts function pointers to smallc functions.

qsort() accepts a pointer to a compare function. qsort is written in assembly language and expects to invoke the compare function using smallc linkage.

sdcc, however, is unable to compile c code using smallc convention - it can only call assembly code using smallc linkage.

This program will not work:

int ascending_order(int *a, int *b) __smallc
{
   return *a - *b;
}

void main(void)
{
   ...
   qsort(numbers, NUM, sizeof(int), ascending_order);
   ...
}

sdcc will ignore the __smallc qualifier for ascending_order() and qsort() will therefore effectively call ascending_order() with parameters reversed, leading to a descending order sort.

I think we need to do two things here:

suborb commented 7 years ago

I think I need to add in an error/warning, but this may well be another solution:

int myfunc(void (*func)() __stdc)
{
        func(1,2); // Correctly called in r->l format
}

int test(int v, int b) __smallc
{
    return b;
}
int test2(int v, int b) __stdc
{
   return b;
}

int main()
{
        void (*var)() __stdc;

        var = test;   // Warning generated - correct
        var = test2; // No warning - correct

        myfunc(test); // No warning - this is a bug
        myfunc(test2); // No warning - correct
}

myfunc() is generating the code with a R->L call. test2() is generated with r->l access. So if I add in a warning/error for the myfunc(test) line then the same qsort() library code can be shared, but it's a little inconvenient to use from sccz80 (but...how often is qsort() actually used?).

I remember leaving qsort/memop* etc inaccessible to sdcc for this exact reason.

Alternatively, we could use ix as a pointer to a (stack buffer) holding the comparator and a flag and setup parameters so the call is correct (similar to the trick in printf for sdcc/sccz80).

aralbrec commented 7 years ago

I have a solution ready that uses macros in the stdlib.h header to select between an sdcc implementation and an sccz80 implementation of qsort at compile time.

extern void __LIB__  qsort_sccz80(void *base, unsigned int nel, unsigned int width, void *compar) __smallc;
extern void __LIB__  qsort_sccz80_callee(void *base, unsigned int nel, unsigned int width, void *compar) __smallc __z88dk_callee;

extern void __LIB__  qsort_sdcc(void *base, unsigned int nel, unsigned int width, void *compar) __smallc;
extern void __LIB__  qsort_sdcc_callee(void *base, unsigned int nel, unsigned int width, void *compar) __smallc __z88dk_callee;

#ifdef Z88DK_USES_SDCC

#define qsort                  qsort_sdcc
#define qsort_sdcc(a,b,c,d)    qsort_sdcc_callee(a,b,c,d)

#else

#define qsort                  qsort_sccz80
#define qsort_sccz80(a,b,c,d)  qsort_sccz80_callee(a,b,c,d)

#endif

Compilation will be transparent with the understanding that sccz80 must use a smallc compare whereas sdcc must use a standard compare (and these are of course what happens without function decorations so there is source code compatibility).

If you mix sccz80 and sdcc produced object files, programs will still work but there will be two copies of qsort in the output binary, one for each compiler.

And like you say these are fringe use-cases anyway.

I'll commit the change and you can have a look.

aralbrec commented 7 years ago

Unfortunately I had all this stuff in the master branch locally and my git foo is not that strong yet so it's in the last two commits to master.

The testing is being done in the sorting benchmark. sort.c will now compile correctly without changes to the source.

aralbrec commented 7 years ago

I remember leaving qsort/memop* etc inaccessible to sdcc for this exact reason.

I'd say slate memop* for removal. It's too non-standard. But a compliant bsearch is missing that will have the same problem. bsearch is implemented properly in the new c lib so it could just be copied over with minor changes. But I was thinking we could save a homogenization effort for after a library reorganization, or at least I've got too many things going to worry about it yet.

suborb commented 7 years ago

Looks good, I'll close this one now since the actual issue is resolved.

aralbrec commented 7 years ago

sdcc has been updated to generate errors for the cases mentioned.

I'm not sure what Philip means by " In revision #9868, support for the Small-C calling convention in callees has been implemented." because I'm under the impression smallc callee linkage is already working.

Updated patch and windows binary in the usual place (9869).