#3834 [Z80][SDCC 4.5] Compiler bug

[Aoineko]

Hello,

I tested all the 50 samples of the MSXgl library with the new SDCC 4.5 version.
Everything works fine except 1 sample called "s_swsprt" for which there are rendering glitches.
The exact same code works fine with SDCC 4.2, 4.3 and 4.4.
I packaged all compiled output files for each version in the attached SDCC_4.5_bug.zip file.
You can test the .ROM files by drag & drop them on the online MSX emulator: https://webmsx.org.
With SDCC 4.5, adding a line of code after the bug location fix the issue.
I think the problem could be about inline functions.
You can compare the s_swsprt.asm generated in the out_4.5_bug/ folder and out_4.5_fix/.

BTW, I've been able to verify that not only does SDCC 4.5 correct the problems of increased final binary file sizes present in SDCC 4.3 and 4.4, but it even produces binaries that are generally smaller than SDCC 4.2, which was the reference in the field.
All the results can be found here: https://docs.google.com/spreadsheets/d/1ZTBx3nAW32ZxbbLYjrwJ2ISelW34hyPq6hx7q86Mobk/edit?usp=sharing

Once the bug in the “s_swsprt” sample has been corrected, I'll do some performance tests, as I have the impression that the code generated is, at least sometimes, less efficient than with SDCC 4.2.

SDCC version: 4.5.0 #15242 (MINGW64)

[Janko Stamenović]

The way I read that report is: if two different programs are compiled, they produce different pictures. You show that two different programs on 4.5.0 produce different pictures, but they are different, as you say: "adding a line of code after the bug location fix the issue."

The question is, how can it be demonstrated that it's an actual problem in the compiler, and not the bug in the drawing code which was simply masked in previous versions of the compiler, that you only now fixed in your code?

[Aoineko]

You're right. I don't have formal proof that the bug wasn't hidden by older versions of SDCC, but it seems very unlikely to me.
I've tried to isolate the bug, but the problem is that it disappears as soon as I modify the code.
Here's where the bug occurs:

    VDP_CommandHMMV(0, 0, src->Width, HEIGHT, src->Background);
    for(u8 i = 0; i < 255; ++i)
    {
        u16 x = Math_GetRandom16() % src->Width;
        u16 y = Math_GetRandom16() % (HEIGHT - 24) + 16;
        VDP_CommandLMMV(x, y, pixelWidth, 1, src->Red, VDP_OP_IMP);
        x = Math_GetRandom16() % src->Width;
        y = Math_GetRandom16() % (HEIGHT - 24) + 16;
        VDP_CommandLMMV(x, y, pixelWidth, 1, src->White, VDP_OP_IMP); // <<BUG HERE>>
    }

    // Initialize sprite
    VDP_CommandHMMV(0, HEIGHT, sprtWidth * 6, 16, 0);
    for(u8 i = 0; i < 6; ++i)
    {
        VDP_CommandLMMC(src->DataLMMC + i * (16 * 16 * pixelWidth), i * sprtWidth, HEIGHT, sprtWidth, 16, VDP_OP_TIMP);
    }

The VDP_CommandXXX functions are inline functions that setup a structure in RAM then call a function that copy it to the video processor.
The bug appear to be the structure to not been setup correctly when the code is as it.
If I move the sprite initialization code else where, for example, the generated assembler code for the bugged part is modified and the bug disappear.

I think someone with a good knowledge of z80 assembler and how the compiler works will easily find out what's going on.
As for me, I'll keep trying to isolate the problem by simplifing the code that generates the bug.

[Aoineko]

I made some progress.

The bug is linked to a local variable (sprtWidth), located at IX-17.
When the bug is present (with the original code), the use of this variable in first VDP_CommandLMMV() is not correct: the assembler code thinks that the value is in HL when it is not.
In the 2nd use, the value is correctly recovered from the right offset of IX.

Here is bugged assembler code (note that HL is got from AF value, what is nonsense):

;./s_swsprt.c:154: VDP_CommandLMMV(x, y, pixelWidth, 1, src->Red, VDP_OP_IMP);
    ld  l, -7 (ix)
    ld  h, -6 (ix)
    ld  de, #0x0008
    add hl, de
    ld  a, (hl)
    ld  e, -17 (ix)
    ld  d, #0x00
;D:\Dev\Private\MSX\MSXgl\engine/src/vdp_inl.h:298: g_VDP_Command.DX = dx; 
    push    de
    ld  hl, #(_g_VDP_Command + 4)
    push    af
    ld  a, -3 (ix)
    ld  (hl), a
    inc hl
    ld  a, -2 (ix)
    ld  (hl), a
    pop hl ; <<BUG:HL become AF but should be (IX-17)>>
    pop af
;D:\Dev\Private\MSX\MSXgl\engine/src/vdp_inl.h:299: g_VDP_Command.DY = dy; 
    ld  ((_g_VDP_Command + 6)), bc
;D:\Dev\Private\MSX\MSXgl\engine/src/vdp_inl.h:300: g_VDP_Command.NX = nx; 
    ld  ((_g_VDP_Command + 8)), hl ; <<BUG:HL is incorect>>
