#4070 sfr32 MSB instruction address is wrong

[RvD]

__sfr32 causes instruction destication register to be wrong for the MSB.

Assembly output of main.c compile sdcc -mmcs51 main.c.

             000000 E4               [12]  192     clr a                                                                       
  193        000001 F5 A7            [12]  193     mov ((_SFR_DATA_U32 >> 0) & 0xFF),a                                         
  194        000003 F5 A6            [12]  194     mov ((_SFR_DATA_U32 >> 8) & 0xFF),a                                         
  195        000005 F5 A5            [12]  195     mov ((_SFR_DATA_U32 >> 16) & 0xFF),a                                        
  196        000007 F5 FF            [12]  196     mov ((_SFR_DATA_U32 >> 24) & 0xFF),a                                        
  197                                      197 ;   ../../main.c:6: while(1);    

mov ((_SFR_DATA_U32 >> 24) & 0xFF),a generate F5 FF but is should generate F5 A4.

Seen on

• sdcc 4.5.0 on debian 13
• sdcc 4.6.0 from the website also shows this issue.
• sdcc 4.6.2 #16825 (Linux), lastest snapshot

[RvD]

I see that the main.c has changed A4 into A3 but still the same issue. It keeps generating F5 FF.

[RvD]

I also see it in reverse mov a, (SFR_DATA_U32), E5 FF. FF location should A4

4359:      029675 E5 A7            [12] 4359    mov     a,((_SFR_DATA_U32 >> 0) & 0xFF)
4361:      029678 E5 A6            [12] 4361    mov     a,((_SFR_DATA_U32 >> 8) & 0xFF)
4364:      02967C E5 A5            [12] 4364    mov     a,((_SFR_DATA_U32 >> 16) & 0xFF)
4367:      029680 E5 FF            [12] 4367    mov     a,((_SFR_DATA_U32 >> 24) & 0xFF)

and in the lst-file it get this notation

                                  106 ;--------------------------------------------------------                     
   107                                      107 ; special function registers                                                  
   108                                      108 ;--------------------------------------------------------                     
   109                                      109     .area RSEG    (ABS,DATA)                                                  
   110        000000                        110     .org 0x0000                                                               
   111                             0000A0   111 _SFR_EXEC_GO    =   0x00a0                                                    
   112                             0000A1   112 _SFR_EXEC_STATUS    =   0x00a1                                                
   113                             00A2A3   113 _SFR_REG_ADDR_U16   =   0xa2a3                                                
   114                             0000A2   114 _SFR_REG_ADDRH  =   0x00a2                                                    
   115                             0000A3   115 _SFR_REG_ADDRL  =   0x00a3                                                    
   116                             00A6A7   116 _SFR_DATA_U16   =   0xa6a7                                                    
   117                             FFA5A6A7   117 _SFR_DATA_U32 =   0xa4a5a6a7      

[Philipp Klaus Krause]

Looks like a problem in the assembler. For comparison, I tried with upstream asxxxx 6.10, but there the upper two bytes of the address of the sfr are 0x00, which looks even more wrong. I'm not that familiar with this stuff, but I guess there is some way to enable support for 32-bit relocations currently missing?

P.S.: I can also reproduce the bug as reported (0xff MSB) on my Debian GNU/Linux testing system using SDCC built from current trunk.

[Philipp Klaus Krause]

I'm a bit confused by this, though: AFAIK, we only have an 8-bit address space for __sfr? So shouldn't that be a 32-bit wide sfr at 0xa7 to 0xaa (with SDCC emitting a warning that 0xa3a5a6a7 is out of sfr range)? So this would actually be a compiler issue for even trying to emit those shifts to get the upper bytes of the address? I'm not an expert on mcs51, though.

[RvD]

I guess there is some way to enable support for 32-bit relocations currently missing?

Is this a question to me or other maintainers?
I don't have an answer for that.

I'm a bit confused by this, though: AFAIK, we only have an 8-bit address space for __sfr?

I think that is correct.

So shouldn't that be a 32-bit wide sfr at 0xa7 to 0xaa (with SDCC emitting a warning that 0xa3a5a6a7 is out of sfr range)? So this would actually be a compiler issue for even trying to emit those shifts to get the upper bytes of the address? I'm not an expert on mcs51, though.

Our hardware Realtek RTL8372, 10GBit network chip has internal DW8051-core and they interface thru SFRs.
Networkchip settings are written/read via 32-bit value thru sfr a4..a7.
So a other sfr address don't help us.
__sfr32 also generated better code, most noticeable then the values are zero. Then is used clr a; mov sfr_x a instead of mov sfr_x 0x00

