[Bug c/43876] New: [avr] Improper updating of struct members when written out of order from struct definition

[Bug c/43876] New: [avr] Improper updating of struct members when written out of order from struct definition

[justin at mattair dot net]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4319 bytes --]

avr-gcc may improperly update members of a struct if:

1) The access to the members is in a different order than the struct was
defined and
2) The volatile modifier is used on the struct variable (instance) and
3) The target device is set to atxmega128a1 (possibly all xmega chips) and
4) Optimizations are enabled (I tested -Os)

The following code replicates the problem:

#include <avr/io.h>

typedef struct {
        uint16_t        year;
        uint8_t         month;
        uint8_t         mday;
        uint8_t         hour;
        uint8_t         min;
        uint8_t         sec;
} RTC_time_t;

volatile RTC_time_t t;

void rtc_init (void)
{
        t.sec = 0;
        t.min = 0;
        t.hour = 0;
        t.mday = 1;
        t.month = 1;
        t.year = 2010;
        //t.month = 1;
        //t.mday = 1;
        //t.hour = 0;
        //t.min = 0;
        //t.sec = 0;
}

void main(void)
{
        rtc_init();

        while (1);
}

The makefile used was the WinAVR Makefile Template written by Eric B.
Weddington, Jörg Wunsch, et al. with MCU = atxmega128a1. The problem does not
exist when using, for example, MCU = atmega32u2, or if the volatile modifier is
removed.

The problem can be seen in the disassembly of rtc_init(). The disassembly
without the problem:

      t.year = 2010;
    7a1c:   8a ed          ldi   r24, 0xDA   ; 218
    7a1e:   97 e0          ldi   r25, 0x07   ; 7
    7a20:   80 93 1b 2b    sts   0x2B1B, r24
    7a24:   90 93 1c 2b    sts   0x2B1C, r25
      t.month = 1;
    7a28:   81 e0          ldi   r24, 0x01   ; 1
    7a2a:   80 93 1d 2b    sts   0x2B1D, r24
      t.mday = 1;
    7a2e:   80 93 1e 2b    sts   0x2B1E, r24
      t.hour = 0;
    7a32:   10 92 1f 2b    sts   0x2B1F, r1
      t.min = 0;
    7a36:   10 92 20 2b    sts   0x2B20, r1
      t.sec = 0;
    7a3a:   10 92 21 2b    sts   0x2B21, r1 

which is pretty straightforward.

The disassembly of the nonworking version:

                t.sec = 0;
    7a1c:   10 92 21 2b    sts   0x2B21, r1
      t.min = 0;
    7a20:   e0 e2          ldi   r30, 0x20   ; 32
    7a22:   fb e2          ldi   r31, 0x2B   ; 43
    7a24:   10 82          st   Z, r1
      t.hour = 0;
    7a26:   12 92          st   -Z, r1
      t.mday = 1;
    7a28:   81 e0          ldi   r24, 0x01   ; 1
    7a2a:   82 93          st   -Z, r24
      t.month = 1;
    7a2c:   82 93          st   -Z, r24
      t.year = 2010;
    7a2e:   8a ed          ldi   r24, 0xDA   ; 218
    7a30:   97 e0          ldi   r25, 0x07   ; 7
    7a32:   31 97          sbiw   r30, 0x01   ; 1
    7a34:   81 93          st   Z+, r24
    7a36:   90 83          st   Z, r25
    7a38:   32 97          sbiw   r30, 0x02   ; 2 

As you can see, the high byte of t.year is being updated with the low byte of
2010. Then t.month is overwritten with the high byte of 2010. The compiler
should have subtracted 2 from Z (the first sbiw instruction), but it only
subtracts 1. It does manage to subtract 2 in the last instruction, which seems
rather pointless.

