Atomic_*s are not atomic?!

So I’ve just started porting the FreeRTOS Kernel (11.0.1) to a new architecture and have come across the following:

In atomic.h, ATOMIC_ENTER_CRITICAL and ATOMIC_EXIT_CRITICAL are defined differently depending on whether portSET_INTERRUPT_MASK_FROM_ISR is defined! However, in FreeRTOS.h this macro is stubbed out if it is not already defined by the port.

This means that on all ports that do not, in fact, define their own (and functional) portSET_INTERRUPT_MASK_FROM_ISR, the atomics will end up silently not being atomic resulting in who-knows-what sort of bugs.

This is particularly egregious since the template port does not even mention that macro in its template portmacro.h!

There’s a possibly related but not quite identical post that dates back 7 years. So this may have been there a while? Unfortunately I can’t seem to link to the post since I just signed up, but the topic number is 6295.

This is not correct - if the port does not define portSET_INTERRUPT_MASK_FROM_ISR, ATOMIC_ENTER_CRITICAL and ATOMIC_EXIT_CRITICAL are defined using portENTER_CRITICAL and portEXIT_CRITICAL - FreeRTOS-Kernel/include/atomic.h at main · FreeRTOS/FreeRTOS-Kernel · GitHub.

Are you facing an issue on any platform?

I hate to disagree with you, but they are not. If the port does not define portSET_INTERRUPT_MASK_FROM_ISR then FreeRTOS.h does and ATOMIC_ENTER_CRITICAL is still defined in terms of the, now stubbed, portSET_INTERRUPT_MASK_FROM_ISR because it uses #if defined in line 62.

I believe Sebastian’s observation is correct and

#if defined( portSET_INTERRUPT_MASK_FROM_ISR )

needs to be changed to

#if portSET_INTERRUPT_MASK_FROM_ISR

in freertos.h for the intended semantic to work (should be easy to verify).

In any case, the stub in freertos.h is strange as a parameterized macro is #defined as 0. That appears to violate a few design rules, even if it may work.

I think the key is that the function-like macro portSET_INTERRUPT_MASK_FROM_ISR is supposed to be used as a function returning a value, so the constant 0 gives such a value, which will then be ignored by portCLEAR_INTERRUPT_MASK_FROM_ISR.

But yes, since the macro portSE_INTERRUPT_MASK_FROM_ISR is always defined by FreeRTOS.h its presence can’t be used to know that it is usable.

#if portSET_INTERRUPT_MASK_FROM_ISR unfortunately is likely not going to work with ports that actually do define this macro since they are unlikely to define it to something that can be evaluated by the pre-processor.

I think the best solution is to not stub these in FreeRTOS.h at all. If the port does not provide one, they should remain undefined. Anything that breaks will have already been broken because it used bad implementations of these macros. In this case atomic.h can stay as it is.

Except the Kernal uses them, allowing them to be effectively NOPs on processors that don’t need critical sections in ISRs.

Perhaps FreeRTOS.h should define derived macros portSET_INTERRUPT_MASK_FROM_ISR_if_supported that are functional or stubbed and the kernel should use those. That way every client that uses them can see from the name that these do not always do what one might expect. This approach will break every client of these macros that uses them unconditionally thus requiring someone to inspect the client to make sure it is indeed only ever run from ISR context (and thus can live with the stubbed versions). If so, they can use the new versions instead. If not, the client is already broken and needs changing. I consider this loud breakage an advantage as it forces someone to consider every use-case and make a call.

The only alternative I can think off is to have a completely separate macro that indicates whether or not portSET_INTERRUPT_MASK_FROM_ISR are useable. This macro can be defined by FreeRTOS.h if portSET_INTERRUPT_MASK_FROM_ISR was already defined by the port or left undefined if FreeRTOS.h stubbed it. Then atomic.h can use the new macro to test. The down-side of that is, that it only fixes atomic.h. If the same problem has happened somewhere else, it remains undetected.

There’s also a related, more conceptual issue:

Are the atomics supposed to be useable from ISR context? Their preferred use of portSET_INTERRUPT_MASK_FROM_ISR seems to indicate so. However, in that case is it even valid for them to fall back on portENTER_CRITICAL? I assumed the whole point of the former is that portENTER_CRITICAL is not (always) useable from ISR context? If portENTER_CRITICAL is always useable from ISR context, then do we even need portSET_INTERRUPT_MASK_FROM_ISR?

Or was the whole point of the distinction that portENTER_CRITICAL is always useable and always works (both from ISR and not) but always takes time whereas portSET_INTERRUPT_MASK_FROM_ISR is only to be used from ISR context and only takes time on ports that need it (because they have nested interrupts).

I believe this bug originates from different people having different understanding of those distinctions. The name portSET_INTERRUPT_MASK_FROM_ISR aligns with a lot of other FreeRTOS functions which can be used both from ISR and user contexts which suggests one thing, but the way the macro is currently defined (being stubbed out on non-nesting ports) seems to indicate another. I think this needs to be cleared up and very prominently defined somewhere (preferably in the symbol name).

If the latter interpretation of portSET_INTERRUPT_MASK_FROM_ISR is correct and the atomics can be used from non-ISR context, then they should simply never use that macro at all.

portENTER_CRITIAL might not work in an ISR if you have nestable interrupts. For ports that don’t support interrupt nesting, I suspect that it may work for most cases, as the issues needing the extra features that the ISR version support aren’t needed.

portENTER_CRITIAL might not work in an ISR if you have nestable interrupts. For ports that don’t support interrupt nesting, I suspect that it may work for most cases, as the issues needing the extra features that the ISR version support aren’t needed.

