Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#1929 Masking bits generates non-atomic code

open
nobody
MCS51
5
2015-03-25
2012-02-09
Anonymous
No

An expression to clear a bit (or bits) in a byte in the DATA area (or an SFR) should be atomic.

// sdcc -c testcase.c

#define MASK 0x0f
unsigned char reg;

void testcase(void)
{
reg &= ~MASK;
}

should generate:

anl _reg,#0xFe

but actually generates:

mov r7,_reg
anl ar7,#0xF0
mov _reg,r7

Using SDCC : mcs51 3.1.0 #7066 (Nov 28 2011) (Mac OS X x86_64)

Suggestion: Could this be fixed by a peeophole rule?

Discussion


  • Anonymous
    2012-02-09

    Test case

     
    Attachments

  • Anonymous
    2012-02-09

    On checking the instruction set reference (http://www.win.tue.nl/~aeb/comp/8051/set8051.html#51anl), I may have this wrong as anl only suppords an indirect address
    as the first operand. However, I would then expect SDCC to generate an atomic operation if 'reg' is declared as IDATA, viz:

    unsigned char __idata reg;

    Instead, it generates the following:

    mov r0,#_reg
    mov ar7,@r0
    anl ar7,#0xF0
    mov r0,#_reg
    mov @r0,ar7

    Also, are SFR's implitly considered DATA or IDATA?

    Apologies if I'm being daft here :)

     
  • Thomas Sailer
    Thomas Sailer
    2012-02-09

    If you change reg &= ~MASK to reg &= (unsigned char)~MASK, then it will generate what you want, i.e. anl _reg,#0xF0

    And yes, that cast shouldn't be necessary...

     
  • Gabriele Gorla
    Gabriele Gorla
    2012-02-14

    sdcc 3.0 compiles it as anl _reg,#0xF0 without the cast.

     

  • Anonymous
    2012-02-14

    Note also that by using an inverse macro, avoiding the need for the ~ operator, the correct code is generated with 3.1.0:

    #define IMASK 0xf0
    unsigned char reg;

    void testcase(void)
    {
    reg &= IMASK;
    }

     
  • Patryk
    Patryk
    2012-06-14

    >> Also, are SFR's implitly considered DATA or IDATA?
    SFR's can be accessed only directly, i.e. as DATA.

     
  • Ondrej Petr
    Ondrej Petr
    2014-02-22

    Issue is more general and survive in sdcc 3.3 - it seems to appear any time:
    lvalue is __data register,
    assignment operator is |=, &= or ^=,
    rvalue has type other than unsigned char.

    Explicit cast of rvalue to (unsigned char) seems to force compiler to generate proper code with atomic access to lvalue.

    If the bug will not be solved in next release, it should be noticed in sdcc manual as users tend to expect proper atomic behaviour that is absolutely required when lvalue is 8051 port with mixed input/output!

    Example:
    #include "compiler_defs.h"
    #include "C8051F120_defs.h"
    __bit bit_variable;
    void main(void)
    {
    P0 |= bit_variable; // non atomic sequence generated
    P0 |= (unsigned char) bit_variable; // atomic sequence generated
    P0 &= ~0x01; // non atomic sequence generated for 8051 port access!!! - it's a pity, as
    P0 &= (unsigned char) ~0x01; // atomic sequence generated when cast to unsigned char
    P0 &= (int) 0x01; // non atomic sequence generated when cast to int
    P0 &= 0x01; // atomic sequence generated w/o cast to unsigned char
    P0 |= (char) ~0x01; // non atomic sequence generated when cast to unsigned char
    P0 |= (char) 0x01; // non atomic sequence generated when cast to char
    P0 ^= (char) 0x01; // non atomic sequence generated when cast to char
    }

    Command line:
    + C:\PROGRA~1\SDCC\bin\sdcpp.exe -nostdinc -Wall -I"c:\SiLabs\MCU\Inc" -I"D:\Dokumenty\microsource\BlahoX\pcb_HVPSU_interface\firmware\nonatomic" -obj-ext=.rel -D__SDCC_MODEL_SMALL -DSDCC_MODEL_SMALL -D__SDCC_FLOAT_REENT -DSDCC_FLOAT_REENT -D__SDCC=3_3_0 -DSDCC=330 -D__SDCC_REVISION=8604 -DSDCC_REVISION=8604 -D__SDCC_mcs51 -DSDCC_mcs51 -D__mcs51 -D__STDC_NO_COMPLEX__ -D__STDC_NO_THREADS__ -D__STDC_NO_ATOMICS__ -D__STDC_NO_VLA__ -isystem "C:\Program Files\SDCC\bin..\include\mcs51" -isystem "C:\Program Files\SDCC\bin..\include" "D:\Dokumenty\microsource\BlahoX\pcb_HVPSU_interface\firmware\nonatomic\nonatomic.c"

     
    Last edit: Ondrej Petr 2014-02-22
  • Ben Shi
    Ben Shi
    2015-03-25

    • Category: --> MCS51