David Brown <david.brown@hesbynett.no> writes:> The msp430 gcc port has an elegant method - it implements a "critical" > function attribute, which causes interrupt state to be saved and > disabled at the start of the function, and restored at the end. It > works fine with inline functions too. I would like to see that one > implemented on other gcc targets (if you have the spare time, DJ ?).That's a highly target-specific attribute, so each target maintainer would have to add it separately. I think some other targets have similar features, though, but it's not something standard across all targets.
gnu compiler optimizes out "asm" statements
Started by ●May 28, 2015
Reply by ●May 29, 20152015-05-29
Reply by ●May 29, 20152015-05-29
On Thu, 28 May 2015 21:23:35 -0500, Les Cargill wrote:> Tim Wescott wrote: >> This is related to my question about interrupts in an STM32F303 >> processor. It turns out that the problem is in the compiler (or I'm >> going insane, which is never outside the realm of possibility when I'm >> working on embedded software). >> >> I'm coding in C++, and I'm using a clever dodge for protecting chunks >> of code from getting interrupted. Basically, I have a class that >> protects a block of code from being interrupted. The constructor saves >> the interrupt state then disables interrupts, and the destructor >> restores interrupts. >> >> This has been reliable for me for years, but now the destructor is not >> being called. I suspect that the optimizer can't make sense of it >> because of the asm statements, and is throwing it away. >> >> If someone knows the proper gnu-magic to tell the optimizer not to do >> that, I'd appreciate it. I'm going to look in my documentation, but I >> want to make sure I use the right method, and don't just stumble onto >> something that works for now but should be depreciated, or is fragile, >> or whatever. >> >> Here's the "protect a block" class: >> >> typedef class CProtect { >> public: >> >> CProtect(void) >> { >> int primask_copy; >> asm("mrs %[primask_copy], primask\n\t" // save interrupt >> status >> "cpsid i\n\t" // disable interrupts : >> [primask_copy] "=r" (primask_copy)); >> _primask = primask_copy; >> } >> >> ~CProtect() >> { >> int primask_copy = _primask; >> // Restore interrupts to their previous value asm("msr primask, >> %[primask_copy]" : : [primask_copy] >> "r" (primask_copy)); >> } >> >> private: >> volatile int _primask; >> } CProtect; >> >> and here's how it's used: >> >> { >> CProtect protect; >> >> // critical code goes here >> } >> >> > > There are no side effects in the destructor.Yes there are. the MSR instruction loads the contents of a register (primask) into the mask register, setting the mask bit to whatever's in the register's bit 0.> Notes: > - extern is not extern "C". You can dern tootin' make 'em > extern "C" but I don't see the point. > - SFAIK, the "extern...inline" combination works. I have > no way to test it. "extern...inline' isn't critical; it's > just more or less an apology for making things slightly more > complex. > - If you are not using separate compilation it gets > even simpler. > > I would: > > #ifndef _CPROTECT_H_ > #define _CPROTECT_H_ > // CProtect.h > > extern void inline interruptsOn(void); extern void inline > interruptsOff(void); > > typedef class CProtect { > private: > > public: > > CProtect(void) > { > interruptsOff(); > } > > ~CProtect() > { > interruptsOn(); > } > > } CProtect; > #endif > > > > // CProtect.cpp > > #include <CProtect.h> > > static int _primask; > > void inline interruptsOff(void) > { > int primask_copy; > asm("mrs %[primask_copy], primask\n\t" // save interrupt status > "cpsid i\n\t" // disable interrupts : > [primask_copy] "=r" (primask_copy)); > _primask = primask_copy; > } > > void inline interruptsOn() > { > int primask_copy = _primask; > // Restore interrupts to their previous value asm("msr primask, > %[primask_copy]" : : [primask_copy] > "r" (primask_copy)); > }Yes, very good. And the first time you hit the block with interrupts OFF, they will be (erroneously) ON when you exit it. Hence, you want to save the interrupt state and restore it on exit. -- Tim Wescott Wescott Design Services http://www.wescottdesign.com
Reply by ●May 29, 20152015-05-29
On 29/05/15 22:37, DJ Delorie wrote:> > David Brown <david.brown@hesbynett.no> writes: >> The msp430 gcc port has an elegant method - it implements a "critical" >> function attribute, which causes interrupt state to be saved and >> disabled at the start of the function, and restored at the end. It >> works fine with inline functions too. I would like to see that one >> implemented on other gcc targets (if you have the spare time, DJ ?). > > That's a highly target-specific attribute, so each target maintainer > would have to add it separately. I think some other targets have > similar features, though, but it's not something standard across all > targets. >I know that it's a target-specific attribute, but in theory it could be made common across a range of targets (at least most typical embedded microcontrollers). If there were common functions such as __builtin_disable_interrupts returning the current interrupt mask, and __builtin_restore_interrupt_mask to restore it, you could make those target-specific while the "critical" attribute could be common to every target that implemented those builtins. For those of us who work with a variety of gcc microcontroller targets, it can be frustrating to see useful extensions from one target unavailable on other targets. As well as "critical", another that I miss on some targets is "naked".
Reply by ●May 29, 20152015-05-29
On Sat, 30 May 2015 01:57:47 +0200, David Brown wrote:> On 29/05/15 22:37, DJ Delorie wrote: >> >> David Brown <david.brown@hesbynett.no> writes: >>> The msp430 gcc port has an elegant method - it implements a "critical" >>> function attribute, which causes interrupt state to be saved and >>> disabled at the start of the function, and restored at the end. It >>> works fine with inline functions too. I would like to see that one >>> implemented on other gcc targets (if you have the spare time, DJ ?). >> >> That's a highly target-specific attribute, so each target maintainer >> would have to add it separately. I think some other targets have >> similar features, though, but it's not something standard across all >> targets. >> >> > I know that it's a target-specific attribute, but in theory it could be > made common across a range of targets (at least most typical embedded > microcontrollers). If there were common functions such as > __builtin_disable_interrupts returning the current interrupt mask, and > __builtin_restore_interrupt_mask to restore it, you could make those > target-specific while the "critical" attribute could be common to every > target that implemented those builtins. > > For those of us who work with a variety of gcc microcontroller targets, > it can be frustrating to see useful extensions from one target > unavailable on other targets. As well as "critical", another that I > miss on some targets is "naked".Part of the reason for the "CProtect" class is to allow me to wrap different things in the same syntax. It may not be portable outside of Wescott Design Services, but it means that my internal code can be portable across different processors. -- Tim Wescott Wescott Design Services http://www.wescottdesign.com
Reply by ●May 31, 20152015-05-31
David Brown wrote:> On 29/05/15 04:23, Les Cargill wrote: >> Tim Wescott wrote: >>> This is related to my question about interrupts in an STM32F303 >>> processor. It turns out that the problem is in the compiler (or I'm >>> going insane, which is never outside the realm of possibility when I'm >>> working on embedded software). >>> >>> I'm coding in C++, and I'm using a clever dodge for protecting chunks of >>> code from getting interrupted. Basically, I have a class that protects a >>> block of code from being interrupted. The constructor saves the >>> interrupt state then disables interrupts, and the destructor restores >>> interrupts. >>> >>> This has been reliable for me for years, but now the destructor is not >>> being called. I suspect that the optimizer can't make sense of it >>> because of the asm statements, and is throwing it away. >>> >>> If someone knows the proper gnu-magic to tell the optimizer not to do >>> that, I'd appreciate it. I'm going to look in my documentation, but I >>> want to make sure I use the right method, and don't just stumble onto >>> something that works for now but should be depreciated, or is fragile, or >>> whatever. >>> >>> Here's the "protect a block" class: >>> >>> typedef class CProtect >>> { >>> public: >>> >>> CProtect(void) >>> { >>> int primask_copy; >>> asm("mrs %[primask_copy], primask\n\t" // save interrupt status >>> "cpsid i\n\t" // disable interrupts >>> : [primask_copy] "=r" (primask_copy)); >>> _primask = primask_copy; >>> } >>> >>> ~CProtect() >>> { >>> int primask_copy = _primask; >>> // Restore interrupts to their previous value >>> asm("msr primask, %[primask_copy]" : : [primask_copy] >>> "r" (primask_copy)); >>> } >>> >>> private: >>> volatile int _primask; >>> } CProtect; >>> >>> and here's how it's used: >>> >>> { >>> CProtect protect; >>> >>> // critical code goes here >>> } >>> >> >> >> There are no side effects in the destructor. >> >> Notes: >> - extern is not extern "C". You can dern tootin' make 'em >> extern "C" but I don't see the point. >> - SFAIK, the "extern...inline" combination works. I have >> no way to test it. "extern...inline' isn't critical; it's >> just more or less an apology for making things slightly >> more complex. >> - If you are not using separate compilation it gets >> even simpler. >> >> I would: >> >> #ifndef _CPROTECT_H_ >> #define _CPROTECT_H_ >> // CProtect.h >> >> extern void inline interruptsOn(void); >> extern void inline interruptsOff(void); >> >> typedef class CProtect >> { >> private: >> >> public: >> >> CProtect(void) >> { >> interruptsOff(); >> } >> >> ~CProtect() >> { >> interruptsOn(); >> } >> >> } CProtect; >> #endif >> >> >> >> // CProtect.cpp >> >> #include <CProtect.h> >> >> static int _primask; >> >> void inline interruptsOff(void) >> { >> int primask_copy; >> asm("mrs %[primask_copy], primask\n\t" // save interrupt status >> "cpsid i\n\t" // disable interrupts >> : [primask_copy] "=r" (primask_copy)); >> _primask = primask_copy; >> } >> >> void inline interruptsOn() >> { >> int primask_copy = _primask; >> // Restore interrupts to their previous value >> asm("msr primask, %[primask_copy]" : : [primask_copy] >> "r" (primask_copy)); >> } >> > > This looks like you believe making an external function call enforces a > memory order. This is simply incorrect - if the compiler knows the > effects the function may have, it can use that information to re-order > the code. In particular, it knows something about the data the function > could access (it cannot legally access function-local or file static > data if the data does not "escape" by passing pointers to it out to > unknown code), and it may have full access to the source code through > inlining or link-time optimisation. >It's less that than that _primask is now a side effect. Again, I can't test this in his environment so the idea is to suggest a possible direction less than a destination. It may well be that interruptsOn() doesn't quite work unless you write back to _primask. Or something. Others have more directly addressed direct directives to tell the optimizer to hold off. I also understand not wanting to do that.> So while the use of external functions might appear to work, and /will/ > work in some contexts, it will work by accident of compiler flags and > limited compiler capabilities, rather than by design. The use of > "inline" (or "extern inline") here is utterly pointlessOf course.> - if the > compiler is able to inline the code, then you lose all hope of it > working.Not so much. It'll work either way.> And when the code is /not/ inlined, you generate significantly > bigger and slower code for something that should boil down to 2 > instructions for the constructor and 1 instruction for the destructor. >Life is like that. I would have used 'C' macros myself because those always work, but the use of a class to do this is pretty slick.> >-- Les Cargill
Reply by ●May 31, 20152015-05-31
Dave Nadler wrote:> On Thursday, May 28, 2015 at 10:17:18 PM UTC-4, Les Cargill wrote: >> ... >> CProtect(void) >> { >> interruptsOff(); >> } > > Careful Les, that's a bug if entered in a context where > interrupts are already disabled and must remain so... >"Doctor, doctor is hurts when I do that..." That is inherent in the design of this widget. -- Les Cargill
Reply by ●May 31, 20152015-05-31
Tim Wescott wrote:> On Thu, 28 May 2015 21:23:35 -0500, Les Cargill wrote: > >> Tim Wescott wrote: >>> This is related to my question about interrupts in an STM32F303 >>> processor. It turns out that the problem is in the compiler (or I'm >>> going insane, which is never outside the realm of possibility when I'm >>> working on embedded software). >>> >>> I'm coding in C++, and I'm using a clever dodge for protecting chunks >>> of code from getting interrupted. Basically, I have a class that >>> protects a block of code from being interrupted. The constructor saves >>> the interrupt state then disables interrupts, and the destructor >>> restores interrupts. >>> >>> This has been reliable for me for years, but now the destructor is not >>> being called. I suspect that the optimizer can't make sense of it >>> because of the asm statements, and is throwing it away. >>> >>> If someone knows the proper gnu-magic to tell the optimizer not to do >>> that, I'd appreciate it. I'm going to look in my documentation, but I >>> want to make sure I use the right method, and don't just stumble onto >>> something that works for now but should be depreciated, or is fragile, >>> or whatever. >>> >>> Here's the "protect a block" class: >>> >>> typedef class CProtect { >>> public: >>> >>> CProtect(void) >>> { >>> int primask_copy; >>> asm("mrs %[primask_copy], primask\n\t" // save interrupt >>> status >>> "cpsid i\n\t" // disable interrupts : >>> [primask_copy] "=r" (primask_copy)); >>> _primask = primask_copy; >>> } >>> >>> ~CProtect() >>> { >>> int primask_copy = _primask; >>> // Restore interrupts to their previous value asm("msr primask, >>> %[primask_copy]" : : [primask_copy] >>> "r" (primask_copy)); >>> } >>> >>> private: >>> volatile int _primask; >>> } CProtect; >>> >>> and here's how it's used: >>> >>> { >>> CProtect protect; >>> >>> // critical code goes here >>> } >>> >>> >> >> There are no side effects in the destructor. > > Yes there are. the MSR instruction loads the contents of a register > (primask) into the mask register, setting the mask bit to whatever's in > the register's bit 0. >I do not think the compiler understands side effects in asm code. SFAIK, those are opaque to the optimizer. If that were not true, it would not optimize the asm stuff out.>> Notes: >> - extern is not extern "C". You can dern tootin' make 'em >> extern "C" but I don't see the point. >> - SFAIK, the "extern...inline" combination works. I have >> no way to test it. "extern...inline' isn't critical; it's >> just more or less an apology for making things slightly more >> complex. >> - If you are not using separate compilation it gets >> even simpler. >> >> I would: >> >> #ifndef _CPROTECT_H_ >> #define _CPROTECT_H_ >> // CProtect.h >> >> extern void inline interruptsOn(void); extern void inline >> interruptsOff(void); >> >> typedef class CProtect { >> private: >> >> public: >> >> CProtect(void) >> { >> interruptsOff(); >> } >> >> ~CProtect() >> { >> interruptsOn(); >> } >> >> } CProtect; >> #endif >> >> >> >> // CProtect.cpp >> >> #include <CProtect.h> >> >> static int _primask; >> >> void inline interruptsOff(void) >> { >> int primask_copy; >> asm("mrs %[primask_copy], primask\n\t" // save interrupt status >> "cpsid i\n\t" // disable interrupts : >> [primask_copy] "=r" (primask_copy)); >> _primask = primask_copy; >> } >> >> void inline interruptsOn() >> { >> int primask_copy = _primask; >> // Restore interrupts to their previous value asm("msr primask, >> %[primask_copy]" : : [primask_copy] >> "r" (primask_copy)); >> } > > Yes, very good. And the first time you hit the block with interrupts > OFF, they will be (erroneously) ON when you exit it. >interruptsOn() is only called by the destructor. We presume the destructor is only called at the end of a block where a CProtect object has been instantiated.> Hence, you want to save the interrupt state and restore it on exit. >I believe what I put up there is equivalent to what you'd put up. I just added some scaffolding to tweak some of the framing rules in C++. If I made some mistake in that way, my bad - I have just used this technique with GNU before. -- Les Cargill
Reply by ●May 31, 20152015-05-31
On 31/05/15 18:28, Les Cargill wrote:> David Brown wrote: >> On 29/05/15 04:23, Les Cargill wrote: >>> Tim Wescott wrote: >>>> This is related to my question about interrupts in an STM32F303 >>>> processor. It turns out that the problem is in the compiler (or I'm >>>> going insane, which is never outside the realm of possibility when I'm >>>> working on embedded software). >>>> >>>> I'm coding in C++, and I'm using a clever dodge for protecting >>>> chunks of >>>> code from getting interrupted. Basically, I have a class that >>>> protects a >>>> block of code from being interrupted. The constructor saves the >>>> interrupt state then disables interrupts, and the destructor restores >>>> interrupts. >>>> >>>> This has been reliable for me for years, but now the destructor is not >>>> being called. I suspect that the optimizer can't make sense of it >>>> because of the asm statements, and is throwing it away. >>>> >>>> If someone knows the proper gnu-magic to tell the optimizer not to do >>>> that, I'd appreciate it. I'm going to look in my documentation, but I >>>> want to make sure I use the right method, and don't just stumble onto >>>> something that works for now but should be depreciated, or is >>>> fragile, or >>>> whatever. >>>> >>>> Here's the "protect a block" class: >>>> >>>> typedef class CProtect >>>> { >>>> public: >>>> >>>> CProtect(void) >>>> { >>>> int primask_copy; >>>> asm("mrs %[primask_copy], primask\n\t" // save interrupt >>>> status >>>> "cpsid i\n\t" // disable interrupts >>>> : [primask_copy] "=r" (primask_copy)); >>>> _primask = primask_copy; >>>> } >>>> >>>> ~CProtect() >>>> { >>>> int primask_copy = _primask; >>>> // Restore interrupts to their previous value >>>> asm("msr primask, %[primask_copy]" : : [primask_copy] >>>> "r" (primask_copy)); >>>> } >>>> >>>> private: >>>> volatile int _primask; >>>> } CProtect; >>>> >>>> and here's how it's used: >>>> >>>> { >>>> CProtect protect; >>>> >>>> // critical code goes here >>>> } >>>> >>> >>> >>> There are no side effects in the destructor. >>> >>> Notes: >>> - extern is not extern "C". You can dern tootin' make 'em >>> extern "C" but I don't see the point. >>> - SFAIK, the "extern...inline" combination works. I have >>> no way to test it. "extern...inline' isn't critical; it's >>> just more or less an apology for making things slightly >>> more complex. >>> - If you are not using separate compilation it gets >>> even simpler. >>> >>> I would: >>> >>> #ifndef _CPROTECT_H_ >>> #define _CPROTECT_H_ >>> // CProtect.h >>> >>> extern void inline interruptsOn(void); >>> extern void inline interruptsOff(void); >>> >>> typedef class CProtect >>> { >>> private: >>> >>> public: >>> >>> CProtect(void) >>> { >>> interruptsOff(); >>> } >>> >>> ~CProtect() >>> { >>> interruptsOn(); >>> } >>> >>> } CProtect; >>> #endif >>> >>> >>> >>> // CProtect.cpp >>> >>> #include <CProtect.h> >>> >>> static int _primask; >>> >>> void inline interruptsOff(void) >>> { >>> int primask_copy; >>> asm("mrs %[primask_copy], primask\n\t" // save interrupt status >>> "cpsid i\n\t" // disable interrupts >>> : [primask_copy] "=r" (primask_copy)); >>> _primask = primask_copy; >>> } >>> >>> void inline interruptsOn() >>> { >>> int primask_copy = _primask; >>> // Restore interrupts to their previous value >>> asm("msr primask, %[primask_copy]" : : [primask_copy] >>> "r" (primask_copy)); >>> } >>> >> >> This looks like you believe making an external function call enforces a >> memory order. This is simply incorrect - if the compiler knows the >> effects the function may have, it can use that information to re-order >> the code. In particular, it knows something about the data the function >> could access (it cannot legally access function-local or file static >> data if the data does not "escape" by passing pointers to it out to >> unknown code), and it may have full access to the source code through >> inlining or link-time optimisation. >> > > It's less that than that _primask is now a side effect. Again, > I can't test this in his environment so the idea is to suggest > a possible direction less than a destination. > > It may well be that interruptsOn() doesn't quite work > unless you write back to _primask. Or something. > > Others have more directly addressed direct directives to tell the > optimizer to hold off. I also understand not wanting to do that.Please read the posts I have made in this thread, that give complete and correct answers to the problem, along with an explanation for why my recommendation is correct, and as close to optimal as possible given the ease-of-use constraint. You may also like to read other posts in this thread. If you would prefer a list of issues with your class here: 1. It relies on having the interruptOn and interruptOff functions in an external module - as noted above, this is not safe. 2. It relies on a static object to hold the previous interrupt state. This means the method cannot be nested, making it useless for the task in hand. 3. It is seriously inefficient, meaning interrupts are disabled for much longer than necessary, and making critical code a costly operation rather than merely the 3 cycles or so that are actually needed. In embedded programming, this is important. 4. Even if the functions are treated as external black boxes, calling them will not enforce ordering on memory references which could not legally be accessed by an external function, thus defeating the purpose of the class.> >> So while the use of external functions might appear to work, and /will/ >> work in some contexts, it will work by accident of compiler flags and >> limited compiler capabilities, rather than by design. The use of >> "inline" (or "extern inline") here is utterly pointless > > Of course.Then why is it there in the first place?> >> - if the >> compiler is able to inline the code, then you lose all hope of it >> working. > > Not so much. It'll work either way.No, it will not work. Rather than repeating myself here, I direct you to my other posts in this thread (as well as earlier in this thread) where I have explained this. If you find flaws in something I wrote there, please say. If you don't understand the issues, please ask.> >> And when the code is /not/ inlined, you generate significantly >> bigger and slower code for something that should boil down to 2 >> instructions for the constructor and 1 instruction for the destructor. >> > > Life is like that. I would have used 'C' macros myself > because those always work, but the use of a class to do this > is pretty slick. >If your C macros are similarly incorrect, then you are merely lucky that they have worked. Feel free to post those macros, and I will try to comment on them too. Do you believe that my class, as posted earlier, has errors? If so, I would like to know - while I am confident that it is correct, I would be glad to hear of any suspected problems or situations where it would not work. But assuming it /is/ correct, why would you prefer to use a solution that is more limited in its use cases, dependent on the compiler's optimisation flags, and big and slow in the cases when it might work?
Reply by ●May 31, 20152015-05-31
On 31/05/15 18:29, Les Cargill wrote:> Dave Nadler wrote: >> On Thursday, May 28, 2015 at 10:17:18 PM UTC-4, Les Cargill wrote: >>> ... >>> CProtect(void) >>> { >>> interruptsOff(); >>> } >> >> Careful Les, that's a bug if entered in a context where >> interrupts are already disabled and must remain so... >> > > "Doctor, doctor is hurts when I do that..." >You are not in a position to be sarcastic here!> That is inherent in the design of this widget. >No, you are incorrect here. You either misunderstood Dave's comment, or you misunderstood your own code. Dave was incorrect in his complaint - as long as the class is used without any nesting or recursion, it will preserve the interrupt status on construction, and restore the previous state on destruction. However, it is misleading (and therefore bad coding) to call the destructor function "interruptsOn", because that is not what the function does - it should be called something like "interruptsRestore". Bad identifier naming is a crime in programming - it lead directly to Dave's misunderstanding here.
Reply by ●May 31, 20152015-05-31
On Sunday, May 31, 2015 at 4:35:16 PM UTC-4, David Brown wrote:> Dave was incorrect in his complaint - as long as the class is used > without any nesting or recursion, it will preserve the interrupt status > on construction, and restore the previous state on destruction. > > However, it is misleading (and therefore bad coding) to call the > destructor function "interruptsOn", because that is not what the > function does - it should be called something like "interruptsRestore". > Bad identifier naming is a crime in programming - it lead directly to > Dave's misunderstanding here.Yep, I thought he was re-enabling interrupts in all cases... I've been using the ctor/dtor pattern for interrupt bracketing and similar resource management for decades, but I'll be fooled by poor naming every time! Thanks for the clarification, Best Regards, Dave