I hope this answer your questions!

[Philipp Klaus Krause]

To anyone. While I am an SDCC dev, I'm not that familiar with mcs51, and how __sfr32 is supposed to work there. @maartenbrock is the mcs51 port maintainer.

P.S.: I'm a bit surprised by how __sfr16 and __sfr32 work for mcs51, but apparently this approach (the C code from this issue is correct), and SDCC allows to combine arbitrary locations into a 32-bit __sfr for mcs51. At least this justifies having a separate keyword vs. using types for wide __sfr. Clearly what happens in the assembler then doesn't make sense, so I wonder when this broke, and why it was not found earlier? After all, it must have been working when __sfr32 was introduced long ago?

[Maarten Brock]

Yes, __sfr16 and __sfr32 are special. Because sfr space cannot be accessed indirectly and many sfr-pairs are at non-adjacent locations historically and with mixed endianness on top, I introduced these with a special meaning of their __at address.

The LSB of the address specifies the address of the functional LSB, The higher bytes of the address specify the addresses of the higher functional bytes. Code generation often becomes much more efficient if it can handle the larger integers.

Consider the standard timers 0 and 1: Their sfr's are at:
0x8A TL0 timer 0 low byte
0x8B TL1 timer 1 low byte
0x8C TH0 timer 0 high byte
0x8D TH1 timer 1 high byte
You can define a 16 bit sfr to access each timer as a 16 bit integer.

__sfr16 __at(0x8C8A) TMR0;
__sfr16 __at(0x8D8B) TMR1;
void foo(void)
{
    TMR0 = TMR1 + 1;
}

The 8052 standard timer 2 does have adjacent addresses for TMR2L and TMR2H in little-endian order, just like DPTR is in adjacent DPL and DPH in little-endian order. But beware if you write self-modifying code, because LJMP/LCALL/MOV DPTR have a big-endian value. There are also mcs51 derivatives with 24 bit, 32 bit or even 40 bit sfr's. Or with big endian 16 bit sfr's.

The assembler and linker need to support 32 bit addresses for this to work and they did back then when I introduced this. But asxxxx later started to mask the higher bits and this is now merged in sdas. That it even performs sign extension (0xFF) is quite weird.

What we of course miss deerly is a good regression test that can check the generated address. The obvious 'just take the address of the sfr32 and compare it' is not allowed because sfr's cannot be accessed indirectly.

Other, commercial, compilers also have sfr16 but none support non-adjacency AFAIK.

[Philipp Klaus Krause]

I see. Quite different from the requirements of other ports more familiar to me.
Regarding tests: how about using some R/W SFR supported by uCsim to make a __sfr32 that aliases with some __sfr16 and __sfr? We could write the __sfr32, the read it as two __sfr16 and four __sfr, and check if the values match (and the other way round).

[Gabriele Gorla]

the fix is to change exprmasks(3); to exprmasks(4); in sdas/as8051/i51mch.c
Unfortunately this completely breaks relocation in the linker.

    00000000 78r00            [12]  133     mov r0,#_s_c_s
    00000002 74 61            [12]  134     mov a,#0x61
    00000004 F2               [24]  135     movx    @r0,a