;D:\Dev\Private\MSX\MSXgl\engine/src/vdp_inl.h:301: g_VDP_Command.NY = ny; 
    ld  hl, #0x0001
    ld  ((_g_VDP_Command + 10)), hl
;D:\Dev\Private\MSX\MSXgl\engine/src/vdp_inl.h:302: g_VDP_Command.CLR = col; 
    ld  hl, #(_g_VDP_Command + 12)
    ld  (hl), a
;D:\Dev\Private\MSX\MSXgl\engine/src/vdp_inl.h:303: g_VDP_Command.ARG = arg; 
    inc hl
    ld  (hl), #0x00
;D:\Dev\Private\MSX\MSXgl\engine/src/vdp_inl.h:304: g_VDP_Command.CMD = VDP_CMD_LMMV + op;
    inc hl
    ld  (hl), #0x80
;D:\Dev\Private\MSX\MSXgl\engine/src/vdp_inl.h:305: VPD_CommandSetupR36();
    push    bc
    push    de
    call    _VPD_CommandSetupR36

Attached, the .C file and the corresponding .ASM.
Note that I was able to slightly simplify the loop while preserving the bug.

[Philipp Klaus Krause]

At first sight, it looks like the problem is a swapped order of the push de, push af vs. the pop hl, pop af.

Loos the same at second sight, too. Apparently a codegen bug in src/z80/gen.c, genPointerSet. Still, I'd like code to reproduce the issue, so I can write a regression test for it.

[Aoineko]

Ok, it seems the "only" problem is that the compiler think the value of sprtWidth is in HL register when setting g_VDP_Command.NX, while in fact, it is in DE.

I'm surprised no one else has come across this SDCC 4.5 bug.

[Philipp Klaus Krause]

Could you give us a compileable example, which does not depend on anything that is not part of SDCC? Just the affected function and all necessary declarations in one source file?

[Aoineko]

I understand that it would be far easier to check the problematic code if it were isolated, but it's a big task to remove all the dependencies and I don't necessarily have the time right now.
In the meantime, if you want to test the bug and a fix, you can clone MSXgl (https://github.com/aoineko-fr/MSXgl.git), change the SDCC path to your 4.5 version (SDCCPath) in projects/samples/project_config.js, then just execute there: “build s_swsprt” (or “./build.sh s_swsprt” on Linux).
You'll have all the intermediate files in /out/.

[Janko Stamenović]

Attached the reduced code which compiles only with sdcc -c -mz80 s_swsprt.c and still produces in the asm:

;s_swsprt.c:115: g_VDP_Command.DY = dy;
    push    hl
    ld  hl, #(_g_VDP_Command + 6)
    push    af
    ld  a, -3 (ix)
    ld  (hl), a
    inc hl
    ld  (hl), #0x00
    pop hl
    pop af
;s_swsprt.c:116: g_VDP_Command.NX = nx;
    ld  ((_g_VDP_Command + 8)), hl    

(edit: added the lines to see that the hl is used wrong)

[Janko Stamenović]

I've modified code so that after the InitScreen function is run as: SX = 64; SY = 64; InitScreen(); the buffer words is populated with the values from the calls which go wrong (I hope) and the buffer buff with the textual representation of the same values (I can see them so I'm sure that works). There are total 4 logged calls, and two calls are wrong: the first and the third. On that web emulator, linked with the rest of the runtime these 4 values are: 0xa010, 1, 0xa010, 1 and the "right" values should be all four times 1.

[Janko Stamenović]

Edit: here now the minimal version.
sdcc -mz80 bug_3834.c should still produce the bug. That words is filled as described in the c file (now 5 words, and different values, two from 5 are bad).
For higher optimizations I saw the correct values, for default I see that bad code (using the official mingw64 build of 4.5.0, and also my current build on Linux).

[Aoineko]

Thank you very much Janko.

I hope this will enable the SDCC team to fix this bug quickly.
When this kind of compilation bug is fixed, is a new official version of SDCC released?

[Janko Stamenović]

I think you will have to use a snapshot build in your project, if you want to use it before the new version is released. As far as I understood, the tradition of SDCC is to publish a new "official release" each year: 4.5 in 2025, 4.0 in 2020, and 3.5 in 2015, see:

https://sourceforge.net/projects/sdcc/files/sdcc-win64/

My experiments suggested that there's a chance that you can avoid that bug in your examples by simply adding something like e.g.

--max-allocs-per-node9000

(or a bigger number) to your compilation options. As I tried to reproduce, I saw the problem go away with higher settings, and also with --opt-code-size.

Edit: tried: even if the above is true for the smaller examples as I've tried to reduce, it seems that your full C code is more "robust" against the optimizations(!)

BTW you should definitely learn to produce these kinds of C files that don't depend on any includes yourself, by adding the option :

  -E  --preprocessonly      Preprocess only, do not compile

to your compilation and then sending the output for a given .c file produced that way.

[Philipp Klaus Krause]

The SDCC 4.5.0 release will probably still be the most stable version of SDCC. Your options are:

