ATOMIC_BLOCK, control reaches end of non-void function

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

Whenever I try to exit a function by returning something from within ATOMIC_BLOCK, I get this warning:

warning: control reaches end of non-void function
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Show how you are actually attempting that. Are you using it in the form shown in the manual?

http://www.nongnu.org/avr-libc/u...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
int func(void)
{
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
    {
        return 1;
    }
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

But you don't want the return in the middle of the block. Assign what is being atomically accessed to a return variable then return that outside the atomic block:

int func(void) 
{ 
    int retval
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) 
    { 
        retval = something; 
    } 
    return retval;
} 

If you run your previous example through the pre-processor you get:

int func(void)
{
    for ( uint8_t sreg_save __attribute__((__cleanup__(__iRestore))) = (*(volatile uint8_t *)((0x3F) + 0x20)), __ToDo = __iCliRetVal(); __ToDo ; __ToDo = 0 )
    {
        return 1;
    }
}

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

clawson wrote:
But you don't want the return in the middle of the block.
Why not (apart from the warning)?
One advantage of the macro is that it regards every exit path.

Stefan Ernst

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

Quote:

(apart from the warning)?

But that's the very reason the OP started this thread. The two solutions are either to cover every potential return path with a return that provides a value - so in this case add a dummy "return N;" beyond the atomic block or to follow my advice.

Personally I don't like functions with multiple exit points unless it's totally unavoidable (like heavily nested error escapes).

If you later decide to add something that must be done before return (like deselecting an SPI device say) then having just one point of exit means you need only do it in one place - rather that duplicating it at several points.

But I guess that's just a question of personal preference over style/readability/maintainability.

Cliff

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

Cliff wrote:
If you later decide to add something that must be done before return (like deselecting an SPI device say) then having just one point of exit means you need only do it in one place - rather tha[n] duplicating it at several points.
Which then leads us to the "goto" rat hole of a discussion.

As you said, sometimes severely nested code leads to... ummm... uncomfortable coding.

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

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

Stu,

Yup as I'm sure you are aware, in "big systems" you tend to have a lot of malloc() and free() going on and it's all too easy to have code paths where you don't catch all the free()s required. Especially when you have that kind of:

{
  malloc()
  if (ptr) {
    malloc()
    if (ptr) {
      malloc()
      if (ptr) {
        ...
      }
    }
  }

thing going on. Spattering return's all over this is a classic way to inadvertently get an inbalance.

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

clawson wrote:
Quote:

(apart from the warning)?

But that's the very reason the OP started this thread. The two solutions are either to cover every potential return path with a return that provides a value - so in this case add a dummy "return N;" beyond the atomic block or to follow my advice.
In the example given, the warning is clearly superfluous.
My suspicion is that the OP has optimization turned off and that
the compiler never realizes that the for loop will never complete.

Iluvatar is the better part of Valar.

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

clawson wrote:
Quote:

(apart from the warning)?

But that's the very reason the OP started this thread. The two solutions are either to cover every potential return path with a return that provides a value - so in this case add a dummy "return N;" beyond the atomic block or to follow my advice.

Personally I don't like functions with multiple exit points unless it's totally unavoidable (like heavily nested error escapes).

Sorry Cliff, it was a misunderstanding then. I thought your "you don't want" was meant as "it won't work anyway".

Stefan Ernst

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

I only have one exit path, but I guess I will use the solution posted here anyway just to shut up the compiler.

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

Quote:
I only have one exit path

If you have a return in the middle of an atomic block, then you most certainly don't have only one exit path, at least as far as the compiler is concerned.

Regards,
Steve A.

The Board helps those that help themselves.

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

No, as has been stated this:

int func(void) 
{ 
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) 
    { 
        return 1; 
    } 
    return 12345;
} 

will work. Insert any int you like in place of "12345" above - that return statement will never be reached anyway.

However do follow the advice someone gave above and check your optimisation. The optimiser should have spotted that anything beyond the ATOMIC_BLOCK() that contains the return; would be unreachable anyway so that "return 12345" is unreachable, the compiler should know this and not care whether it's there or not.

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

I'm using -Os, so I don't think that's it.

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

Koshchi wrote:
Quote:
I only have one exit path

If you have a return in the middle of an atomic block, then you most certainly don't have only one exit path, at least as far as the compiler is concerned.
Memloop's example has only one exit path.
The warning is clearly superfluous.
Optimization turned off probably is.

