avr-gcc-4.5.1 using uninitialized register?

Log in or register to post comments
Go To Last Post
5 posts / 0 new
Author
Message
dak664
dak664's picture
Level: Raving Lunatic
Joined: Sun. Jun 15, 2008
Posts: 2118 View posts
Location: North Carolina USA
#1
Posted by dak664: Sat. Jul 30, 2011 - 10:42 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Not sure if this is a bug, known or otherwise but if so I can try to distill it to the simplest case. Identical makefile and source in both cases, Os.
WinAVR gcc-4.3.3 works fine with this:

static u16_t
upper_layer_chksum(u8_t proto)
{
  u16_t upper_layer_len;
  u16_t sum;
  
  upper_layer_len = (((u16_t)(UIP_IP_BUF->len[0]) << 8) + UIP_IP_BUF->len[1] - uip_ext_len) ;
  
  /* First sum pseudoheader. */
  /* IP protocol and length fields. This addition cannot carry. */
  sum = upper_layer_len + proto;
  /* Sum IP source and destination addresses. */
  sum = chksum(sum, (u8_t *)&UIP_IP_BUF->srcipaddr, 2 * sizeof(uip_ipaddr_t));

  /* Sum TCP header and data. */
  sum = chksum(sum, &uip_buf[UIP_IPH_LEN + UIP_LLH_LEN + uip_ext_len],
               upper_layer_len);
    
  return (sum == 0) ? 0xffff : uip_htons(sum);
}

but 4.5.1 from AVR Studio 5 toolchain (and 4.5.2 according to another report) gives random results unless upper_layer_len and sum are declared volatile. Here is how 4.3.3 handles it, with some comments added:

static u16_t
upper_layer_chksum(u8_t proto)
{
    1144:	0f 93       	push	r16
    1146:	1f 93       	push	r17
  u16_t upper_layer_len; ;r16-17
  u16_t sum;             ;r24-25
  
  upper_layer_len = (((u16_t)(UIP_IP_BUF->len[0]) << 8) + UIP_IP_BUF->len[1] - uip_ext_len) ;
    1148:	10 91 a2 1e 	lds	r17, 0x1EA2 len[0]<<8
    114c:	00 e0       	ldi	r16, 0x00	; 0
    114e:	90 91 7e 03 	lds	r25, 0x037E ; r25=uip_ext_len
    1152:	09 1b       	sub	r16, r25
    1154:	11 09       	sbc	r17, r1     ; carry
    1156:	90 91 a3 1e 	lds	r25, 0x1EA3  ;len[1]
    115a:	09 0f       	add	r16, r25     ; r16-r17=upper_layer_len
    115c:	11 1d       	adc	r17, r1 ;OK so far
  
  /* First sum pseudoheader. */
  /* IP protocol and length fields. This addition cannot carry. */
  sum = upper_layer_len + proto;
  /* Sum IP source and destination addresses. */
  sum = chksum(sum, (u8_t *)&UIP_IP_BUF->srcipaddr, 2 * sizeof(uip_ipaddr_t));
    115e:	98 01       	movw	r18, r16 ; r18-19 = upper_layer_len
    1160:	28 0f       	add	r18, r24     ;   + proto, r24 now free
    1162:	31 1d       	adc	r19, r1      ;   carry
    1164:	c9 01       	movw	r24, r18 ;r24-25 = sum
    1166:	66 ea       	ldi	r22, 0xA6	 ; 166 ;&srcipaddr
    1168:	7e e1       	ldi	r23, 0x1E	 ; 30
    116a:	40 e2       	ldi	r20, 0x20	 ; 32 ;2*sizeof(uip_ipaddr_t)
    116c:	50 e0       	ldi	r21, 0x00	 ; 0
    116e:	0e 94 71 08 	call	0x10e2	 ; 0x10e2 


  /* Sum TCP header and data. */
  sum = chksum(sum, &uip_buf[UIP_IPH_LEN + UIP_LLH_LEN + uip_ext_len], upper_layer_len);
    1172:	60 91 7e 03 	lds	r22, 0x037E  ;
    1176:	70 e0       	ldi	r23, 0x00	; 0
    1178:	6a 53       	subi	r22, 0x3A	; 58
    117a:	71 4e       	sbci	r23, 0xE1	; 225
    117c:	a8 01       	movw	r20, r16
    117e:	0e 94 71 08 	call	0x10e2	; 0x10e2 

    
  return (sum == 0) ? 0xffff : uip_htons(sum);
    1182:	00 97       	sbiw	r24, 0x00	; 0
    1184:	19 f4       	brne	.+6      	; 0x118c 
    1186:	2f ef       	ldi	r18, 0xFF	; 255
    1188:	3f ef       	ldi	r19, 0xFF	; 255
    118a:	02 c0       	rjmp	.+4      	; 0x1190 
}