• SDCC 4.5.0, containing this (and some other) known bugs. But well-tested, and with many, many fixed bugs.
• Current snapshot after this bug is fixed. You'll get the fix for this bug and a few others, but also new features and optimizations that might cause regressions elsewhere.
• Pick the bug fixes from current trunk, and apply them on top of SDCC 4.5.0. That will only give you the lower-risk changes. But you get a version that didn't go through all the testing that was done on SDCC 4.5.0 and the current trunk.

We've had SDCC 4.5.0 RC1 on 2025-01-03, and the release on 2025-01-28 (with RC2 and RC3 in between). That was 26 days of testing and fixing release candidates, more than usual for an SDCC release.

[Aoineko]

It's not that simple. The MSXgl library is used by quite a few MSX developers and I can't ask them to build their own version of SDCC to have a stable compiler. What's more, building a version manually, picking up patches here and there, doesn't provide any guarantees, as you end up with a version that nobody has ever tested.

26 days may be a long time for those who are on this forum every day, but personally I missed this period because I was busy elsewhere (I was finishing a game with a January 31 deadline).

Anyway, I'm going to continue my tests with SDCC 4.5 in particular to see how it performs, but this bug seems far too dangerous for me to consider this version as the new minimum version for MSXgl, which will remain the super-stable SDCC 4.2 version.

Thanks for the quick fix.

[Janko Stamenović]

It is simpler than you think it is. You have your really nice set of programs you can use for tests, and you can just take a snapshot 15282 and run your tests with it, and you already include the SDCC binaries your package, so there's really nothing against including the snapshot 15282 in the package.
The "official releases" in the release process of SDCC by design aren't "the known optimum". The users can have their own "known" optimum, for their own use cases, which definitely can be a snapshot.
I admit my perspective is biased, as I expect from SDCC both the best and smallest code it can produce, and the "most correct" even for the "unknown" C input, and I still think 4.2.0 can't be better than what exists now in 15282. If 4.2.0 appears more stable to you it doesn't mean it's for your own purposes in any way better than the snapshot 15282. I definitely suggest you to save it, both the source and the binaries... until some more recent version with which you could be even more satisfied appears.

[Janko Stamenović]

That was from the pure "what works" perspective. But I know some companies (read: managers) insist on using "just releases" and nothing else. If such a company actually cares about "just releases", the consistence of the timing of the official releases though the years allows it to very easily schedule their own tests to happen before each official release. In your case, you certainly didn't have to run your set of tests only in January: the problem was probably visible long ago, and now that you know it, if you would use some much older snapshots you could probably see it and confirm. Maybe it was harder to cause it to be manifested, maybe not, but it could be that it was present since a while: the specific poporder, where the bug was in this case, seems to have been the same since 2021, and is only changed now.
In short: whichever tests the compiler team does, they can't protect you from having to run your own tests to see that the code you write works correctly.

---

Additionally and tangentially, as I've had to reduce the code from your library, I some observations related to the "principles" on which it depends: the way the library is written and used is from the perspective of both the resulting code size and speed suboptimal. Specifically in this loop:

    for(u8 i = 0; i < 255; ++i)
    {
        u16 x = Math_GetRandom16() % src->Width;
        u16 y = Math_GetRandom16() % (HEIGHT - 24) + 16;
        VDP_CommandLMMV(x, y, pixelWidth, 1, src->Red, VDP_OP_IMP);
        x = Math_GetRandom16() % src->Width;
        y = Math_GetRandom16() % (HEIGHT - 24) + 16;
        VDP_CommandLMMV(x, y, pixelWidth, 1, src->White, VDP_OP_IMP);
    }

what you have behind these VDP_CommandLMMV functions are writes that can't be optimized by a compiler. You have a structure that has to be updated, and which has different fields and after the update of the structure some additional function is called. So in this case, even if it's inlined, the compiler can't know that that additional function is not wiping out the content of the structure, and it has to update the variables that potentially don't have to ever be updated, if the library or library use followed somewhat different principles. Specifically, if that additional function doesn't wipe the structure, then writing to pixelWidth, 1 and VDP_OP_IMP could be done only before the loop as the content then never changes. So the potential hypothetical code could be just:

    g_VDP_Command.NX = pixelWidth
    g_VDP_Command.NY = 1
    g_VDP_Command.OP = ..
    for(u8 i = 0; i < 255; ++i)
    {
         g_VDP_Command.DX = x;
         g_VDP_Command.DY = y;
        some_simpler_call...
   }    

and then the overall generated code is both shorter and faster, and also more obvious what it actually does.
But the more complex code helped catching a bug, so it's also good that it exists, so "thumb up" for that.
Also, one more observation: as far as I see the only widths for the given modes used are 256 and 512, in that case, instead of using width to do %, the faster code could get the mask of the mode and instead of

x = Math_GetRandom16() % src->Width;

do just

x = Math_GetRandom16() & currentWidthMask;

(again, the compiler can't know if the called functions will change the content behind the src-> so it's you who have to move it out of the loop).

[Philipp Klaus Krause]

Fixed in [r15277].