is incorrectly relocated to (note the extra 00 and the byte stream is shifted by 1:

    000000D9 78 0B            [12]  133     mov r0,#_s_c_s
    000000DB 00 74            [12]  134     mov a,#0x61
    000000DD 61               [24]  135     movx    @r0,a

I am trying to track it down but it is not easy to follow through all the globals in asxxx.

[d00f]

I think I found why exprmasks(4) breaks the relocations, and a smaller fix.

The 0xFF comes from rngchk() in asexpr.c: it sign-extends every value to the width set by exprmasks(), and the 8051 port calls exprmasks(3), so 0xa4a5a6a7 becomes 0xFFA5A6A7 before >> 24 is applied. exprmasks(4) fixes that, but a_bytes also controls how many address bytes go into the T lines. The sdas 8051 single byte relocation in outrb() (asout.c, "sdas mcs51 specific") marks the entry as R_BYT3 and writes a_bytes bytes, so with a_bytes = 4 the linker takes three bytes and the fourth one stays in the stream. That is the extra 00 gorlik saw.

The two widths don't have to be the same. The attached patch splits exprmasks() into the address width and a new valmasks() for the arithmetic precision, and has the 8051 port use exprmasks(3) plus valmasks(4). The .rel format and the linker are untouched.

Tested on 4.6.0 with the reproducer and a slightly bigger one with relocatable code and xdata (the 4.5.0 sdas8051 with the same change gives the same bytes for the reproducer): all four __sfr32 bytes now get a4..a7 (and a7..a4 for the reversed list), and the .ihx differs from the unpatched build only in those address bytes, so the relocations are unchanged. A small assembler test with negative constants, 24-bit values, .if conditions and byte selections produces identical .rel files with and without the patch; only >> 24 of a value with bit 23 set changes, which is the point. I also built a real 8051 firmware (RTLPlayground for the RTL8373, about 130 KB of code in four banks) with __sfr32 in its register write macro: the two binaries differ in exactly the 72 operand bytes that were 0xFF, and the segment sizes are identical. Going from four byte writes to the __sfr32 saves 236 bytes of code there, and the firmware built this way runs on the switch: the registers it writes at boot read back the same as with the byte writes, and the MAC table, LACP and STP come up as before.

One thing the patch doesn't touch: the linker still masks symbol values to 24 bits, so the .map shows the __sfr32 as 00A5A6A7. That is cosmetic, since the compiler defines the sfr in every module and the assembler resolves it locally.

Patch against current trunk attached; the same change applies to 4.5.0 and 4.6.0 with different offsets.

Index: sdas/asxxsrc/asexpr.c
===================================================================
--- sdas/asxxsrc/asexpr.c
+++ sdas/asxxsrc/asexpr.c
@@ -1358,13 +1358,54 @@

  *     int n       T Line Bytes in Address
  *
  * The function exprmasks() configures the assembler
- * for 16, 24, or 32-Bit Data/Addresses.
+ * for 16, 24, or 32-Bit Data/Addresses.  The arithmetic
+ * precision follows the address width unless a port
+ * calls valmasks() afterwards.
  *
  * local variables:
  *     none
  *
  * global variables:
  *     int a_bytes     T Line Bytes in Address
+ *
+ * functions called:
+ *     void    valmasks()  asexpr.c
+ *
+ * side effects:
+ *     The address width and arithmetic precision
+ *     parameters are set.
+ */
+
+void
+exprmasks(int n)
+{
+   switch(n) {
+   default:
+       n = 2;
+   case 2:
+   case 3:
+   case 4:
+       break;
+   }
+   a_bytes = n;
+   valmasks(n);
+}
+
+/*)Function    void    valmasks(n)
+ *
+ *     int n       Bytes of arithmetic precision
+ *
+ * The function valmasks() configures the 16, 24, or
+ * 32-Bit arithmetic precision independently of the
+ * T Line address width.  A port whose addresses fit
+ * in 24 bits but whose symbols may hold 32-bit values
+ * (the 8051 __sfr32 address lists, for example) uses
+ * exprmasks(3) followed by valmasks(4).
+ *
+ * local variables:
+ *     none
+ *
+ * global variables:
  *     a_uint  a_mask      Address mask
  *     a_uint  s_mask      Sign mask
  *     a_uint  v_mask      Value mask
@@ -1377,14 +1418,11 @@
  */

 void
-exprmasks(int n)
+valmasks(int n)
 {

-   a_bytes = n;
-
 #ifdef LONGINT
-   switch(a_bytes) {
+   switch(n) {
    default:
-       a_bytes = 2;
    case 2:
        a_mask = (a_uint) 0x0000FFFFl;
        s_mask = (a_uint) 0x00008000l;
@@ -1404,9 +1442,8 @@
        break;
    }
 #else
-   switch(a_bytes) {
+   switch(n) {
    default:
-       a_bytes = 2;
    case 2:
        a_mask = (a_uint) 0x0000FFFF;
        s_mask = (a_uint) 0x00008000;
Index: sdas/asxxsrc/asxxxx.h
===================================================================
--- sdas/asxxsrc/asxxxx.h
+++ sdas/asxxsrc/asxxxx.h
@@ -1424,6 +1424,7 @@
 extern void        expr(struct expr *esp);
 extern void        exprx(struct expr *esp, int n);
 extern void        exprmasks(int n);
+extern void        valmasks(int n);
 extern void        exprscan(struct expr *esp, char *end, char *bgn);
 extern void        exprsym(struct expr *esp, char *str);
 extern int     is_abs(struct expr *esp);
Index: sdas/as8051/i51mch.c
===================================================================
--- sdas/as8051/i51mch.c
+++ sdas/as8051/i51mch.c
@@ -805,9 +805,12 @@
    hilo = 1;

    /*

-    * Address Space
+    * Address Space: 24-bit addresses in the T lines,
+    * 32-bit arithmetic so that __sfr32 address lists
+    * like 0xa4a5a6a7 keep their most significant byte.
     */
    exprmasks(3);
+   valmasks(4);

    /*

     * First time only: