• AVR Freaks

Hot!XC16 V1.36, dsPIC33C and bitfield shenanigans

Page: 12 > Showing page 1 of 2
Author
JPortici
Super Member
  • Total Posts : 768
  • Reward points : 0
  • Joined: 2012/11/17 06:27:45
  • Location: Grappaland
  • Status: offline
2019/04/11 04:42:46 (permalink)
0

XC16 V1.36, dsPIC33C and bitfield shenanigans

Greetings.
I've stumbled upon what i believe is a serious bug (or a serious case of me not knowing how to declare bitfields in C)

I've updated a design that uses CANBUS with the dsPIC33CK256MP502 and i'm having problems because the ID is not correct.
Upon investigation, the ID is not correct because the memory buffer for the ID IS NOT CORRECT!
In fact, if i write directly to the fifo area the correct ID, the correct ID is transferred.

To make things more portable between architectures my CAN library uses several layers and types... Unfortunately that's necessary to mantain sanity.
To the scope of this example i have two different structures that hold a CAN frame, one that i call "can_t" which has the frame in human-readable format, as it will appear on a bus decoder, defined as such


(More will come if the Forum allows me but basically, the compiler output is wrong. a 9+ bit field is accessed with byte instructions when optimization is greater than zero, so the wrong value is written back)
 
edit: at this pastebin. I don't want to waste more time in trying to get the post to pass https://pastebin.com/56y4V8Sw
post edited by JPortici - 2019/04/11 04:45:34
#1

