Compiler warnings when using atomic

Go To Last Post
21 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

When using atomic per the example below, the compiler issues a warning about the absence of a return statement.

 

Adding the dummy return avoids the warning and I think is optimised out by the compiler.

Provided result is assigned within the atomic block, I don't see a problem on moving the return outside the atomic block - it is a local var to this specif function call. \

Any thoughts?

Has anyone found a good solution to this already?

uint8_t afunc(void)
{
uint8_t result;
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
    {
        /// do stuff with protected resources - hence atomic - and assign a value to result
        return result;
    }
    return 0; // needed to avoid compiler warning of end of non void function without return
} 

 

l

l

 

regards
Greg

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The definition of ATOMIC_BLOCK involves a for loop with a local variable within to store SREG. The compiler believes that it's possible that this for loop could be skipped and therefore generated the warning.

 

Conclusion don't jump out of ATOMIC_BLOCK or NONATOMIC_BLOCK let them exit normally.

 

To be fair an unconditional return inside a code block just looks wrong.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

N.Winterbottom wrote:
To be fair an unconditional return inside a code block just looks wrong.
Especially if you are MISRA !

 

MISRA would suggest (mandate?) that functions only ever have one point of return: https://rules.sonarsource.com/c/...

 

But why would you write what you did in the first place? Why not simply:

uint8_t afunc(void)
{
    uint8_t result;
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
    {
        /// do stuff with protected resources - hence atomic - and assign a value to result
    }
    return result;
} 

What possible reason/motivation is there for putting the return inside the atomic block anyway?

Last Edited: Wed. Jun 15, 2022 - 08:19 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks gents

 

clawson wrote:

What possible reason/motivation is there for putting the return inside the atomic block anyway?

A change in architecture meant a set of existing functions needed interrupt protection.

I think of atomic as a compiler directive - implict cli()/sei() pairs - rather than the actual implementation as a for loop.

 

I just wrapped the existing code in atomic.... and gained the warnings.

I can't think of any need to keep the return inside the atomic and will change it.

 

