[En-Nut-Discussion] Not fixed!!!: Re: confirmed!!! Re: NutOS 4.4.0 on ARM7: possibly bug in NutEnterCritical / NutExitCritical

[Nathan Moore]

>
> > Yes. As I understand the current implementation of NutEnterCritical /
> > NutExitCritical the original value of the CPSR is saved on the stack to
> > allow a later restore. This _must_ fail if the stack will be modified in
> > the meanwhile.
>
> I agree here as well and this is probably the cause of some strange
> behaviour happening when the code is compiled without optimization.


Well since you never can be sure what a compiler will produce I'm sure it
has probably caused problems at any optimization level.
If the compiler doesn't use frame pointers (gcc's -fomit-frame-pointer) and
there are local registers that are on the stack and the stack pointer is
used
in calculating their address this address will be off by one within a
critical
section.

>
>
> > Other operatin systems implement it the way you propose (e.g. Linux:
> > spinlock_irq_save()) and save the flags in a variable you need to
> > provide.
>
> This is what I wanted to avoid, but haven't found any alternate solution
> so far.

May I ask why?  I implemented an AVR version of this last night (for both
gcc and imagecraft) and after thinking about it realized that while it is
slightly
more of a headache to developers to use it can often result in smaller code
since compilers will often have a register with a dead value in it (ie, a
value that
will not be used again in the future) that it can use to hold the state
value and
the worst case would be that the compiler puts the state or another value to
the
stack, which would be exactly the same as the current situation except that
the compiler would be able to deal with it better.
I'm planning on reimplementing the current NutEnterCritical and
NutExitCritical
using my NutGetCritical and NutGiveUpCritical (ie, push and pop the varaible
that I then pass to Get/GiveUp) and then it would be easy to replace all
uses
and over time replace them with the direct calls to Get/GiveUp.
For AVR with GCC this would be something like:
#define NutEnterCritical() \
    do { \
         register Critical C;
         NutGetCritical( C ); \
         asm( "push %0\n\t" : "=r" (C) : ); \
    } while ( 0 )
With a pop before GiveUp in Exit.

>
>
> Some time ago a contributor suggested a different solution, which is
> still in arch/h8300/include/atom.h. It adds an opening curly brace to
> NutEnterCritical and a closing one to NutExitCritical.  The intention
> was to avoid difficult to find problems due to any missing
> NutExitCritical(). Btw. that's also the reason for NutJumpOutCritical().

This still causes problems.
   NutEnterCritical();
   if  (value_that_isr_can_change) {
     other_value_that_isr_can_change++;
     NutExitCritical();
     DoSomeStuff();
   } else {
      other_value_that_isr_can_change--;
      NutExitCritical();
      DoSomeOtherStuff();
   }
Code of the above form would fail to compile, and it may be possible that
other code might compile but do wrong stuff.
Passing code to execute to a macro might work better:
#define NutDoCritical( Code ) \
   do { \
       Critical C; /* this is the type I defined to hold the state */ \
       NutGetCritical( C ); \
       Code; \
       NutGiveUpCritical( C ); \
   } while (0)
Here you could pass inline statements, another macro, a function call, or a
comma separated list of statements:
NutDoCritical( (++x, y=*x) );
For anything except for very short and simple code this would look terrible,
but for most correct uses of critical sections
this wouldn't look bad since it would be just a single statement passed.

>
>
> May be we can use this to add a local variable to NutEnterCritical. This
> may break some existing code, but avoids re-writing all the critical
> sections.


I think that any solution that doesn't use a local variable to hold state
will have subtle flaws that will produce
either strange compiler errors for some code or possibly wrong results.

Nathan