Iluvatar is the better part of Valar.

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

I don't understand the point: shall interrupts remain disabled?

As this is defeating the very purpose of ATOMIC_BLOCK, (not to say ATOMIC_BLOCK has been shown not to ensure atomicity anyway), I'd recommend to rewrite this to asm, rather than to try to rape the compiler.

JW

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

wek wrote:
I don't understand the point: shall interrupts remain disabled?
With that macro the SREG is restored no matter what exit path is taken (even in case of the return).
So the point is to simply exit the function without jumping out of the block first.

Stefan Ernst

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

That's the effect, but surely not the intention? Placing a return inside a block intends to avoid any action represented by the rest of the block, including the implied actions at the closing bracket, e.g. the increment/compare in *for* etc.

Anyway, attempting to break out of a cludge like the ATOMIC_BLOCK is such a naughty thing that it deserves to result in an unexpectedly randomly working code, and the programmer's fingers hit by a school cane, rather than given hints how to avoid warnings.

JW

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

wek wrote:
That's the effect, but surely not the intention?

http://www.nongnu.org/avr-libc/u...
Quote:
Exit paths from both block types are all managed automatically without the need for special considerations, i. e. the interrupt status will be restored to the same value it has been when entering the respective block.

What part of that is ambiguous?

I'd prefer to write code like this:

if (!something)
{
    return ERROR;
}

/* A lot of code here that depends on something being in a good state. */
...

return NO_ERROR;

... instead of doing it like this:

err = NO_ERROR;
if (!something)
{
    err = ERROR;
}

if (err == NO_ERROR)
{
    /* Do something here. */
}

if (err == NO_ERROR)
{
    /* Do something else. */
}

...

return err;

Especially when you only have to worry about one single error point.

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

Quote:
will work. Insert any int you like in place of "12345" above - that return statement will never be reached anyway.

But the macro resolves to:

for (...)
{
    return 1:
}

But for loops are not guaranteed to be entered, so the compiler sees the possibility that the return 1 will not be hit.

Regards,
Steve A.

The Board helps those that help themselves.

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

Koshchi wrote:
Quote:
will work. Insert any int you like in place of "12345" above - that return statement will never be reached anyway.

But the macro resolves to:

for (...)
{
    return 1:
}

But for loops are not guaranteed to be entered, so the compiler sees the possibility that the return 1 will not be hit.

The compiler isn't seeing well, probably because optimization is turned off.
__iCliRetVal() always returns a true, so this for is guaranteed to be entered.

Iluvatar is the better part of Valar.

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

Memloop wrote:
Quote:
Exit paths from both block types[...]

What part of that is ambiguous?

Oh, but that's written by the same folks who make you believe ATOMIC_BLOCK will guarantee no code reordering (which is the very heart of atomicity) - and that's not the case.

Please listen carefully. ATOMIC_BLOCK is a kludge, not a fully engineered feature. In most of the simple cases (for which it was intended) it appears to work as advertised. Don't expect more; always expect less. Don't put kludge on kludge, don't mess with the mess.

JW

Last Edited: Mon. Mar 29, 2010 - 08:15 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I wrote:
In most of the simple cases (for which it was intended) it [ATOMIC_BLOCK] appears to work as advertised.

... which after a bit of thinking makes me wonder, what sort of tests do you perform which needs atomicity/protection?

Please don't take this as trolling - I understand the need for this and I do use such in my programs (not in conjunction with ATOMIC_BLOCK, though, mostly hand-crafted asm, but that you might've guessed by now probably). I recently started thinking about a set of safe (although less flexible) atomicity macros, and I would like to know the setups where these issues occur typically.

Would you please share the relevant snippets for further discussion?

Jan Waclawek

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

Memloop: Is optimization turned off?

Iluvatar is the better part of Valar.

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

skeeve wrote:
Memloop: Is optimization turned off?

No. I'm using -Os.

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

Quote:
__iCliRetVal() always returns a true, so this for is guaranteed to be entered.

But it also needs to look that the exit condition is that __ToDo is false, and that there is a definitive exit inside the loop. Though this certainly could be seen, I would not think it unusual that the optimizer would miss an optimization that relies on several conditions.

Regards,
Steve A.

