#3160 SDCC on z80 blindly emits `LD ()` on any pointer access, violating `__sfr` I/O space.

[Kyra Zimmer]

after some investigation into a problem described in a discussion I'm convinced this is an actual bug:

when encountering any pointer access, genPointerGet / genPointerSet create LD x,(y) / LD (x),y opcodes. without actually ever checking whether the pointer is pointing to a memory location, or I/O space (as in __sfr*).

Sample code:

void main(){
  __sfr* io_ptr;
  io_ptr=(__sfr*)0x12; // IO address
  *io_ptr=0x34; // write a byte
}

Compile with: sdcc -mz80 --no-std-crt0 -c test.c

Generated assembler:

    .area _CODE
;test.c:1: void main(){
;   ---------------------------------
; Function main
; ---------------------------------
_main::
;test.c:4: *io_ptr=0x34; // write a byte
    ld  hl, #0x0012
    ld  (hl), #0x34
;test.c:5: }
    ret

As you can see it writes to memory location 0x0012 using a LD (HL) instruction, while it should instead do something like this:

  ld c, #0x12
  ld a, #0x34
  out (c), a

The same applies to the uint8_t readvalue = *io_ptr; / IN a, (c) direction of course.

I'm on version 4.0.0 #11528 which isn't the bleeding edge of course, but I did search the bug tracker and changelog for the upcoming RC for mentions of this, and didn't seem to find anything about this being fixed.

[Philipp Klaus Krause]

Pointer-to-sfr is apparently broken for many targets in SDCC.

Fixing it requires a fine-grained approach:

• On some targets (stm8, gbz80), the sfr is in normal address space, so we get normal pointers.
• On some targets, the sfr is in a separate address space and we need special pointers. There, pointers to sfr should require to be declared as such, and there should be a compiler error when casting away the sfr; reads and writes via such pointers needs to be handled in a special way.
• On some targets, there is no efficient way to support pointers to sfr. These targets should just disallow taking the address of an sfr or declaring a pointer to sfr.

[Kyra Zimmer]

this per-target distinction already happens though, it should just be a matter of adding the correct cases (or error messages in the 3rd case) in for the pointer access function.

[Philipp Klaus Krause]

Unfortunately, current SDCC looses the information that a pointer points to __sfr early in the frontend. Making sure that the informatiomn is passed correctly throughout SDCC is probably at least as much work, as adding hte necessary code in codegen for z80 pointer accesses.

[Sebastian Riedel]

Even though we have memory mapped in normal address space on gameboy, I don't think gbz80 needs to be treated much different.
We have ldh (c), a/ldh (0xFF12), a to access 0xFF00-0xFFFF(I/O and HiRAM) after all.
The only big difference is, that it would be legal to cast it to a normal pointer.

[Philipp Klaus Krause]

Also, for gbz80, ldh could be used for normal pointer write where the pointer is known to fall into the 0xff00-0xfff range.

[Sebastian Riedel]

That would be indeed desirable, but I doubt the compiler knows enough to properly do that in all cases.
GBDK-2020 for example has all register addresses in an .s file, so it should be known only at link time.
So it should probably do both: handle sfr as ldh and use ldh where addresses are known.

[Kyra Zimmer]

sfr does nothing on gbz80, it should just be silently ignored, and handle addresses with LDH as the optimizer sees fit because that's all ldh is for. on the z80 IN/OUT is required for the "sfr" (btw horrible term for IO) address space, on the gbz80 that address space simply doesn't exist. LD/LDH distinctions are purely an optimization thing, unrelated to this topic.

like Philipp said, you can use LDH for pointers you definitely know (e.g. consts) are gonna be high addresses but that's it. and of course the compiler doesn't know to properly do it in all cases, and it shouldn't, that's what pointers are for, and in those cases it'd just use LD. you might abuse the sfr again(none of those uses for IO are inherently special, functional, or registers after all) for an at(any high address) hint on gbz80 but that's got nothing to do with fixing this bug.