I have tested this using both avr-gcc 4.3.3 (built on Linux using the
"build-avr-gcc-4.3.3-binutils-2.20-libc-1.6.8-insight6.8-dude-5.10-insight-patch"
script available on avrfreaks.net), avr-gcc 4.3.4, as well as WinAVR20100110 on
Windows. The problem exists in all of these cases.


Using built-in specs.
Target: avr
Configured with: ../../source/gcc-4.3.4/configure -v --target=avr --disable-nls
--prefix=/usr/local/avr --with-gnu-ld --with-gnu-as --enable-languages=c,c++
--disable-libssp --with-dwarf2
Thread model: single
gcc version 4.3.4 (GCC)

Command Line: avr-gcc -c -mmcu=atxmega128a1 -I. -gdwarf-2 -DF_CPU=32000000UL 
-Os -funsigned-char -funsigned-bitfields -fpack-struct -fshort-enums -Wall
-Wstrict-prototypes -Wundef -Wall -Wstrict-prototypes -Wa,-adhlns=test.lst 
-std=gnu99 -MD -MP -MF .dep/test.o.d test.c -o test.o


-- 
           Summary: [avr] Improper updating of struct members when written
                    out of order from struct definition
           Product: gcc
           Version: 4.3.4
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: justin at mattair dot net
GCC target triplet: avr-*-*


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43876

[Bug target/43876] [avr] Improper updating of struct members when written out of order from struct definition

[justin at mattair dot net]

------- Comment #1 from justin at mattair dot net  2010-04-27 19:52 -------
I have upgraded this to critical because it causes data corruption and because
I am not certain how to classify this. Lower if needed.


-- 

justin at mattair dot net changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |justin at mattair dot net
           Severity|normal                      |critical


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43876

[Bug target/43876] [avr] Improper updating of struct members when written out of order from struct definition

[justin at mattair dot net]

------- Comment #2 from justin at mattair dot net  2010-04-29 02:38 -------
Created an attachment (id=20511)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=20511&action=view)
patch for patch-xmega

It looks like possibly a copy-paste error was made in the patch-xmega file. The
following starting on line 573 of patch-xmega (from the
build-avr-gcc-4.3.4-binutils-2.20-libc-1.6.8-insight6.8-dude-5.10-insight-patch
script available on AVRFreaks.net):

+        return *l = 4, (AS2 (sbiw,%r0,1)    CR_TAB 
+                        AS2 (st,%p0+,%A1)   CR_TAB
+                        AS2 (st,%p0,%B1)    CR_TAB
+                        AS2 (sbiw,%r0,2));

I think should be:

+        return *l = 4, (AS2 (sbiw,%r0,2)    CR_TAB 
+                        AS2 (st,%p0+,%A1)   CR_TAB
+                        AS2 (st,%p0,%B1)    CR_TAB
+                        AS2 (sbiw,%r0,1));

This may have been caused by copying code from one of the post-increment
sections utilizing adiw, then converting it to pre-decrement utilizing sbiw,
then forgetting to swap the 2 and 1.

I assume that the second sbiw instruction is needed to put the pointer back at
the location it was at after the first sbiw.
Someone more knowledgeable about GCC's innards should check this.
Attached is a patch for the patch-xmega, um, patch.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43876

[Bug target/43876] [avr] Improper updating of struct members when written out of order from struct definition

[eric dot weddington at atmel dot com]

------- Comment #3 from eric dot weddington at atmel dot com  2010-06-09 02:13 -------
Confirmed.
Testing fix...


-- 

eric dot weddington at atmel dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |eric dot weddington at atmel
                   |dot org                     |dot com
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2010-06-09 02:13:23
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43876

[Bug target/43876] [avr] Improper updating of struct members when written out of order from struct definition

[eric dot weddington at atmel dot com]

------- Comment #4 from eric dot weddington at atmel dot com  2010-09-20 17:14 -------
AFAIK, fixed in the latest xmega patch, which is still not upstream.


-- 

eric dot weddington at atmel dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43876