Compiler Bug: incorrect code generated with -Os

Go To Last Post
19 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I've spent many hours the last week or two debugging a problem which is apparently the result of incorrect code generated with optimization set to -Os. The total code is fairly large (13,134 bytes). I've tried to reduce it to a minimum size but everything I've tried which is smaller works. The code works with no optimization and when I set the toolchain flavour to WinAVR-20100101.

The code is built for an ATmega644.

Here's the Native Studio 6 (Version 6.0.1703 - beta) -Os generated code:

ptr = IINCHIP_READ( Sn_TX_WR0( s ) );
  27b8:	ed b7       	in	r30, 0x3d	; 61
  27ba:	fe b7       	in	r31, 0x3e	; 62
  27bc:	36 96       	adiw	r30, 0x06	; 6
  27be:	0f b6       	in	r0, 0x3f	; 63
  27c0:	f8 94       	cli
  27c2:	fe bf       	out	0x3e, r31	; 62
  27c4:	0f be       	out	0x3f, r0	; 63
  27c6:	ed bf       	out	0x3d, r30	; 61
  27c8:	c8 01       	movw	r24, r16
  27ca:	0e 94 62 0e 	call	0x1cc4	; 0x1cc4 
ptr = ( ( ptr & 0x00ff ) << 8 ) + IINCHIP_READ( Sn_TX_WR0( s ) + 1 );
  27ce:	20 e0       	ldi	r18, 0x00	; 0
  27d0:	e9 01       	movw	r28, r18
  27d2:	c6 01       	movw	r24, r12
  27d4:	0e 94 62 0e 	call	0x1cc4	; 0x1cc4 
  27d8:	c8 0f       	add	r28, r24
  27da:	d1 1d       	adc	r29, r1

And here's the WinAVR-20100101 -Os generated code:

ptr = IINCHIP_READ( Sn_TX_WR0( s ) );
  292e:	ed b7       	in	r30, 0x3d	; 61
  2930:	fe b7       	in	r31, 0x3e	; 62
  2932:	36 96       	adiw	r30, 0x06	; 6
  2934:	0f b6       	in	r0, 0x3f	; 63
  2936:	f8 94       	cli
  2938:	fe bf       	out	0x3e, r31	; 62
  293a:	0f be       	out	0x3f, r0	; 63
  293c:	ed bf       	out	0x3d, r30	; 61
  293e:	c6 01       	movw	r24, r12
  2940:	0e 94 58 0e 	call	0x1cb0	; 0x1cb0 
ptr = ( ( ptr & 0x00ff ) << 8 ) + IINCHIP_READ( Sn_TX_WR0( s ) + 1 );
  2944:	18 2f       	mov	r17, r24
  2946:	00 e0       	ldi	r16, 0x00	; 0
  2948:	c7 01       	movw	r24, r14
  294a:	0e 94 58 0e 	call	0x1cb0	; 0x1cb0 
  294e:	08 0f       	add	r16, r24
  2950:	11 1d       	adc	r17, r1

The code makes 2 calls to IINCHIP_READ (which returns a uint8_t in r24) and combines the results in ptr, a uint16_t.

Note that the WinAVR-20100101 generated code saves the result from the first IINCHIP_READ call in r17 (mov 17, r24). The Native generated code doesn't use the result from the first IINCHIP_READ call at all.

The WinAVR-20100101 result is 0xD240 and the Native result is 0x0040.

I'd like to make a bug report but like I mentioned, I can only create this with a significant amount of code. Any ideas? Atmel support - should I submit a bug report and provide all my code?

Don

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Here's a couple of additional observations. There are 7 places in the code (which, by the way, isn't mine) that use nearly identical constructs. That is:

  val0 = IINCHIP_READ( Sn_TX_FSR0( s ) );
  val0 = ( val0 << 8 ) + IINCHIP_READ( Sn_TX_FSR0( s ) + 1 );

For 4 of them, correct code is generated. The other three have incorrect code generated (the result of the first IINCHIP_READ call is discarded and the upper byte of the uint16_t is set to zero).

I tried changing the code from addition to logical or:

  ptr = IINCHIP_READ( Sn_TX_WR0( s ) );
#if 0
  ptr = ( ( ptr & 0x00ff ) << 8 ) + IINCHIP_READ( Sn_TX_WR0( s ) + 1 );
#else
  ptr = ( ( ptr & 0x00ff ) << 8 ) | IINCHIP_READ( Sn_TX_WR0( s ) + 1 );
#endif

In all cases, the Native Studio 6 compiler then generates the correct code.

Don

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This definitely looks like a bug to me.

It is certainly legal C.
OTOH, you could say that any author who dreams up such constructs deserves it.

It is such a common operation that most people would use more straightforward code. IINCHIP_READ() looks like a macro, so it may not evaluate to a strictly typed expression. Most people would say:

  ptr  = IINCHIP_READ( Sn_TX_WR0( s ) ) << 8;    // hi
  ptr |= IINCHIP_READ( Sn_TX_WR0( s+1 ) ) & 255;    // lo

They might even use += in the second statement. Have you tried that? I bet it works fine.

Perhaps SprinterSB can shed some light on it.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