[Philipp Klaus Krause]

__sfr has its name for historic reasons: SDCC was initally created as a compiler for MCS-51, where the registers in I/O space are called "special function registers", commonly abbreviated sfr.
I agree that we should probably drop support for __sfr from gbz80. Possibly marking it as obsolescent in 4.2.0 and emitting a warning before fully removing it in 4.3.0.

[Kyra Zimmer]

oh yeah i know it makes sense on other platforms, i'm just saying if it's abused as IO on z80 it might as well be abused as an "this will always be highmem" hint on gbz80, but that's an optimizer thing, unrelated to the fact a c compiler that doesn't support half the address space of one of its targets is a pretty big deal

[Philipp Klaus Krause]

Having a quick look by by adding some debug output in genPointerSet in src/z80/gen.c, it looks as if the information that the pointer points to an __sfr is already lost early.
In fact even the earliest iCode from dumpraw0 (obtained by --dump-i-code) already looks like io_ptr is an ordinary unsigned char *.
Maybe we need another special pointer type in the DECLARATOR_TYPE enum to handle pointers to __sfr?

[Kyra Zimmer]

there's {symbol.a,asm}op.type == AOP_SFR to specify an operand to be a (non-pointer) __sfr address, i'm not sure if those survive inside the pointer's metainfo until (whatever calls) genPointer{S,G}et though?

[Maarten Brock]

Keep in mind that the MCS-51 and almost all derivatives do not support indirect access to special function registers. But the mcs51 port does allow turning an __sfr pointer into a generic pointer and places it in __idata where it belongs if some derivative does support indirect sfr access (as a few do).

I agree that on a platform without sfr/io space the keyword should not exist. And if it would please anyone, we could also make __io a synonym for __sfr.

[Kyra Zimmer]

not sure how the mcs51 port is supposed to be related to z80 IN/OUT instructions tho?

[Philipp Klaus Krause]

It is relevant, as we have a pointer to __sfr inthe C code. What should happen to that now depends on the port: maintain the information that it points to __sfr would ideal for z80. But apparently, for sm83 and mcs51 we instead want to transform it to other pointer types.

[Kyra Zimmer]

so 2 releases fixing lots of new bugs and renaming a few unimportant things and generally cleaning up later and current trunk still can't use IO pointers :/

is nobody using this compiler for actual Z80 projects that aren't GB?

[Benedikt Freisen]

IO pointers are not a standard relevant feature. Standard compliant code will therefore not use them and most people who use SDCC for Z80 presumably do so via hardware abstractions of a downstream project like Z88DK, which is built around SDCC's limitations.

On a side note: Passing structs as parameters, a feature that is standard relevant, was only recently implemented thanks to public funding, 19 years after the feature request.

[Kyra Zimmer]

i would very much disagree there. pointers behaving like pointers is very much "standard relevant", and SDCC defines what _sfr is supposed to mean. especially given the fact SDCC just silently generates the wrong code instead of e.g. failing to compile.

can you elaborate as to why you'd think "the standard" (also, which one? :P) is perfectly fine with all pointer accesses ignoring a compiler-defined keyword that just silently does the opposite of its documented behavior?

and yes, Z88DK does work around the limitation. by defining a function for each of the two instructions. yeah i hope you'd agree that's extreme overkill, and with the only other option being "write all your IO stuff in asm by hand" somehow i fail to see why SDCC's inconformity with both "the standard" and its own documented behavior is somehow not a problem.

---

just to make perfectly sure we're talking the right standard here: according to the SDCC documentation, _sfr is an "intrinsic named address space" as defined in "section 5.1 of the Embedded C standard", which (at least in ISO/IEC TR 18037:2008) says:

In addition to the generic address space, an implementation may support other, named address spaces. Objects may be allocated in these alternate address spaces, and pointers may be defined that point to objects in these address spaces. It is intended that pointers into an address space only need be large enough to support the range of addresses in that address space.