A different but similar issue I had recently was porting some existing twi code (someone else's code)  into a naked ISR. The existing code contained a return statement in the middle of the ISR. This generated a jump to the end of my custom epilogue; after the reti.  Entirely logical, easily fixed, and confusing as $@#$@ when the PC happily arrived back at 000 without a reset :-(

 

regards
Greg

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

gregd99 wrote:
I think of atomic as a compiler directive - implict cli()/sei() pairs
And if that was how it was implemented think of the consequences of what you were doing. You may enter afunc() with interrupts enabled. Then if the ATOMIC block were just CLI (and SEI at the end) then you would be "returning" with the interrupt state having been switched from enabled to disabled!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

....interrupt state having been switched from enabled to disabled!

Indeed.

 

..... a smart compiler directive - smart cli()/sei() pairs

regards
Greg

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

With the current definitions of cli() and sei(),

ATOMIC_BLOCK can usually be replaced by a cli(), sei() pair.

ATOMIC_BLOCK also handles the case of a return inside the block.

 

I'm surprised that avr-gcc could not tell that the loop would not exit normally.

What was the optimization level?

 

There is a GNU pseudo-function, __builtin_unreachable(),

that tells gcc* that it is unreachable.

 

The memory barriers in the current cli() and sei()

are pretty heavy hammers.

The following might be useful

#define cliv()  SREG &= ~_BV(SREG_I)   // ideally, one instruction
#define seiv()  SREG |=  _BV(SREG_I)   // ideally, one instruction

Unlike inline assembly,

cliv() and seiv() are ordered with respect to volatile accesses.

I'd expect that to be sufficient in most cases.

 

*To be clear, the gcc compiler.

No need to wait for an assembler or linker message.

 

Edit: footnote

Moderation in all things. -- ancient proverb

Last Edited: Fri. Jun 17, 2022 - 03:43 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you so much for this discussion.  I have been dancing around a soft-reset problem when I called a function subject to interrupts.  Checked for un-handled interrupts, incorrect register bit settings, global variables, adding atomic blocks, etc with no success till now. 

 

The function had a return statement in each if/else block to indicate success or failure of the function's purpose.  This morning, I changed the code to add a variable in each block to indicate the function's return state and then placed one return statement with that variable at the end of the function.  Never would have guessed (lack of knowledge/ experience) that multiple returns was the problem. The program now works without the soft-rest.

 

Thanks again for you help,

Alan

Last Edited: Wed. Jun 15, 2022 - 02:59 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Happy to have provided a suggestion but if you are serious about writing solid code you might want to explore MISRA. Unfortunately it is not an open document so you do have to pay to access a copy

 

https://www.misra.org.uk/shop/

 

but some static code checkers (lint, QAC, Klockwork) include MISRA checking so they may be another way to see if your code conforms to MISRA rules/guidance.

 

A job I seem to have done a lot is reviewing/editing other people's code in our organization to try and clear every last MISRA violation! In the process you get to realise that everything they propose basically is just common sense/good practice anyway.

 

(the ones about not using malloc() and not using pre-pro macros can be quite a challenge in a large code base!)

Last Edited: Wed. Jun 15, 2022 - 03:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
MISRA would suggest (mandate?) that functions only ever have one point of return

 

MISRA, apparently, is a collection of bizarre anti-patterns. I vaguely recall that they also suggested explicitly casting the results of memory allocation functions (e.g. `malloc`)  O_o!

 

No, do not strive to have a single exit point in functions. 

 

Firstly, as a general advice, prepare yourself to exception-aware programming even if your programming language does not directly support exceptions at this point. In other words, assume that your function can suddenly exit from any point, despite your efforts to organize a single exit. Only special lowest-level functions can be excluded from this rule.

 

Secondly, more specifically, adopt "handle-simple-first" approach to programming: recognize, separate and handle trivial and simple cases first, to get them out of the way as quickly as possible and forget about them. Then, once you  are done with the "chaff", proceed to handle more complicated cases. This is expressed through early `return`s in functions and early `continue`s in cycles

 

void foo(...params...)
{
  if (trivial case)
  {
    /* Handle trivial case */
    return;
  }

  if (simple case)
  {
    /* Handle simple case */
    return;
  }

  /* Handle general case */
}

This "recognize-early-handle-and-forget" approach to function structure will significantly improve the readability (and therefore quality) of your code. Do not strive to refactor the above into a multileveled concoction of nested `if`s just for the sake of achieving a single exit point.

Dessine-moi un mouton

Last Edited: Wed. Jun 15, 2022 - 08:03 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Dealing with atomic in C++ is much nicer, as a simple class can let the the compiler handles it via constructor/destructor, and you just have to realize they take place in the current scope- constructor when it is reached, destructor when leaving the current scope.

 

https://godbolt.org/z/4d8s6ej8v

 

The nice thing is you can use the same thing for other mcu's, just need to modify the class to suite the hardware-

https://godbolt.org/z/rWe51fdva

 

Add that to the list of nice things you get when using C++.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

That one above is really a good example to demonstrate how easily you can rephrase most code parts to fulfil MISRA requirements:

 

void foo(...params...)
{
  if (trivial case)
  {
    /* Handle trivial case */
  }
  else if (simple case)
  {
    /* Handle simple case */
  }
  else
  {
    /* Handle general case */
  }
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Menahem wrote:

That one above is really a good example to demonstrate how easily you can rephrase most code parts to fulfil MISRA requirements:

 

No, no, no...

 

This completely defeats the purpose - the expressive strength - of my original example. The very key point there is the immediate visibility of the fact that the processing is over, it's done. The early `return` clearly telegraphs the fact that we've isolated a separate subset of "trivial" cases, handled them fully and exhaustively and now can forget about them for good in this function. This is exactly what those `return`s signify: we are done with these, no further processing is necessary, no need to look for anything extra down the code.

 

Your version obfuscates that fact: it forces the reader to suspect that something else, some additional processing might be present somewhere further down the code. It forces the reader to track the sequence of {}-pairs to determine where all these branches converge to a common point and check whether there is something there. That's much less readable. That is catastrophically less readable.

 

---

 

Once again, the early `return` approach should only be used when different branches of processing are obviously disbalanced. E.g. this

 

void foo(params)
{
  if (condition)
  {
    /* Trivial case - short code */
  }
  else
  {
    /* Complicated case - long and heavy code */
  }
}

is much better expressed as

 

void foo(params)
{
  if (condition)
  {
    /* Trivial case - short code */
    return;
  }

  /* Complicated case - long and heavy code */
}

Separate simple/trivial cases into short dedicated branches, fully process them, and make sure the reader understands that we are done with them by placing an explicit early `return` into the branch.

 

But when all branches of processing are approximately equal in complexity, then the early `return` technique becomes inappropriate. Again, this technique is only intended for quick getting-out-of-the-way of simple/trivial cases.

Dessine-moi un mouton

Last Edited: Wed. Jun 15, 2022 - 08:22 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The early return in the trivial case also has the advantage of less indentation in the complicated long and heavy code.

Less indentation tends to associate with fewer bugs.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

To nicely demonstrate AndreyT's point here's an extract from some real AVR code. https://github.com/arduino/ArduinoCore-avr/blob/master/cores/arduino/HardwareSerial.cpp

 

int HardwareSerial::read(void)
{
  // if the head isn't ahead of the tail, we don't have any characters
  if (_rx_buffer_head == _rx_buffer_tail) {
    return -1;
  } else {
    unsigned char c = _rx_buffer[_rx_buffer_tail];
    _rx_buffer_tail = (rx_buffer_index_t)(_rx_buffer_tail + 1) % SERIAL_RX_BUFFER_SIZE;
    return c;
  }
}

 

Bonus prizes are available for spotting the superfluous line.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

There is nothing wrong in finding our own style/perspective/mindset and this way creating well constructed, easily understandable, robust and still effective software products.

But the MISRA rules were made not for this. They cannot and dont want to help in writing code you like and clearly understand. It helps in writing code anybody can understand.

And do not forget about another important factor: besides a well designed code should be easily readable, intuitively(?) understandable, it should be easily maintainable (by anybody, pros and fledgelings) as well, and should contain as less error prone* elements as possible.

One more thing about MISRA rules (or any similar guidelines): you should not grab a single rule and lamenting about 'how stupid a rule it is', always the complete rule-set (or okay, if not all, but at least a comprehensive subset of rules) together should be considered.

 

*An example how inside returns can be dangerous (let's see your sample code): during the continuous developing, the code lines inside of your if statements are getting overgrown, and one of your younger colleagues gets the job to tidy up the code a bit. He or she then starts to reorganize the code and moves the inside parts of the if statements to separate functions. What will happen if he or she accidentaly also moves some or all of the return keywords as well?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Separate simple/trivial cases into short dedicated branches, fully process them, and make sure the reader understands that we are done with them by placing an explicit early `return` into the branch.

According to MISRA all cases should be simple and short. smiley

 

void foo(params)
{
  if (condition)
  {
    /* Trivial case - short code */
    return;
  }

  /* Complicated case - long and heavy code */
}

Why would you write there a long and heavy code?

 

[edited: second quote added]

Last Edited: Wed. Jun 15, 2022 - 09:28 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
void foo(type *ptr)
{
    if (ptr == NULL)
    {
        return;
    }

    /* code with 1 level indentation */
}

void bar(type *ptr)
{
    if (ptr != NULL)
    {
        /* code with 2 levels indentation */
    }
}

I think i prefer the 1st way if the code is more than a few lines and the 2nd way if the code is only a few lines!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

skeeve wrote:

ATOMIC_BLOCK also handles the case of a return inside the block.

Are you 100% sure about this? The only way how to be sure is scoped object with cli() in constructor and sei() in destructor (= c++, RAII idiom). I'll test this later, but I can't imagine how could it do anything after return (other than object destruction).

Computers don't make errors - What they do they do on purpose.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

This:

#include <util/atomic.h>

volatile uint8_t foo;

int main(void) {

    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        PORTB = foo;
    }
    while(1);
}

is preprocessed to be:

# 6 ".././main.c"
volatile uint8_t foo;

int main(void) {

# 10 ".././main.c" 3
   for ( uint8_t sreg_save __attribute__((__cleanup__(__iRestore))) = (*(volatile uint8_t *)((0x3F) + 0x20)), __ToDo = __iCliRetVal(); __ToDo ; __ToDo = 0 )
# 10 ".././main.c"
                                     {

# 11 ".././main.c" 3
       (*(volatile uint8_t *)((0x05) + 0x20))
# 11 ".././main.c"
             = foo;
    }
    while(1);
}

As I understand it the real "trick" here is the use of the __attribute__((__cleanup__(some_fn)))

 

I think one would need to read more about exactly what the action of __cleanup__ is and when it comes into play. It does sounds like it is when the thing is going out of context so in this sense it is like RAII for C.

 

In fact a quick google ("gcc attribute cleanup raii" gives LOADS of interesting results) leads to this among others:

 

https://echorand.me/posts/clean_...

 

That is suggesting that __attribute__((__cleanup__)) is a mechanism for doing RAII in C.

 

(when you consider that the (c) on util/atomic.h is 2007 a very young Dean was doing some pretty radical stuff a long time ago !)

Last Edited: Thu. Jun 16, 2022 - 09:43 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

KIIV_cz wrote:
skeeve wrote:

ATOMIC_BLOCK also handles the case of a return inside the block.

Are you 100% sure about this? The only way how to be sure is scoped object with cli() in constructor and sei() in destructor (= c++, RAII idiom). I'll test this later, but I can't imagine how could it do anything after return (other than object destruction).

Yes.  Clawson gives the mechanism.  The source for ATOMIC_BLOCK is not too hard to read.

I suspect issues like this are why MISRA requires one return per function.

 

When ATOMIC_BLOCK was new, cli() and sei(), did not have memory barriers.

Memory barriers are an explicit part of ATOMIC_BLOCK,

but now they are redundant.

With memory barriers part of cli() and sei(),

ATOMIC_BLOCK is only needed if a block might be left in the middle,

e.g. because of a return, break or continue.

Moderation in all things. -- ancient proverb