david.prentice wrote:
IINCHIP_READ() looks like a macro, so it may not evaluate to a strictly typed expression.
My first thought was that IINCHIP_READ was a macro too. It's not, it's really a function that returns a uint8_t. Sn_TX_WR0 is a macro which evaluates to a uint16_t at run time.
david.prentice wrote:
Most people would say:
  ptr  = IINCHIP_READ( Sn_TX_WR0( s ) ) << 8;    // hi
  ptr |= IINCHIP_READ( Sn_TX_WR0( s+1 ) ) & 255;    // lo

They might even use += in the second statement. Have you tried that? I bet it works fine.

Just tried it. The |= compiles correctly. The += does not!

Don

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If += does not work, that is a serious bug.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

"IINCHIP_READ" looks like adaptation of WizNet's code for their ethernet modules.
(I use them, though I don't use their code).

Here's the function from their 5100 chip code

uint8 IINCHIP_READ(uint16 addr)
{
	uint8 data;

...

  return(data);
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

stevech wrote:
"IINCHIP_READ" looks like adaptation of WizNet's code for their ethernet modules.
(I use them, though I don't use their code).
Yes, that's what it is. I've done a lot of clean up on the code and fixed some problems. For example, I'm using the standard AVR data types not the original typedef's (uint8_t instead of uint8).

BTW: I've been successful in creating a much smaller code sample that exhibits this problem. I'm going to create an official bug report if I can ever get a logon to Bugzilla.

Don

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

avrfreaks does not support Opera. Profile inactive.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yup - I think that's it.

Now, how do we get Atmel to fix it (or at least make sure they're aware of it)? I've request a Bugzilla signon but still have not received a response (see Reporting bugs for the beta release of Atmel Studio 6).

Don

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Does Atmel/Studio 6 mean that Atmel willsustain the Atmel variants of GCC?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I've put this bug in the internal bugzilla with reference number AVRTC-476 . Let's see what the compiler guys say.

Dan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

My latest tests indicate that this problem has been fixed with Atmel Studio 6 production release (Version 6.0.1843) with AVR Toolchain 8 Bit (3.4.0.663 - GCC 4.6.2).

Don

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have a question
How does this function work?

uint8 IINCHIP_READ(uint16 addr)
{
      return (*((uint8*)addr));

or this function?

void IINCHIP_WRITE(uint16 addr, uint8 data)
{
      (*((vuint8*)addr)) = data;

I dont get it. How ATMEGA128 is connected to W5300?
W5300 has a 8 bit Data bus and 10 bit Address bus. Where to connect them ?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

I have a question
How does this function work?

While superficially the context is the same, your question is not much related to the original subject of this thread. (It is also a fact that this thread has been dormant for 14 months..)

When you have a question that is not directly related to anything you find here at AVRfreaks then you should start your own new thread. Use the "New subject" button (not the "New reply" button) for that.

I'll ask a moderator to break off your question into it's own thread.

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The first one takes a 16 bit number 0x0000 to 0xFFFF. It casts it to be interpreted as a pointer to unsigned 8 bit then reads the contents of the location. So if location 0xBABE in memory holds 0xC7 and you call IINCHIP_READ(0xBABE) it will return 0xC7.

The write function is very similar, IINCHIP_WRITE(0xFACE, 0x55) will write the 8 bit value 0x55 to location 0xFACE. By casting the 0xFACE vale to be a pointer to uint8_t then writing through that pointer.

[Johan, I'm not going to attempt splitting a thread while not using a real computer, I'll do it later if no else has by then]

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@JohanEkdahl
Your right. I was aware of this. Then I realized there isn't much topics around W5300 so it would be better if I ask here. It's better than nothing! Plus I saw that two functions being discussed here.

Quote:

The write function is very similar, IINCHIP_WRITE(0xFACE, 0x55) will write the 8 bit value 0x55 to location 0xFACE. By casting the 0xFACE vale to be a pointer to uint8_t then writing through that pointer.
@clawson
Thank you. In IINCHIP_WRITE function how come it uses Volatile (vuint8) not (uint8) like in IINCHIP_READ ?

Actually something that I haven't realized yet is how exactly W5300 communicates with Mega128?
In IINCHIP_READ or IINCHIP_WRITE functions you are reading from or writing to a memory. What does it have to do with W5300 chip? How does it program W5300 registers?
Are the W5300 registers mapped to AVR memory somehow or what??

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

how come it uses Volatile (vuint8) not (uint8)

To ensure that the compiler makes the access and doesn't simply discard the code.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

Then I realized there isn't much topics around W5300 so it would be better if I ask here. It's better than nothing!

The alternative wasn't "nothing". The alternative was a separate thread. With that you could e.g. have a title that attracted more people interested in your specific problem. E.g. "Wiznet 5300 connections and driver functions". As it stands now it hides behind the (for you) meaningless title "Compiler bug [...]".

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

mostafanfs wrote:
I dont get it. How ATMEGA128 is connected to W5300?
W5300 has a 8 bit Data bus and 10 bit Address bus. Where to connect them ?
I started this thread more than a year ago. I'm using a WIZnet W5100 and an ATMega644. I'm using the W5100 Serial Peripheral Interface (SPI) not the 15-bit address/8-bit data interface.

Don