[Benedikt Freisen]

What I was trying to say is that the most realistic "bug fix" right now is to simply say that it doesn't work, i.e. that pointers on Z80 imply the generic address space.
If that less-than-ideal path is not satisfactory, well, patches are welcome.

[Kyra Zimmer]

so it's a standard relevant feature and SDCC simply implements neither C nor the referenced "embedded C" despite claims to the contrary and the documentation lies and your proposed "solution" is to keep it like that and fix the documentation to lie slightly less; because implementing the standards one claims (and needs to, to correctly support the hardware one claims) is "not relevant"?

yeah no, in that case a more realistic bug fix would be writing an actual C compiler.

[Benedikt Freisen]

and your proposed "solution" is to keep it like that and fix the documentation to lie slightly less

The proposed "solution" is to let people like you know what the compiler can actually do so that people like you waste less time trying things it can't do, until somebody has time to implement it. I'm sorry for suggesting that.

yeah no, in that case a more realistic bug fix would be writing an actual C compiler.

Nobody's holding you back. Have at it!
The golden rule when dealing with chronically understaffed volunteer-run open source projects is: "When in doubt, do it tf yourself!"
It's frankly precisely how I ended up here, replying to posts written with a strange sense of entitlement, on a Sunday afternoon.
Have a nice day!

[Benedikt Freisen]

[r13786] amends section 3.1.6 and 3.5 in the manual. They no longer claim that SDCC fully supports such address spaces.

[Kyra Zimmer]

thanks for the confirmation of the wontfix, and the revelation about developer attitude when people merely ask why a bug could go unnoticed for that long, will likely just fork sccz80 or something.

just before i go, you'll note the "strange sense of entitlement" only started after you berated me for having the audacity of merely asking why this bothers nobody else for so long, condescendingly lying about C, embedded C and SDCCman.

have a day.

[Philipp Klaus Krause]

The ticket has been left open, not tagged "wontfix". Having support for pointers to __sfr would be good. But there are more urgent things to do.
Pointers to __sfr are something very few users need or want (as we see from support for this being requested rarely vs. other far more often requested features).
On the other hand, the silent generation of code that does not do what users would expect is unfortunate. It looks like the 4.3.0 release will be delayed a bit anyway. If I find the time, I'll look into the possibility of emitting an error message to prevent this.

[Benedikt Freisen]

Consider my attitude revealed. In the light of the original poster's ostensible departure and for full transparency, let us preserve this full interaction for posterity and let us let posterity decide, whether negating the standard relevance of something not required by ANSI or ISO C in an ANSI/ISO C compiler constitutes a "condescending lie", whether giving an admittedly drastic example of how long it can take to get things done constitutes "berating", whether an inaccuracy in the manual corrected upon notice fulfills the lexical definition of "lying", whether a compiler with limited support for optional extensions can or cannot be "an actual C compiler", how exactly "until somebody has time to implement it" translates to "confirmation of the wontfix", how exactly an apparent unwillingness to write patches to speed things up relates to a willingness to single-handedly fork a less modern competing compiler, and whose choice of words ultimately was or was not appropriate within the respective context.

Additionally, I would like to hereby put my prediction on record, that this interaction of a kind I had never seen on here before will neither positively nor negatively affect, when somebody will be able to take the time to address the original subject of this issue report.

[Philipp Klaus Krause]

Clearly, most users that work on Z80 projects don't use pointers to __sfr. Actually, AFAIK, the z80 backend is one of the most-used ones (along with mcs51 and stm8).
However, AFAIK, the typical user will just use __sfr at fixed locations. Often not even direct but through some abstraction (be it third party or rolled by themselves).
For platforms, where there is a whole range of I/O adresses that make sense to be treated as an array, I would expect users to be much more interested in having pointers to __sfr. But that isn't typical for Z80-based systems.