Strict aliasing and chains revisited

[Sebastian Huber]

Hello,

some time ago there was a discussion about strict aliasing problems in
combination with RTEMS chains:

http://www.rtems.com/ml/rtems-users/2006/november/msg00096.html

The basic problem shows up here:

http://www.rtems.com/ml/rtems-users/2006/november/msg00154.html

Unfortunately I was not able to figure out what action fixed the strict
aliasing problems.  We don't use the -fno-strict-aliasing option currently and
don't suffer from aliasing problems so it seems to be fixed somehow.

I started to clean up the chain implementation a bit.  I changed it such that
the only functions using fields of Chain_Control are:

_Chain_Head()
_Chain_Tail()

All other functions work with Chain_Node pointers.

This is still a hack since the Chain_Control has nothing to do with a
Chain_Node from a C standard perspective, but this should fix the aliasing
problems.

Please have a look at (patch):

https://www.rtems.org/bugzilla/show_bug.cgi?id=1711

During this work I fixed several chain API violations (direct usage of
structure fields) throughout the RTEMS sources and an old comment showed up (in
'cpukit/score/src/watchdoginsert.c'):

restart:
  delta_interval = the_watchdog->initial;

  /*
   * We CANT use _Watchdog_First() here, because a TICK interrupt
   * could modify the chain during the _ISR_Flash() below. Hence,
   * the header is pointing to volatile data. The _Watchdog_First()
   * INLINE routine (but not the macro - note the subtle difference)
   * casts away the 'volatile'...
   *
   * Also, this is only necessary because we call no other routine
   * from this piece of code, hence the compiler thinks it's safe to
   * cache *header!!
   *
   *  Till Straumann, 7/2003 (gcc-3.2.2 -O4 on powerpc)
   *
   */
  for ( after = (Watchdog_Control *) ((volatile Chain_Control *)header)->first ;
        ;
        after = _Watchdog_Next( after ) ) {

     if ( delta_interval == 0 || !_Watchdog_Next( after ) )
       break;

     if ( delta_interval < after->delta_interval ) {
       after->delta_interval -= delta_interval;
       break;
     }

     delta_interval -= after->delta_interval;

     /*
      *  If you experience problems comment out the _ISR_Flash line.
      *  3.2.0 was the first release with this critical section redesigned.
      *  Under certain circumstances, the PREVIOUS critical section algorithm
      *  used around this flash point allowed interrupts to execute
      *  which violated the design assumptions.  The critical section
      *  mechanism used here WAS redesigned to address this.
      */

     _ISR_Flash( level );

     if ( the_watchdog->state != WATCHDOG_BEING_INSERTED ) {
       goto exit_insert;
     }

     if ( _Watchdog_Sync_level > insert_isr_nest_level ) {
       _Watchdog_Sync_level = insert_isr_nest_level;
       goto restart;
     }
  }

This volatile cast was added 2003-07-18 to fix a synchronization problem with
ISRs.  I think that this fix is obsolete since 2006-03-07 because _ISR_Flash()
uses now a compiler memory barrier.  Proposed patch:

Index: score/src/watchdoginsert.c


===================================================================
RCS file: /usr1/CVS/rtems/cpukit/score/src/watchdoginsert.c,v
retrieving revision 1.10
diff -u -r1.10 watchdoginsert.c
--- score/src/watchdoginsert.c  17 Aug 2005 22:49:56 -0000      1.10
+++ score/src/watchdoginsert.c  20 Oct 2010 13:23:27 -0000
@@ -59,21 +59,7 @@
 restart:
   delta_interval = the_watchdog->initial;

-  /*
-   * We CANT use _Watchdog_First() here, because a TICK interrupt
-   * could modify the chain during the _ISR_Flash() below. Hence,
-   * the header is pointing to volatile data. The _Watchdog_First()
-   * INLINE routine (but not the macro - note the subtle difference)
-   * casts away the 'volatile'...
-   *
-   * Also, this is only necessary because we call no other routine
-   * from this piece of code, hence the compiler thinks it's safe to
-   * cache *header!!
-   *
-   *  Till Straumann, 7/2003 (gcc-3.2.2 -O4 on powerpc)
-   *
-   */
-  for ( after = (Watchdog_Control *) ((volatile Chain_Control *)header)->first ;
+  for ( after = _Watchdog_First( header ) ;
         ;
         after = _Watchdog_Next( after ) ) {

@@ -87,15 +73,6 @@

      delta_interval -= after->delta_interval;

-     /*
-      *  If you experience problems comment out the _ISR_Flash line.
-      *  3.2.0 was the first release with this critical section redesigned.
-      *  Under certain circumstances, the PREVIOUS critical section algorithm
-      *  used around this flash point allowed interrupts to execute
-      *  which violated the design assumptions.  The critical section
-      *  mechanism used here WAS redesigned to address this.
-      */
-
      _ISR_Flash( level );

      if ( the_watchdog->state != WATCHDOG_BEING_INSERTED ) {

Have a nice day!

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.