And here is 4.5.1. r19 seems to be used uninitialized; it seems it should be set to 0x1EA2 as for r17 above:

static u16_t
upper_layer_chksum(u8_t proto)
{
    10c8:	ef 92       	push	r14
    10ca:	ff 92       	push	r15
    10cc:	0f 93       	push	r16
    10ce:	1f 93       	push	r17
    10d0:	cf 93       	push	r28
    10d2:	df 93       	push	r29
  u16_t upper_layer_len;
  u16_t sum;
  
  upper_layer_len = (((u16_t)(UIP_IP_BUF->len[0]) << 8) + UIP_IP_BUF->len[1] - uip_ext_len) ;
    10d4:	00 91 7e 03 	lds	r16, 0x037E
    10d8:	20 e0       	ldi	r18, 0x00	; 0 ;r19 should be set to 0x1EA2?
    10da:	e9 01       	movw	r28, r18  ;r29 now undetermined
    10dc:	10 e0       	ldi	r17, 0x00	; 0
    10de:	c0 1b       	sub	r28, r16
    10e0:	d1 0b       	sbc	r29, r17
    10e2:	90 91 a3 1e 	lds	r25, 0x1EA3
    10e6:	c9 0f       	add	r28, r25
    10e8:	d1 1d       	adc	r29, r1
  
  /* First sum pseudoheader. */
  /* IP protocol and length fields. This addition cannot carry. */
  sum = upper_layer_len + proto;
  /* Sum IP source and destination addresses. */
  sum = chksum(sum, (u8_t *)&UIP_IP_BUF->srcipaddr, 2 * sizeof(uip_ipaddr_t));
    10ea:	7e 01       	movw	r14, r28
    10ec:	e8 0e       	add	r14, r24
    10ee:	f1 1c       	adc	r15, r1
    10f0:	c7 01       	movw	r24, r14
    10f2:	66 ea       	ldi	r22, 0xA6	; 166
    10f4:	7e e1       	ldi	r23, 0x1E	; 30
    10f6:	40 e2       	ldi	r20, 0x20	; 32
    10f8:	50 e0       	ldi	r21, 0x00	; 0
    10fa:	0e 94 3c 08 	call	0x1078	; 0x1078 

  /* Sum TCP header and data. */
  sum = chksum(sum, &uip_buf[UIP_IPH_LEN + UIP_LLH_LEN + uip_ext_len],
    10fe:	b8 01       	movw	r22, r16
    1100:	6a 53       	subi	r22, 0x3A	; 58
    1102:	71 4e       	sbci	r23, 0xE1	; 225
    1104:	ae 01       	movw	r20, r28
    1106:	0e 94 3c 08 	call	0x1078	; 0x1078 
               upper_layer_len);
    
  return (sum == 0) ? 0xffff : uip_htons(sum);
    110a:	00 97       	sbiw	r24, 0x00	; 0
    110c:	19 f0       	breq	.+6      	; 0x1114 
}

Tags:

Tools, Compilers and General Programming
Top
SprinterSB
SprinterSB's picture
Level: Raving Lunatic
Joined: Thu. Dec 21, 2006
Posts: 2113 View posts
Location: Saar-Lor-Lux
Posted by SprinterSB: Sun. Jul 31, 2011 - 09:56 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Reminds me of evil PR46779 which is fixed in 4.6.2+ and affects al least 4.3 ... 4.6.1. Note the bug is even in 4.3, but that does not mean it will show up.

But without a test case that will compiler it's just a guess (gessing what UIP_IP_BUF is, guessing what uip_ext_len is, guessing what u16_t is...)

avrfreaks does not support Opera. Profile inactive.

Top
dak664
dak664's picture
Level: Raving Lunatic
Joined: Sun. Jun 15, 2008
Posts: 2118 View posts
Location: North Carolina USA
Posted by dak664: Sun. Jul 31, 2011 - 04:06 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Here's the simplest reduction I can find. Anything simpler and r19 gets initialized. avr-gcc 4.5.1 from AVR Studio 5/AVR Toolchain.

main.c

#include 

uint8_t extvar,extbuffer[100];
uint16_t foo(uint8_t bar);

