Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Loop variable adding ptr offset to value #1188

Open
Elthial opened this issue Jul 7, 2022 · 2 comments
Open

Bug: Loop variable adding ptr offset to value #1188

Elthial opened this issue Jul 7, 2022 · 2 comments

Comments

@Elthial
Copy link

Elthial commented Jul 7, 2022

It appears a [BP -2h] is not being declared inside a If scope and then from that point forward the [BP -2h] starts causing FP-EvenNum errors in the code as Reko loses tracking on that base pointer offset.

Incorrect Reko HL:

int16 wLoc0E_824 = 0x00;
        do
        {
            if (ds->ptr5416->a3954[wLoc0E_824] != 0x00)
            {
                Eq_11130 wLoc28_826 = 0x33;
                do
                {
                    //// Notice how this is an empty do loop? 
                    ///This by context should be setting indexed variables or incrementing a counter
                    ///
                    Eq_11130 v39_295 = (word16) wLoc28_826 + 1;
                    wLoc28_826 = v39_295;
                } while (v39_295 <= 0x55);
            }
            int16 v40_302 = wLoc0E_824 + 0x01;
            wLoc0E_824 = v40_302;
        } while (v40_302 < 0x04);

I've read the ASM and think it should be:
(Which makes sense, its cycling through damaged locations on a vehicle checking for a component (0x22) as part of a repair function)

[bp -2h] = 0x00;
int16 wLoc0E_824 = 0x00;
        do
        {
            if (ds->ptr5416->a3954[wLoc0E_824] != 0x00)
            {
                Eq_11130 wLoc28_826 = 0x33;
                do
                {
                    if (ds->ptr5412->tC918[wLoc28_826] = 0x22)
						[bp -2h]++

                    Eq_11130 v39_295 = (word16) wLoc28_826 + 1;
                    wLoc28_826 = v39_295;
                } while (v39_295 <= 0x55);
            }
            int16 v40_302 = wLoc0E_824 + 0x01;
            wLoc0E_824 = v40_302;
        } while (v40_302 < 0x04);

ASM:

0DAB:0259 83 7E FA 02 cmp word ptr [bp-6h],2h
0DAB:025D 7D 03 jge 0262h
0DAB:025F E9 BD 00 jmp 031Fh
0DAB:0262 C7 46 FE 00 00 mov word ptr [bp-2h],0h
0DAB:0267 C7 46 F4 00 00 mov word ptr [bp-0Ch],0h
0DAB:026C 8B 5E F4 mov bx,[bp-0Ch]
0DAB:026F D1 E3 shl bx,1h
0DAB:0271 8E 06 16 54 mov es,[5416h]
0DAB:0275 26 83 BF 54 39 00 cmp word ptr es:[bx+3954h],0h
0DAB:027B 74 28 jz 02A5h
0DAB:027D C7 46 DA 33 00 mov word ptr [bp-26h],33h
0DAB:0282 B8 7D 00 mov ax,7Dh
0DAB:0285 F7 6E F4 imul word ptr [bp-0Ch]
0DAB:0288 8B D8 mov bx,ax
0DAB:028A 03 5E DA add bx,[bp-26h]
0DAB:028D 8E 06 12 54 mov es,[5412h]
0DAB:0291 26 80 BF 18 C9 22 cmp byte ptr es:[bx+0C918h],22h
0DAB:0297 75 03 jnz 029Ch
0DAB:0299 FF 46 FE inc word ptr [bp-2h]
0DAB:029C FF 46 DA inc word ptr [bp-26h]
0DAB:029F 83 7E DA 55 cmp word ptr [bp-26h],55h
0DAB:02A3 7E DD jle 0282h
0DAB:02A5 FF 46 F4 inc word ptr [bp-0Ch]
0DAB:02A8 83 7E F4 04 cmp word ptr [bp-0Ch],4h

@Elthial
Copy link
Author

Elthial commented Jul 8, 2022

[[reko::address("0800:0E4B")]] void Graphical_Operations_0E4B();

Has the same issue / similar issue.
I noticed the first for loop seemed to added the ptr to the variable value.

Eq_2294 wLoc4E_1120 = 17142;             //<----- Should be 0x00
    do
    {
        ds->ptr53B2->*wLoc4E_1120 = 0x00;
        Eq_2294 v15_39 = (word16) wLoc4E_1120 + 1;
        wLoc4E_1120 = v15_39;
    } while (v15_39 < 48418);         //<---- Should be 0x18

ASM:

0800:0E8A C7 46 B4 00 00 mov word ptr [bp-4Ch],0h
0800:0E8F 8B 5E B4 mov bx,[bp-4Ch]
0800:0E92 8E 06 B2 53 mov es,[53B2h]
0800:0E96 26 C6 87 F6 42 00 mov byte ptr es:[bx+42F6h],0h
0800:0E9C FF 46 B4 inc word ptr [bp-4Ch]
0800:0E9F 83 7E B4 18 cmp word ptr [bp-4Ch],18h

@Elthial
Copy link
Author

Elthial commented Jul 11, 2022

Okay, Pretty sure I know what the bug is:

	const unsigned short TerrainOffSet = 0x07A4;
	do
	{
		if ((ds->ptr53C8->*(TerrainGridLocation + TerrainOffSet) & 0x80) != 0x00) //Memory ref: [246C:07A4]
			Bool_HasMech = 0x00;
		
		TerrainGridLocation++;
	} while (TerrainGridLocation < 0x09);

ASM: (0800:4D57)

mov word ptr [bp-4h],1h
mov word ptr [bp-2h],0h
mov bx,[bp-2h]
mov es,[53C8h]
test byte ptr es:[bx+7A4h],80h
jz 4D80h
mov word ptr [bp-4h],0h
inc word ptr [bp-2h]
cmp word ptr [bp-2h],9h

Reko is definitely rolling the ptr offset backwards into the start value.
Cmp then is still valid (in this case) +1. (Less than 9 but range difference is 10)

I can see why that might happen as an optimisation but it makes it human readability worse as they are then non-human values. Perhaps leaving it as two statements + Offset instead of merging the Offset into the

I'll see if I can analysis the procedure decompilation to find where it starts going off-track.

@Elthial Elthial changed the title Bug: Reko doesn't declare [BP -2h] as variable in HL Bug: Loop variable adding ptr offset to value Jul 11, 2022
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

No branches or pull requests

1 participant