Maybe a bug

[DarwinNE]

Hi to all,
I encountered a strange behavior with z88dk and I am not sure if it is a bug. I will prepare a bug report on GitHub if you think it is the case.

I develop text adventure games that run on multiple platforms and I noticed a case where my games (based on a C source that is automatically generated and is almost identical for each platform) could not run correctly on the Spectrum.

I try to show here the problematic code, tested with z88dk 23469-0b7d9f00a9-20250517


This is a code that yields a problematic asm on z88dk:

Code: Select all

object *odummy;

/* many lines of code */

boolean drop(unsigned int o) __z88dk_fastcall
{
    odummy=search_object_p(o);\
    if(odummy->position==CARRIED){\
        odummy->position=current_position;\
    } else {\
        show_message(message1007);\
        return true;\
    }\
    return false;\
}

(the backslashes at the end of the lines is there as it is generated code that yields a macro instead of a function).

The object struct is defined as follows:

Code: Select all

typedef struct object_d {
    obj_code code;
    const char *desc;
    unsigned int position;      // Always int, as carried =1500, worn=1600
    unsigned char attributes;
} object;

The function search_object_p is declared as follows

Code: Select all

object *search_object_p(unsigned int o) __z88dk_fastcall
{
	/* some code*/
}

z88dk is run with options +zx -clib=ansi -lndos -create-app -Cz--screen and generates the following asm:

Code: Select all

; Function drop flags 0x00000208 __smallc __z88dk_fastcall 
; unsigned char booleandrop(unsigned int o)
; parameter 'unsigned int o' at sp+2 size(2)
	C_LINE	1476,"silkdust2_no_UTF8.c::drop::0::32"
._drop
	push	hl
	call	_search_object_p
	ld	(_odummy),hl
	inc	hl
	inc	hl
	inc	hl
	call	l_gint	;
	ld	de,1500
	and	a
	sbc	hl,de
	jp	nz,i_290	;
	ld	hl,(_odummy)
	inc	hl
	inc	hl
	inc	hl
	ex	de,hl
	ld	a,(_current_position)
	ld	(de),a
	jp	i_291	;EOS
.i_290
	ld	hl,_message1007
	call	_show_message
	ld	hl,1	;const
	pop	bc
	ret


.i_291
	ld	hl,0	;const
	pop	bc
	ret

The problem is that odummy->position=current_position does not seem to yield a correct result and odummy->position is not equal to current_position after the code is executed.

I noticed that the behavior is corrected if I read odummy->position, immediately after having updated its value. For instance, this code seems to work:

Code: Select all

object *odummy;
unsigned int dummy; 

/* many lines of code */

boolean drop(unsigned int o) __z88dk_fastcall
{
    odummy=search_object_p(o);\
    if(odummy->position==CARRIED){\
        odummy->position=current_position;\
        dummy=odummy->position;\
    } else {\
        show_message(message1007);\
        return true;\
    }\
    return false;\
}

Resulting asm code:

Code: Select all

; Function drop flags 0x00000208 __smallc __z88dk_fastcall 
; unsigned char booleandrop(unsigned int o)
; parameter 'unsigned int o' at sp+2 size(2)
	C_LINE	1476,"silkdust2_no_UTF8.c::drop::0::32"
._drop
	push	hl
	call	_search_object_p
	ld	(_odummy),hl
	call	l_gint3	;
	ld	de,1500
	and	a
	sbc	hl,de
	jp	nz,i_290	;
	ld	hl,(_odummy)
	inc	hl
	inc	hl
	inc	hl
	ex	de,hl
	ld	hl,(_current_position)
	ld	h,0
	call	l_pint
	ld	hl,(_odummy)
	call	l_gint3	;
	ld	(_dummy),hl
	jp	i_291	;EOS
.i_290
	ld	hl,_message1007
	call	_show_message
	ld	hl,1	;const
	pop	bc
	ret


.i_291
	ld	hl,0	;const
	pop	bc
	ret

Unfortunately, I do not read Z80 code well enough to he really helpful to understand what's wrong. My gut feeling is that there is a peephole optimization that does not consider the side effects due to odummy pointer being global and pointing towards an object in an array.

The program (called Silk Dust) is quite long, I can of course publish the complete C source but if it is necessary, I can try to see if I can obtain a minimal reproducible program that shows the problem.

[dom]

I can reproduce this but only if current_position is a char - which is surely wrong?

It's definitely an incorrect rule kicking in, now I need to find it!

Edit, that was an easy find. I've pushed a fix.

[DarwinNE]

Well spotted, I forgot to specify it, sorry!

As you notice, current_position is indeed an unsigned char.

It is not a problem in my code as current_position cannot contain anything above 256. Since the code generation is automatic, this is explicitly checked.

On the other hand, odummy->position can contain codes that exceeds 255.

Do you want I fill an issue on GitHub?

[dom]

No need for an issue - I've already found the problem and pushed a fix here: https://github.com/z88dk/z88dk/commit/0 ... f6150e48ee

[DarwinNE]

Well, I'm impressed. Hats off.

I haven't said it often enough, z88dk is an AMAZING project and I love it.

[jorgegv]

Well, just 1 hour from bug report to published fix...

Dom is a crack!