Re: [avr-gcc-list] Wrong excution order in 4.1.1, but not 3.4.5, regression?

[Bob Paddock]

On Tue, 20 Feb 2007 03:24:49 -0500, David Brown <address@hidden>  
wrote:

> If you can't post a cut-down version, then at least post a compilable  
> section of your code with the minimal necessary typedef's and other  
> definitions included.  To be able to test your code, people will have to  
> be able to reproduce the bug.

I understand the issue, but the Boss would not be happy
if I posted all of the code, and when I cut it down I can't
reproduce the problem

At the bottom of this message is a version that shows the essence of what
I'm trying to do, however it does not reproduce the initial problem,
which maybe leading us down a path that is a waste of everyones
time.  It does raise a question about assignment to volatiles.

 pattern_flash_ptr_reload_u8_v = (PGM_UP)  
pgm_read_word( &pattern_table[ pattern_u8++ ] );

Has been removed in both 3.4.5 and 4.1.1.  Should not the assignment to
a volatile always be done?

> If the same code is generating assembly with different ordering despite  
> the volatiles, then it is quite likely that there is a bug somewhere.

Yes.

> However, I don't know if the C standard is clear on expressions such as  
> "a = b = c", when some or all of these are volatile - it could be that  
> both implementations are correct because the rules are not strict.

I concede your point on the volatile issues.

<http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf>
5.1.2.3 point 3.
6.7.3

> I'd say that almost any code with "a = b = c" expressions is incorrect  
> code because it is far from clear what you mean,

I do not see how "a = b = c;" is in anyway outside of the C standard,
when volatiles are not involved, and the types are the same.
The order of assignment is always right-to-left.  We are not mixing
any left-to-right/right-to-lefts here.

<http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf>
6.5.16.2 Compound assignment

> When the code is re-written to actually say what you mean, do you still  
> get a difference?

There is no change, object code is identical between:

pattern_flash_ptr_u8_v = pattern_flash_ptr_reload_u8_v = (PGM_UP)  
pgm_read_word( &pattern_table[ pattern_u8++ ] );

and

pattern_flash_ptr_reload_u8_v = (PGM_UP)  
pgm_read_word( &pattern_table[ pattern_u8++ ] );
pattern_flash_ptr_u8_v = pattern_flash_ptr_reload_u8_v;

Footnote #114 seems to be violated here (removed), and in my original  
problem (reordered).
<http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf>

"114) A volatile declaration may be used to describe an object  
corresponding to a memory-mapped
input/output port or an object accessed by an asynchronously interrupting  
function [pattern_flash_ptr_u8_v]. Actions on objects so declared shall  
not be 'optimized out' by an implementation or reordered except as
permitted by the rules for evaluating expressions."

Leaving me to wonder what I'm missing about "permitted by the rules for  
evaluating expressions",
in this context.

Also I know from previous experiments that in AVR-GCC
"a = b = c;" produces smaller code, in some cases,
than "b = c; a = b;", otherwise the ojbect code size is the same.
The exception being if you are assigning
zero, due to r1 always containing zero, "a = 0; b = 0; c = 0;"
produces smaller code than "a = b = c = 0;"


<http://www.open-std.org/jtc1/sc22/wg14/www/standards>


Example Code, for Tiny2313.

#include <avr/interrupt.h>
#include <avr/io.h>
#include <avr/pgmspace.h>
#include <inttypes.h>

#define SLEEP()    __asm__ __volatile__ ("sleep" "\n\t" :: );

typedef uint8_t PROGMEM prog_uint8_t;
#define PGM_UP const    prog_uint8_t *

static volatile PGM_UP  pattern_flash_ptr_u8_v;
static volatile PGM_UP  pattern_flash_ptr_reload_u8_v;

static          uint8_t mode_button_press_count_u8, max_button_presses_u8  
= 2;
static          uint8_t pattern_u8;

ISR( TIMER1_COMPA_vect )
{
  PORTB = pgm_read_byte( pattern_flash_ptr_u8_v++ );  /* Output the  
pattern, then point at the next pattern */

  if( 0 == pgm_read_byte( pattern_flash_ptr_u8_v ) )/* A zero indicates  
that it is time to start pattern over */
	{/* If we have reached the end of the table, so restart at the beginning  
*/
      pattern_flash_ptr_u8_v = pattern_flash_ptr_reload_u8_v;
        }
}

#define LED2 _BV(5)

uint8_t model_100[] PROGMEM =
  {
        LED2,
        0 /* Mark end of table */
  };

PGM_UP pattern_table[] PROGMEM = {
  model_100,
  model_100,
  model_100,
  model_100,
  model_100,
};

int main( void )
{
  /* I/O Setup would be here */
  for(;;)/*ever*/
    {
      /* Clear variables for this run: */
      mode_button_press_count_u8 = 0; /* etc. */

      /* Enable Interrupts here, after loading default values for the  
pattern pointers */
      do{
        pattern_flash_ptr_reload_u8_v = (PGM_UP)  
pgm_read_word( &pattern_table[ pattern_u8++ ] ); /* Pattern will overflow  
array bounds in this example code */
        pattern_flash_ptr_u8_v = pattern_flash_ptr_reload_u8_v;
        /* Interrupts should be disabled during the above */

#if 1/* Setting to zero causes 4.1.1 to remove *all* of main */
        if( PINA & 0x01 )
          {/* Real code would debounce this */
            ++mode_button_press_count_u8;
          }
#endif
      }while( mode_button_press_count_u8 < max_button_presses_u8 );

      /* Enter sleep mode */
      SLEEP(); /* Sleep mode register setup, and hardware house keeping,  
omitted */
      /* Exit from sleep mode on port change IRQ from button press, also  
omitted*/
    } /* for(;;) */
}/* main() */