Incorrect return from chSchGoSleepTimeoutS()

Discussions and support about ChibiOS/NIL, the almost nil RTOS.
wgreiman
Posts: 33
Joined: Sun Oct 23, 2011 3:21 pm
Been thanked: 2 times

Incorrect return from chSchGoSleepTimeoutS()

Postby wgreiman » Tue Jan 10, 2017 6:13 pm

recently the avr-gcc compiler used by Arduino was change and now chSchGoSleepTimeoutS() returns the wrong status for an old version of nil.

I found the problem and checked for a fix in the latest version of nil. It appears it would have the same problem.

At line 680 of ch.c, trunk 10022, the wrong value is returned.

Code: Select all

    if (NIL_THD_IS_READY(ntp)) {
      nil.current = nil.next = ntp;
      if (ntp == &nil.threads[CH_CFG_NUM_THREADS]) {
        CH_CFG_IDLE_ENTER_HOOK();
      }
      port_switch(ntp, otp);
      return nil.current->u1.msg;  <<------------ Line 680.
    }


If I replace the return with this it is correct.

Code: Select all

 return otp->u1.msg;


Also it works if I make the the "current" member of nil_system_t volatile.

What is the best fix?

User avatar
Giovanni
Site Admin
Posts: 11217
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 398 times
Been thanked: 332 times
Contact:

Re: Incorrect return from chSchGoSleepTimeoutS()

Postby Giovanni » Tue Jan 10, 2017 6:27 pm

Hi,

Thanks for finding, I see problems with both solutions:

Making current volatile would probably increase code size, using "otp" could make the compiler save more registers to pass the value across port_switch() worsening stack usage.

Could you try to add a:

Code: Select all

  asm volatile ( : : : "memory");


To port_switch()? this should make the compiler understand the memory can be modified across the function. In other ports port_switch() is implemented into an asm module so this issue does not occurs.

Giovanni

wgreiman
Posts: 33
Joined: Sun Oct 23, 2011 3:21 pm
Been thanked: 2 times

Re: Incorrect return from chSchGoSleepTimeoutS()

Postby wgreiman » Tue Jan 10, 2017 6:54 pm

I tried putting this line in port_switch() with no improvement. I tied at the beginning and just befor the "ret"

Code: Select all

  asm volatile ("" : : : "memory");


If I put it between the port_switch() call and the return, like this, it works. The program actually uses two bytes less flash.

Code: Select all

      port_switch(ntp, otp);
      asm volatile ("" : : : "memory");       
      return nil.current->u1.msg;

User avatar
Giovanni
Site Admin
Posts: 11217
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 398 times
Been thanked: 332 times
Contact:

Re: Incorrect return from chSchGoSleepTimeoutS()

Postby Giovanni » Tue Jan 10, 2017 7:32 pm

I see, however placing it in the portable code is not appropriate.

Probably the proper fix would be to write port_switch() into and asm module like other ports do, the compiler fails to see that the function alters memory for some reason, it assumes it to be pure.

Other things that could be tried:
1) Add ": : : "memory"" to one of the existing statements but I don't think it will change.
2) Add "noinline" attribute to port_switch().
3) Rename port_switch() in _port_switch() then create a macro in chcore.h port_switch() that calls _port_switch() then includes the memory barrier. This way it would not be in the portable code.

Giovanni

wgreiman
Posts: 33
Joined: Sun Oct 23, 2011 3:21 pm
Been thanked: 2 times

Re: Incorrect return from chSchGoSleepTimeoutS()

Postby wgreiman » Tue Jan 10, 2017 8:01 pm

Giovanni,

Thanks for all the help. The new avr-gcc compiler is causing me grief in a lot of my open source software.

I tried adding ":::"memory" to an existing statement earlier and it failed.

I didn't try adding "noinline" since I like the macro idea.

I defined this macro in chcore.h and it works.

Code: Select all

#define port_switch(ntp, otp) {_port_switch(ntp, otp); \
                               asm volatile ("" : : : "memory");}

User avatar
Giovanni
Site Admin
Posts: 11217
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 398 times
Been thanked: 332 times
Contact:

Re: Incorrect return from chSchGoSleepTimeoutS()

Postby Giovanni » Tue Jan 10, 2017 8:12 pm

Thanks, I will do the same change in next release.

Giovanni


Return to “ChibiOS/NIL”

Who is online

Users browsing this forum: No registered users and 1 guest