The Board helps those that help themselves.

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

Koshchi wrote:
Quote:
__iCliRetVal() always returns a true, so this for is guaranteed to be entered.

But it also needs to look that the exit condition is that __ToDo is false, and that there is a definitive exit inside the loop. Though this certainly could be seen, I would not think it unusual that the optimizer would miss an optimization that relies on several conditions.
__ToDo never becomes false.
"return 1;" terminates the loop (and the function) without "incrementing".
Missing it seems odd to me, but I'm not an optimizer.
Too bad one can't put attribute((unroll)) on a for loop.

Iluvatar is the better part of Valar.

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

Quote:

Too bad one can't put attribute((unroll)) on a for loop.

I'd *kill* for that attribute - many times I've had some performance critical repeated code (think 64 copies of a SPI byte transfer, to reduce loop latency) and wanted a quick and easy way to just say "copy this N times". It's probably possible with some preprocessor trickery and a few intermediate header files, but when you start considering that the CTRL+C CTRL+V keys look pretty good after all.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

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

abcminiuser wrote:
Quote:

Too bad one can't put attribute((unroll)) on a for loop.

I'd *kill* for that attribute - many times I've had some performance critical repeated code (think 64 copies of a SPI byte transfer, to reduce loop latency) and wanted a quick and easy way to just say "copy this N times". It's probably possible with some preprocessor trickery and a few intermediate header files, but when you start considering that the CTRL+C CTRL+V keys look pretty good after all.


#ifndef __UNROLL_H
#define __UNROLL_H 1

// perform for loop unrolling for five or ten iterations
// guaranteed to do the wrong thing with any statics in body

#define unroll5(arg1, exp2, exp3, body) \
    { \
    int __fred=1; \
    arg1; \
    __unbun5(exp2, exp3, body) \
    }

#define unroll10(arg1, exp2, exp3, body) \
    { \
    int __fred=1; \
    arg1; \
    __unbun10(exp2, exp3, body) \
    }

#define __unbun10(exp2, exp3, body) \
    __unbun5(exp2, exp3, body) \
    __unbun5(exp2, exp3, body)
    
#define __unbun5(exp2, exp3, body) \
    __fred &&= (exp2); \
    if(__fred) { \
        body; \
        exp3; \
    } \
    __fred &&= (exp2); \
    if(__fred) { \
        body; \
        exp3; \
    } \
    __fred &&= (exp2); \
    if(__fred) { \
        body; \
        exp3; \
    } \
    __fred &&= (exp2); \
    if(__fred) { \
        body; \
        exp3; \
    } \
    __fred &&= (exp2); \
    if(__fred) { \
        body; \
        exp3; \
    }

#endif // __UNROLL_H

Read the .i file at your own risk.

Iluvatar is the better part of Valar.

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

Michael, that's pretty cool. Thanks!

Stu

Engineering seems to boil down to: Cheap. Fast. Good. Choose two. Sometimes choose only one.

Newbie? Be sure to read the thread Newbie? Start here!

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

Some [macro]assemblers do have directives to repeat parts of code.

GNU as appears to have this implemented as .rept, and also in a "parametrisable" version as .irp/,irpc.

(the C-hater of course thinks of no C when it comes to ultimate speed)

JW

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

This version is a bit tidier and a bit shorter:

#ifndef __UNROLL_H
#define __UNROLL_H 1

// perform for loop unrolling for five or ten iterations
// if it compiles,
// guaranteed to do the wrong thing with any statics in body

#define unroll5(arg1, exp2, exp3, body) \
    arg1; \
    do { \
    __unbun5(exp2, exp3, body) \
    } while(0) \

#define unroll10(arg1, exp2, exp3, body) \
    arg1;
    do { \
    __unbun10(exp2, exp3, body) \
    } while(0)

#define __unbun10(exp2, exp3, body) \
    __unbun5(exp2, exp3, body) \
    __unbun5(exp2, exp3, body)
    
#define __unbun5(exp2, exp3, body) \
    if(exp2) break; \
    body; \
    exp3; \
    if(exp2) break; \
    body; \
    exp3; \
    if(exp2) break; \
    body; \
    exp3; \
    if(exp2) break; \
    body; \
    exp3; \
    if(exp2) break; \
    body; \
    exp3;

#endif // __UNROLL_H

Iluvatar is the better part of Valar.