Strict aliasing and chains revisited

[Till Straumann]

  I didn't think about this carefully but I guess a clean solution
would be sacrificing the space for 1 pointer and make the
Chain_Control a proper struct comprised of 2 Chain_Nodes.

A hacked solution would be making Chain_Control a (hopefully properly 
packed)

union Chain_Control {
    struct {
       Chain_Node node;
       Chain_Node *fill;
    } head;
    struct {
       Chain_Node  *fill;
       Chain_Node  node;
    } tail;
} Chain_Control;


- Till

On 10/20/2010 06:52 AM, Sebastian Huber wrote:
> 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!
>