24 Replies Related Threads

    NorthGuy
    Super Member
    • Total Posts : 5654
    • Reward points : 0
    • Joined: 2014/02/23 14:23:23
    • Location: Northern Canada
    • Status: online
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/11 05:51:11 (permalink)
    4 (1)
    Looks like a bug in the compiler. I would report it to Microchip.
    #2
    JPortici
    Super Member
    • Total Posts : 768
    • Reward points : 0
    • Joined: 2012/11/17 06:27:45
    • Location: Grappaland
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/11 05:56:29 (permalink)
    4.5 (2)
    I thought so, i'll finish to test the thing then i will prepare a sample project hoping to be able to reproduce the issue
    #3
    JPortici
    Super Member
    • Total Posts : 768
    • Reward points : 0
    • Joined: 2012/11/17 06:27:45
    • Location: Grappaland
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/11 08:41:44 (permalink)
    4.67 (3)
    for those who want to try, this is a self-contained example.
    After the first call the values should be
    SID:0x765
    EID_L:0x00
    EID_H:0x00
    Aftern the second call the values should be
    SID:0x48D
    EID_L:0x18
    EID_H:0x2B3
     
    I'm getting the wrong values with -O1
     
    device: I tested with dsPIC33CK256MP502/508
    compiler: V1.35 and V1.36
     
    #include <xc.h>
    #include <stdint.h>
    #include <stdbool.h>
    #include <string.h>

    #define CAN_FORMAT_STD 0
    #define CAN_FORMAT_EXT 1

    typedef volatile struct {
      uint32_t id;          //Message ID
      uint8_t format;       //STD or EXT
      uint8_t type;         //RTR or DATA
      uint8_t unused;       //Unused byte so the structure size is a power of two -> 16 bytes
      uint8_t data_lenght;  //Number of Data Bytes
      uint8_t data[8];      //Up to 8 Data Bytes
    } can_t;
    typedef union {
      volatile struct {
        struct {
          uint16_t SID_l  :11;
          uint16_t EID_l  :5;
        } T0;
        struct {
          uint16_t EID_h  :13;
          uint16_t SID_h  :1;
          uint16_t        :2;
        } T1;
        struct {
          uint16_t DLC    :4;
          uint16_t IDE    :1;
          uint16_t RTR    :1;
          uint16_t BRS    :1;
          uint16_t FDF    :1;
          uint16_t ESI    :1;
          uint16_t SEQ_l  :7;
        } T2;
        struct {
          uint16_t SEQ_h;
        } T3;
        struct {
          uint16_t data0  :8;
          uint16_t data1  :8;
        } T4;
        struct {
          uint16_t data2  :8;
          uint16_t data3  :8;
        } T5;
        struct {
          uint16_t data4  :8;
          uint16_t data5  :8;
        } T6;
        struct {
          uint16_t data6  :8;
          uint16_t data7  :8;
        } T7;
      };
      uint16_t word[8];
    } canFDtxFifoEntry_t;


    canFDtxFifoEntry_t tstFifo;

    void testFunction(can_t *can_frame);

    int main(void) {
      can_t tstFrame;

      tstFrame.id = 0x765;
      tstFrame.format = CAN_FORMAT_STD;

      //Test if compiler will correctly access the bitfield
      testFunction(&tstFrame);    //After the call the SID field should be 0x765

      tstFrame.id = 0x12345678;
      tstFrame.format = CAN_FORMAT_EXT;

      //Test if compiler will correctly access the bitfield
      testFunction(&tstFrame);

      while(1) {
        Nop();
      }
      return 0;
    }

    void testFunction(can_t *canFrame) {
      canFDtxFifoEntry_t txMsg;

      if (canFrame->format == CAN_FORMAT_EXT) {
        txMsg.T2.IDE = 1;
        txMsg.T0.SID_l = (uint32_t) canFrame->id >> 18;
        txMsg.T0.EID_l = (uint32_t) canFrame->id;
        txMsg.T1.EID_h = (uint32_t) canFrame->id >> 5;
        txMsg.T1.SID_h = 0;
      }
      else {
        txMsg.T2.IDE = 0;
        txMsg.T0.SID_l = (uint32_t) canFrame->id & 0x7FF;
        txMsg.T0.EID_l = 0;
        txMsg.T1.EID_h = 0;
        txMsg.T1.SID_h = 0;
      }

      Nop();
      Nop();

      memcpy(&tstFifo,(uint16_t *)&txMsg,sizeof(canFDtxFifoEntry_t));

      Nop();
      Nop();
    }

    #4
    JPortici
    Super Member
    • Total Posts : 768
    • Reward points : 0
    • Joined: 2012/11/17 06:27:45
    • Location: Grappaland
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/17 00:26:37 (permalink)
    5 (1)
    Confirmed as compiler bug.
     
    Current workaround is to add __attribute__((optimize("O0"))) to any function that manipulate bitfields.
    I'm not sure if this applies also to hardware registers (because i haven't seen any strange behaviour yet), but i asked
     
    This should affect only dsPIC33CH/CK as the code generated to access bitfields is different, since it makes use of the new bitfield type instructions
    #5
    nice
    Super Member
    • Total Posts : 1089
    • Reward points : 0
    • Joined: 2004/09/18 11:42:25
    • Location: Germany
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/17 15:50:52 (permalink)
    0
    Jack_M
    I'm not sure if this applies also to hardware registers (because i haven't seen any strange behaviour yet), but i asked



    I don’t think that this bug does depend on the target (SFR vs. RAM). I’ve tried to reproduce the bug you’ve reported in more "simple" scenarios (writing to bitfields in RAM) and failed miserably. In general the compiler’s  generated code for writing 10 bits of a bitfield is correct. I also failed to identify the root cause triggering this compiler bug.

    I’d suspect that writing to SFRs works as intended as the code used for it is usually more simple than in your example, but that’s just a guess.


    #6
    JPortici
    Super Member
    • Total Posts : 768
    • Reward points : 0
    • Joined: 2012/11/17 06:27:45
    • Location: Grappaland
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/18 05:04:02 (permalink)
    0
    more simple as in?
     
    Please notice that i had to define the type canFDtxFifoEntry_t (and others) because the CAN FD controller in the dsPIC33C use SRAM to load/store the FIFOs, it's not stored in SFR*. If it was in SFR space there would have been a bitfield defined in the same way from microchip.
     
    Generally i don't define bitfields, but i constantly use bitfields because i read/write registers.. and i don't think i've seen a simillar behaviour so i agree that there must be something really specific that triggers the bug.
     
     
     
    *Side question: Does anybody know how the controller communicates with memory? I don't see any mention of dedicated dma channels or if it stalls the cpu or the dma during read/writes.
    #7
    NorthGuy
    Super Member
    • Total Posts : 5654
    • Reward points : 0
    • Joined: 2014/02/23 14:23:23
    • Location: Northern Canada
    • Status: online
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/18 13:20:03 (permalink)
    4 (1)
    Jack_M
    Generally i don't define bitfields, but i constantly use bitfields because i read/write registers.. and i don't think i've seen a simillar behaviour so i agree that there must be something really specific that triggers the bug.

     
    Most SFRs don't have bitfields which cross byte boundaries.
     
    #8
    nice
    Super Member
    • Total Posts : 1089
    • Reward points : 0
    • Joined: 2004/09/18 11:42:25
    • Location: Germany
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/24 12:29:02 (permalink)
    0
    True, but crossing byte boundaries in general is not triggering this bug. E.g.

    typedef struct
    {
        uint16_t :3;
        uint16_t data:10;
        uint16_t :3;
    } foo_t;

    volatile foo_t foo;
    volatile uint16_t data;

    foo.data = data;


    compiles to

    80:                    foo.data = data;
    0010C0  203FF0     MOV #0x3FF, W0
    0010C2  B6101E     AND data, WREG
    0010C4  0A20C3     BFINS #0x3, #0xa, W0, foo
    0010C6  00101C     NOP


    at optimization level 1. Of course the AND operation on 'data'  is superfluous. At optimization level s it compiles to

    80:                    foo.data = data;
    00106E  8080F0     MOV data, W0
    001070  0A20C3     BFINS #0x3, #0xa, W0, foo
    001072  00101C     NOP

    #9
    nice
    Super Member
    • Total Posts : 1089
    • Reward points : 0
    • Joined: 2004/09/18 11:42:25
    • Location: Germany
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/24 14:54:30 (permalink)
    4 (1)
    Passing a pointer to the bitfield to a function doesn’t seem to be the real culprit as well.

    typedef struct
    {
        uint16_t :3;
        uint16_t data:10;
        uint16_t :3;
    } foo_t;

    foo_t foo;

    void bar (foo_t *foo, uint16_t set_value)
    {
        foo->data = set_value;
    }

    //…
    bar (&foo, 0x123);



    84:                    bar (&foo, 0x123);
    001074  201231     MOV #0x123, W1
    001076  2101E0     MOV #0x101E, W0
    001078  07FFF9     RCALL bar

    69:                void bar (foo_t *foo, uint16_t set_value)
    70:                {
    71:                    foo->data = set_value;
    00106C  0A01C3     BFINS #0x3, #0xa, W1, [W0]
    00106E  000010     NOP
    72:                }
    001070  060000     RETURN


    The only difference between my two examples and the OP’s example is, that the compiler’s generated code uses file or indirect working register writes as the destination for the BFINS instruction, whereas the OP’s example uses direct working register writes. This might give a hint what actually triggers the bug, but this is just pure speculation.

    @JPortici
    Hopefully this answers your question "more simple as in?".
    post edited by nice - 2019/04/24 15:02:50
    #10
    mlp
    boots too small
    • Total Posts : 798
    • Reward points : 0
    • Joined: 2012/09/10 15:12:07
    • Location: previously Microchip XC8 team
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/25 06:54:20 (permalink)
    0
    nice
    typedef struct
    {
        uint16_t :3;
        uint16_t data:10;
        uint16_t :3;
    } foo_t;


    Just a note for completeness here: although it probably makes no difference in the specific case of XC16 and uint16_t, according to the language standard the base type for a bitfield should be either int or unsigned int only. Using anything else is likely to cause you pain at some point.

    Mark (this opinion available for hire)
    #11
    nice
    Super Member
    • Total Posts : 1089
    • Reward points : 0
    • Joined: 2004/09/18 11:42:25
    • Location: Germany
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/25 11:56:59 (permalink)
    0
    Thanks for your note, Mark. Of course you are right regarding the standard.

    Fun fact: Since v1.33 XC16 uses uint16_t as the base type for SFR bitfields.
    typedef struct tagLATBBITS {
        uint16_t LATB0:1;
        //…
    } LATBBITS;
    extern volatile LATBBITS LATBbits __attribute__((__sfr__));


    XC32 v2.15 uses uint32_t:
    typedef union {
        struct {
            uint32_t LATB0:1;
            //…
        };
        struct {
            uint32_t w:32;
        };
    } __LATBbits_t;
    extern volatile __LATBbits_t LATBbits __asm__ ("LATB") __attribute__((section("sfrs"), address(0xBF860130)));


    XC8 seems to be Microchip’s only compiler strictly obeying the language standard’s rule.
    typedef union {
        struct {
            unsigned LATB0                  :1;
            // …
         };
        struct {
            unsigned LB0                    :1;
            // …
         };
    } LATBbits_t;
    extern volatile LATBbits_t LATBbits __at(0xF8A);

    #12
    jtemples
    عُضْوٌ جَدِيد
    • Total Posts : 11332
    • Reward points : 0
    • Joined: 2004/02/13 12:31:19
    • Location: Southern California
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/04/25 14:48:29 (permalink)
    0
    according to the language standard the base type for a bitfield should be either int or unsigned int only

     
    C99, 6.7.2.1p4 says it can be essentially anything:  "A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type."  "int" isn't included, but oddly it's mentioned as a valid bit-field type in 6.3.1.1p2.  C90 specifies only int, unsigned int, or signed int.
     
    Footnote 104 tells me it would be a bad idea to ever use "int":  "As specified in 6.7.2 above, if the actual type specifier used is int or a typedef-name defined as int, then it is implementation-defined whether the bit-field is signed or unsigned."  C90 says that int "may differ from" signed int, but doesn't specify how it may differ.
    #13
    JPortici
    Super Member
    • Total Posts : 768
    • Reward points : 0
    • Joined: 2012/11/17 06:27:45
    • Location: Grappaland
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/08/18 23:06:41 (permalink)
    0
    Just wanted to post an update..
    it was *probably* solved in V1.40
     
    XC16-1180   Fix for dsPIC33C devices, the compiler generates an incorrect move(byte mode move) to load data for bitfield insert operation(bfins) of size 8bits to 15bits.[(quote]
    #14
    marcov
    Super Member
    • Total Posts : 253
    • Reward points : 0
    • Joined: 2006/10/08 01:59:40
    • Location: Eindhoven, NL.
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/08/19 14:41:08 (permalink)
    0
    From V1.40 release notes. Old compiler  Flash access is deprecated, use MCC module which only supports a limited number of devices (most notably not MU).

    Sigh.
    #15
    picy2620
    Senior Member
    • Total Posts : 110
    • Reward points : 0
    • Joined: 2009/11/13 08:12:47
    • Location: Germany
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/09/17 23:23:15 (permalink)
    0
    XC16 V1.41 has again some bitfield fixes:
     
    From the release notes:
     
    Bugfix release; current users of v1.40 should update to v1.41. Please see bug fixes.
     
    v1.41
    • XC16-1202   Fix -merrata=ecc does not prevent all errata conditions.
    • XC16-1203   Fix invalid bitfield operation at higher optimizations in DSPIC33C devices.
    • XC16-1204   Fix modified data from bitfield operation not getting stored for indirect access in DSPIC33C devices.
     
    picy2620
    #16
    mpgmike
    Super Member
    • Total Posts : 278
    • Reward points : 0
    • Joined: 2014/01/23 17:27:06
    • Location: NJ
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/09/18 04:43:01 (permalink)
    0
    The Microchip web site is still showing v1.40 as the latest XC16.

    I don't need the world to know my name, but I want to live a life so all my great-grandchildren proudly remember me.
    #17
    MBedder
    Circuit breaker
    • Total Posts : 6794
    • Reward points : 0
    • Joined: 2008/05/30 11:24:01
    • Location: Zelenograd, Russia
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/09/18 05:14:56 (permalink)
    0
    ..but the link supplied downloads the 1.41.
    #18
    picy2620
    Senior Member
    • Total Posts : 110
    • Reward points : 0
    • Joined: 2009/11/13 08:12:47
    • Location: Germany
    • Status: offline
    Re: XC16 V1.36, dsPIC33C and bitfield shenanigans 2019/09/18 09:01:03 (permalink)
    0
    Correct. It´s always the same behavior. It takes a few days to update the links.
    The long-time residents here know that wink: wink
     
    picy2620
    #19
    du00000001
    Just Some Member
    • Total Posts : 3053
    • Reward points : 0
    • Joined: 2016/05/03 13:52:42
    • Location: Germany
    • Status: offline
    Re: XC16 V1.41, dsPIC33C and bitfield shenanigans 2019/09/18 09:15:37 (permalink)
    0
    The update of the links is manual !
     
    The great thing to know:
    you can address any version directly by modifying one of the archive links available:
    http://ww1.microchip.com/...-windows-installer.exe
    ==>
    http://ww1.microchip.com/...-windows-installer.exe
     
    post edited by du00000001 - 2019/09/18 09:32:12

    PEBKAC / EBKAC / POBCAK / PICNIC (eventually see en.wikipedia.org)
    #20
    Page: 12 > Showing page 1 of 2
    Jump to:
    © 2019 APG vNext Commercial Version 4.5