uint16_t extfunction(uint16_t arg1, uint16_t arg2) {
 return 0;
}

int main(void) {
while (1) {
  foo(42);
  }
}

test.c

#include 

extern uint8_t extvar,extbuffer[100];
extern uint16_t extfunction(uint16_t arg1,uint16_t arg2);

uint16_t
foo(uint8_t bar)
{
  uint16_t len;
  uint16_t sum;

  len = ((uint16_t)(extbuffer[50]) << 8) + extbuffer[51];   
 
  sum = len + bar;

  sum = extfunction(sum, len);

  sum = extfunction(sum, len);
  
  return sum;
}

makefile:

all:
	avr-gcc -Wall -mmcu=atmega1284p -gdwarf-2 -DF_CPU=8000000UL -Os -c main.c -o main.o
	avr-gcc -Wall -mmcu=atmega1284p -gdwarf-2 -DF_CPU=8000000UL -Os -c test.c -o test.o
	avr-gcc -mmcu=atmega1284p -Wl,-Map=test.map main.o test.o -o test.elf
	avr-objdump -S test.elf > test.asm

test.asm output showing r19 not initialized:

uint16_t
foo(uint8_t bar)
{
  c2:	ef 92       	push	r14
  c4:	ff 92       	push	r15
  c6:	cf 93       	push	r28
  c8:	df 93       	push	r29
  uint16_t len;
  uint16_t sum;

  len = ((uint16_t)(extbuffer[50]) << 8) + extbuffer[51];   
  ca:	20 e0       	ldi	r18, 0x00	; 0
  cc:	e9 01       	movw	r28, r18
  ce:	90 91 34 01 	lds	r25, 0x0134
  d2:	c9 0f       	add	r28, r25
  d4:	d1 1d       	adc	r29, r1
Top
SprinterSB
SprinterSB's picture
Level: Raving Lunatic
Joined: Thu. Dec 21, 2006
Posts: 2113 View posts
Location: Saar-Lor-Lux
Posted by SprinterSB: Sun. Jul 31, 2011 - 05:09 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It's PR46779. The bug's subject is from the reporter and confusing; it's nothing to do with array access.

avr-gcc-4.5.2 -Os -mmcu=atmega8 compiles the testcase

int bar (int);

int foo (unsigned char a, unsigned char b)
{ 
    int len = (a << 8) + b;

    return len + bar (len);
}

to

foo:
	push r14
	push r15
	push r28
	push r29
	ldi r24,lo8(0)
	movw r28,r24
	add r28,r22
	adc r29,__zero_reg__
	movw r24,r28
	rcall bar
	add r28,r24
	adc r29,r25
	movw r24,r28
	pop r29
	pop r28
	pop r15
	pop r14
	ret

Unfortunately, there is no command line option that forces the bug to go away.
-fno-split-wide-types might help but it's no guarantee, e.g. you could use a struct to do similar things that trigger the bug.

GCC 4.6.2 fixes this PR and is planned for late September or early October; Eric will need some time to make the WinAVR release.

If you need the fix in avr-gcc < 4.6 you can use the patch, backport it and recompile (you have the sources). I don't backport below 4.6 because I have limited time and therefore focus on 4.7.

avrfreaks does not support Opera. Profile inactive.

Top
dak664
dak664's picture
Level: Raving Lunatic
Joined: Sun. Jun 15, 2008
Posts: 2118 View posts
Location: North Carolina USA
Posted by dak664: Sun. Jul 31, 2011 - 05:30 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks! Patching mine would not help as this error will affect others using > 4.3.3 to build contiki.

Adding volatile to len seems to get it working without much of a performance hit:

uint16_t
foo(uint8_t bar)
{
  c2:	df 93       	push	r29
  c4:	cf 93       	push	r28
  c6:	00 d0       	rcall	.+0      	; 0xc8 
  c8:	cd b7       	in	r28, 0x3d	; 61
  ca:	de b7       	in	r29, 0x3e	; 62
  volatile uint16_t len;
  uint16_t sum;

  len = ((uint16_t)extbuffer[50] << 8) + extbuffer[51];
  cc:	30 91 33 01 	lds	r19, 0x0133
  d0:	20 e0       	ldi	r18, 0x00	; 0
  d2:	90 91 34 01 	lds	r25, 0x0134
  d6:	29 0f       	add	r18, r25
  d8:	31 1d       	adc	r19, r1
  da:	3a 83       	std	Y+2, r19	; 0x02
  dc:	29 83       	std	Y+1, r18	; 0x01
Top