Ok, in that case the separate signalling macro I suggested is probably best. I.e. something like the following:

  • Ports that support nested interrupts define both portENTER_CRITIAL (which is only required to work outside ISR context) and portSET_INTERRUPT_MASK_FROM_ISR (which is required to work from ISR and user context)
  • Ports without nesting ints only define portENTER_CRITIAL which must work both inside and outside ISRs
  • FreeRTOS.h checks if portSET_INTERRUPT_MASK_FROM_ISR is defined. If so, it defines the symbol portHAS_NESTED_INTERRUPTS to 1. If it is not defined, it stubs it (and it’s exit counterpart) and defines portHAS_NESTED_INTERRUPTS to 0.
  • atomic.h uses #if portHAS_NESTED_INTERRUPTS to test
  • The documentation is updated to clearly state that
    – all ISR-only clients must use portSET_INTERRUPT_MASK_FROM_ISR
    – all non-ISR clients must use portENTER_CRITIAL
    – all mixed clients (may or may not be from ISR) must check portHAS_NESTED_INTERRUPTS and use either depending on that macro. In fact, I’d almost say it’d be worth adding a third macro in FreeRTOS.h that does exactly that and simply have all mixed clients use that instead.

Yes, there is sort of a need for 3 different sets of critical section routines/macros.

We want a simple to use for tasks (the current portENTER_CRITICAL) that is reliable at task level, and it currently seems to happen to work in ISRs for non-nesting systems (which in general don’t need critical sections).

We want a simple to use (but maybe a bit more complicated one) to handle critical sections in ISRs. For nesting systems, this needs it to return a value that is given to the end routine. For non-nesting systems, this should reduce to virtually no code (if possible). The current portSET_INTERRUPT_MASK_FROM_ISR macro works for this, and for nested interrupt systems, this turns out to be usable at task level too. (Sometimes I wish that portENTER_CRITCAL used the same signature).

There is also a need for a simple macro that is usable in either context for things like atomic, or other operations that the cost of testing if they are being used in an ISR is just too expensive. This appears to be able to be one or the other of these two (with a change of API for portENTER_CRITICAL) based on if the system has nested interrupts or not. Perhaps this could be called portENTER_CRITICAL_FROM_ALL.

So is there a bug-tracking system somewhere or something that this should go into? Sorry, but I literally only started my journey with FreeRTOS 2 days ago, so haven’t found all this sort of stuff.

Actually, this does not appear to be true (see also c - The subtle difference between #ifdef and #if for a macro defined as 0 - Stack Overflow).

I just tested this on gcc:

#define portSET_INTERRUPT_MASK_FROM_ISR 0

#ifdef portSET_INTERRUPT_MASK_FROM_ISR
#error "ifdef catches"
#endif 

#if portSET_INTERRUPT_MASK_FROM_ISR
#error "if catches"
#else
#error "if preprocessor does not catch"
#endif 

=>

ifdef catches
if preprocessor does not catch

The difference between #if and #ifdef seems fairly elementary to the C preprocessor, so I would not expect compatibility issues between compilers.

Note that the #if in atomic.h [edited, earlier revision that this would need to be done in freertos.h was wrong, of course] would only be used to define other macros.

I do agree, though that the coding is somewhat unfortunate and could/should be sanitized.

The problem with #if portSET_INTERRUPT_MASK_FROM_ISR is that it becomes a syntax error on machines like the Cortex M’s where it is actually implemented, and isn’t just a value of 0.

@sab There is a bug tracker on GitHub, where the download links point to.

I have created an issue on the GitHub tracker (936), but can’t add a link here because I’m too new.

The link is: [BUG] Atomic_*s are not atomic! · Issue #936 · FreeRTOS/FreeRTOS-Kernel · GitHub

well yes, of course, if portSET_INTERRUPT_MASK_FROM_ISR can be anything but a syntactic abstraction, that would be a severe bug, not resulting in runtime errors or unpredictable behavior, however, but build breaks. As long as a port does not provide a functional implementation of portSET_INTERRUPT_MASK_FROM_ISR, the code, although being ugly, underdocumented and questionable, would work as expected with the proposed change from #ifdef to #if in atomic.h; if not, it would result in compiler errors. I would therefore suggest to change the text of the bug description accordingly.

However, the thing that strikes me funny is that the test for a definition of portSET_INTERRUPT_MASK_FROM_ISR in freertos.h (which would already cause the build break) has been around since at least V10.3.1 released February 18 2020 (that is the earliest version trackable in github) and thus would have caused the build breaks. If this applied to the Cortex M port, there is no way that it would have gone unnoticed, Cortex M ports being among the most widely used. At that time I myself have used the M4 port on a daily basis, and I did not encounter any such problems. I briefly checked with the current M4F port, and there is no functional implementation of portSET_INTERRUPT_MASK_FROM_ISR. Which port(s) do you refer to?

I’m not sure what you are saying here at all. We have already established that, if a port does not provide portSET_INTERRUPT_MASK_FROM_ISR, then the current code silently breaks by not being atomic at all. The suggested modification (using #if instead of #ifdef) to which richard-damon was replying is also clearly not a fix since it will just do completely the wrong thing (trying to use a run-time value as a preprocessor directive) and probably synctactically break in most cases.

In my understanding, this is not a problem because the logic in freertos.h will enforce that portSET_INTERRUPT_MASK_FROM_ISR will be either anything defined elsewhere (in this case a mapping to a function) or 0. #if checks for 0 or anything else, so the #if test would be sufficient (see also the link to stackoverflow I posted earlier in this thread).

Again, in my book the way it is coded it is not a good coding style, but with the change from #ifdef to #if it would work as long as portSET_INTERRUPT_MASK_FROM_ISR is never implemented as a function - in which case we would see build errors, not potential runtime errors which we do face now (you point this out correctly in your bug report).