Array initialization in switch statement

[s4mp]

This is the weirdest bug I've ever encountered. I'm writing software for an Arduino Nano (Atmega328P) in the Arduino IDE. The problem lies in the following switch statement:

Code: [Select]

switch (s[0]) {
   case 'D':
   case 'd':
     /* Irrelevant to this problem */
     break;
   case 'P':
   case 'p':
     byte dot_map[6] = {0, 2, 3, 4, 6, 7}; // <-- This line right here
     /* Stuff */
     break;
   case 'B':
   case 'b':
     /* Irrelevant to this problem */
     break;
   case 'F':
   case 'f':
     Serial.println("F");
     asm volatile ("nop");
     asm volatile ("nop");
     break;
   default:
     Serial.println("Incorrect command");
     asm volatile ("nop");
     asm volatile ("nop");
     break;
}

The line I've marked, the declaration of the byte array, when that line is present none of the following case statement work. What's more, I've checked the output of the compiler with objdump and those cases aren't even present in the compiled program. Everything works fine when that declaration is done before the switch statement. Also, everything works when the code inside the case statement containing the line is in braces.

So, is it not allowed to declare arrays inside the case of a switch statement? Is there any other reason this would not work?

Thanks!

[golden_labels]

I do not know the Arduino language, but since it is derived from a mix of C and C++, I may guess the problem from the perspective of those languages. You have scoping problem in that line. Your dot_map array is in what scope? Imagine you read dot_map if s[0] is 'b': what will happen? It is not even initialized there.

tl;dr: never declare variables in switch statements, unless you explicitly introduce another scope for them.

[s4mp]

I understand that, that is why the braces solve the issue. It's just very odd that the compiler decides to solve the issue by completely ignoring the following case statements without giving any warning or error. I was wondering if there's an underlying reason that I don't know about.

[jonroger]

I agree, odd.    It doesn't compile with linux gcc.

Edit: but it does compile if you put ";" before "byte dot_map".   And it runs correctly.  dot_map is in scope in cases below where it is declared.  As one would expect if the switch were replaced with gotos.   Summary - just a pre-processor or compiler bug.

[T3sl4co1l]

The keyword is "hoisting": declarations are moved to the top of whichever scope they are in.  It may be that it doesn't work with switch/case and the compiler starts reading garbage.  It should be an error in C (which does not perform hoisting..?).

Tim

[golden_labels]

the compiler decides to solve the issue by completely ignoring the following case statements without giving any warning or error. I was wondering if there's an underlying reason that I don't know about.

Since I do not know Arduino language’s specs, I may only guess, but the general explanation in similar scenarios is: invoking undefined behaviour. If an UB happens in a program, in most languages the program is allowed to do whatever. In C and C++ specifically, the compiler is allowed to produce code that does literally anything, including setting your device on fire. Heavily optimizing C and C++ compilers do care about portions of code that have undefined behaviour, applying optimizations in a way that may have a side effect of not producing some code or not being concerned with some edge cases. It’s much easier to not care about code that is inherently invalid anyway.

And, despite some well known kernel developer constantly rants about the behaviour, it makes a lot of sense. Take that C code as an example:

Code: [Select]

uint8_t square(uint8_t const a) {
    return a * a;
}On AVR8 it invokes undefined behaviour because of the signed overflow. But avr-gcc simply compiles it to:

Code: [Select]

   0: 88 9f        mul r24, r24
   2: 80 2d        mov r24, r0
   4: 11 24        eor r1, r1
   6: 08 95        ret
With the whole actual multiplication being in the first line (the rest is just to handle the function call). Imagine what would happen, if avr-gcc would concern itself with dealing with that undefined behaviour. What should it do? Write unrebliable compile-time tests that would warn the user about potential UB? Should it include expensive runtime checks to abort execution if UB occurs? How should it abort it? How should it do interprocedural optimization to avoid those tests when UB obviously isn’t possible? Nah… they simply ignore it and create code that ignores the UB portion of the code.

[radiogeek381]

The compiler is misbehaving for not flagging this as an error.

1. Declaration of a variable or array is not a "statement"  -- it is a declaration.  The thingie that follows a case label must be a statement.

• Note that a block { foo; } *is* a statement.
  

2. If declaring a variable inside a case statement was even possible, what would it mean?

• Is the variable visible outside the switch/case statement?
• If so, what happens if someone attempts to use it when the creating case doesn't get chosen?

F'rinstance, what should happen here?

Code: [Select]

int foo(int a) {
  switch (a) {
    case 0:
      int ary[2] = {0, 1};
      break;
    default:
      break;
  }

  return ary[1] + ary[0];
}

int main() {
  foo(3);
}
When we get to the return, ary has never been declared.  What should the program actually do? 

Now if we do this:

Code: [Select]

int foo(int a) {
  switch (a) {
    case 0: {
      int ary[2] = {0, 1};
      break;
    }
    default:
      break;
  }
 
  return ary[0] + ary[1];
}
we are still broken, since ary has "gone out of scope" by the time we get to the return statement.  (It only existed within the enclosing { } braces.)

In general, declarations within case blocks must be in a scope that is limited to that case.  If the language provided otherwise, it could not determine at compile time whether a later reference (outside the case) to the declared variable was legitimate, as it is possible that the variable would not get created at all.

The example you provide doesn't indicate what the original intent was.  If you need the array, then it should be declared outside the switch statement. 

Gcc 8.3 flags the original code with an unfortunately all-too-characteristically cryptic message.  But it does flag it as an error.

[SiliconWizard]

When I've needed local variables in switch/case parts, I have always used braces to create a new block with a new scope. I would have never thought of writing what you posted, because declaring a variable anywhere except at the top of the current scope was not even allowed before C99.

With C99, you can declare variables almost everywhere like in C++, but I still tend to avoid doing this, just for a question of style. It always looks a bit sloppy to me. And the "everywhere" excludes the construct you posted.

The switch/case construct is a weird beast in C, and many weird things can be written with them (which I don't recommend). But a declaration inside a 'case' is not one of them.

GCC gives the following error: "error: a label can only be part of a statement and a declaration is not a statement".

Now Arduino actually uses C++, and not C. (And I'm not sure what compiler they use actually. Is it GCC? Is it based on GCC? What is it? Probably not GCC, as this piece of code would have never compiled.)
I compiled the above code in C++ mode with GCC, and the error is slightly different, although it basically means the same thing: "error: jump to case label case 'B': note:   crosses initialization of 'byte dot_map [6]'". A bit more evocative than the C error IMO.

So as you figured, the only way of doing this would be to enclose the declaration with braces. Obviously it will only have this local scope then, but as others have pointed out, any other scope would have been very weird here anyway.

[IanB]

So, is it not allowed to declare arrays inside the case of a switch statement? Is there any other reason this would not work?

The question to answer (for yourself) is: what is the scope of the thing you are declaring, dot_map?

A switch statement controls flow of execution, and a declaration is not an executable statement, so it doesn't belong where you have put it.

If you want the scope to be just inside case 'p', then you should enclose the code for that branch of the switch statement in braces to create a block. You can then declare local variables inside the block that will exist only inside that block.

[I wanted a rude username]

The switch/case construct is a weird beast in C, and many weird things can be written with them (which I don't recommend).

Behold Duff's device, a do ... while interleaved with a switch:

Code: [Select]

   register n = (count + 7) / 8;      /* count > 0 assumed */

   switch (count % 8)
   {
   case 0:        do {  *to = *from++;
   case 7:              *to = *from++;
   case 6:              *to = *from++;
   case 5:              *to = *from++;
   case 4:              *to = *from++;
   case 3:              *to = *from++;
   case 2:              *to = *from++;
   case 1:              *to = *from++;
                      } while (--n > 0);
   }


C's default fall through in case statements has long been its most controversial single feature; Duff observed that “This code forms some sort of argument in that debate, but I'm not sure whether it's for or against.”

[oPossum]

Now Arduino actually uses C++, and not C. (And I'm not sure what compiler they use actually. Is it GCC? Is it based on GCC? What is it? Probably not GCC, as this piece of code would have never compiled.

Arduino uses GCC, but the code is run through the 'code mangler' before the compiler.

[T3sl4co1l]

In GCC, you can also take the address of labels and, for example, write your own jump table.  I tried this recently, just for S&Gs, but it ended up taking more space than the switch-case version of the same code.  Possibly because the optimizer can't reason about the labels so doesn't try very hard.  (I didn't look too closely at the assembled output, to tell where the major differences are.)

Tim

[SiliconWizard]

Now Arduino actually uses C++, and not C. (And I'm not sure what compiler they use actually. Is it GCC? Is it based on GCC? What is it? Probably not GCC, as this piece of code would have never compiled.

Arduino uses GCC, but the code is run through the 'code mangler' before the compiler.

I don't know what the code mangler is, and don't know how it could transform the above piece of code into something GCC could compile. A more in-depth view of what it does would be "interesting" (or not.)

[s4mp]

The example you provide doesn't indicate what the original intent was.  If you need the array, then it should be declared outside the switch statement. 

The original intent was to only use the variable within the same case block it was declared in. I was aware a switch creates it's own scope, so any variable declared there couldn't be used outside of it.

If I understood it correctly now, I think this best shows how what I wrote can create undefined behaviour:

Code: [Select]

switch(a)
{
case 1:
case 2: int i = 5;
case 3: print(i);
}
the 3th case needs i, which is declared in the same scope, but wont always be declared, right?

GCC gives the following error: "error: a label can only be part of a statement and a declaration is not a statement".

If it's an error for the gcc compiler, it would be nice that any intermediate steps (code manglers) would also consider it an error. I get that formally out of spec code can generate undefined behaviour, but still, an error or at least a warning would have been nice.

[artag]

I don't know what the code mangler is, and don't know how it could transform the above piece of code into something GCC could compile. A more in-depth view of what it does would be "interesting" (or not.)

It's an arduino-specific stage that tries to simplify some C/C++ requirements to help the assumed non-expert user. The most obvious of these is the reduced need for function declarations - the mangler collects them all before compilation and emits them at the start so explicit forward declarations aren't required.

A less useful effect is to damage the semantics of typedefs such that you can't typedef a struct that was declared in the same statement and have to perform some sort of odd workaround involving putting it into an include file. It also makes diagnostic warnings less useful - although it tries to force them into context using #line it sometimes gets that wrong (most often by identifying the wrong file, i think).

I don't think the code mangler has a proper grammar as such - it's just a text-driven preprocessor. So it probably unpicks that unexpected syntax, makes some incorrect assumptions and emits something that compiles but doesn't do what you expected.

I think there's a way to set the compilation flags to show the intermediate mangled code though I forget the details. It would probably be worth looking to see what gcc actually gets given as a result of this.
 

[SiliconWizard]

The example you provide doesn't indicate what the original intent was.  If you need the array, then it should be declared outside the switch statement. 

The original intent was to only use the variable within the same case block it was declared in. I was aware a switch creates it's own scope, so any variable declared there couldn't be used outside of it.

As I said, the only correct way of doing this is the following:

Code: [Select]

switch  (xxx)
{
case yyy:
    {
         int i = 5;
         (...)
    }
    break;


}


[SiliconWizard]

The switch/case construct is a weird beast in C, and many weird things can be written with them (which I don't recommend).

Behold Duff's device, a do ... while interleaved with a switch:

Ahah, exactly. I have seen even weirder stuff.

The main thing to understand is that the "case xxx:" are just to be seen as labels  (which explains the somewhat cryptic error message from GCC.)

Interestingly, the code below exhibits the same problem, but would you have guessed?

Code: [Select]

if ((s[0] == 'P') || (s[0] == 'p'))
    goto P;

// ...

P:
byte dot_map[6] = {0, 2, 3, 4, 6, 7};


You get the exact same error message from GCC. Which is not surprising per se.
But why is that so? Basically "labels" in C are very finicky. Like, very. If you just add a ';' after the label, suddenly  it becomes correct:

Code: [Select]

P:     ;
byte dot_map[6] = {0, 2, 3, 4, 6, 7};


Labels expect statements (even empty statements) right after them, and nothing else. Not even a variable declaration, whereas in C99 you can put them pretty much anywhere you like. Not very consistent if you ask me.

Now with this little "experiment" done, just add a ',' right after the case, and before the variable declaration. Unsurprisingly, it suddenly compiles fine as well.

Code: [Select]

switch (s[0]) {
   case 'D':
   case 'd':
     /* Irrelevant to this problem */
     break;
   case 'P':
   case 'p': ;
     byte dot_map[6] = {0, 2, 3, 4, 6, 7}; // <-- This line right here
     /* Stuff */
     break;

}

Note that in this post, I'm strictly talking about C.
The above code will still not compile with GCC in C++ mode (but the 'goto' label one will.) Sorry, but whereas peeking at the C std is a matter of a couple minutes, I won't do it with the C++ std unless I have a whole week available

[s4mp]

I did a little more digging, for anyone interested, here's what I found.

The arduino IDE does call a pre-processor on the code. It converts the .ino files used by the arduino IDE into .cpp files compatible with the avr-g++ compiler. The compile logs point to where this .cpp file is located. Looking at it with diff, only two functional things are added:

• An include for <arduino.h> is added.
• As artag said, it adds function declarations.
• (Comments are added to make references to line numbers in the original file possible)

So, if this code is going to the compiler without many changes, and the compiler is basically gcc, why doesn't it throw errors as expected? It has the -fpermissive set which tells it not to call these mistakes an error, just a warning (which the IDE doesn't show by default). Seems like a bad idea, looking at the commit (https://github.com/arduino/Arduino/commit/ba0c41b) where this flag was introduced, they added this to "avoid build errors on old libraries". I can't say I agree that's a good solution, but hey, now we know. It's possible to set the compiler warnings to all in the settings. That way, the type of mistake I made is at least shown as a warning.

[T3sl4co1l]

Arduino is kind of the embedded lightweight equivalent of PHP.  It's peculiar and buggy, and it should be no surprise that -fpermissive and hidden warnings are normal.

The pros know better and write C/++ with the mfg lib, and copy in code as needed (e.g., the LCD routines for Arduino are easy to find, and not terrible to interface, just heavy-weight because of all the compatibility and flexibility junk that makes Arduino work at all).

Welcome to the pros!

Tim

[cliffyk]

OP, after adding setup() and loop() functions and defining the s[] array I got your code to compile using the Arduino IDE, however it threw a number of compiler warnings (do you have your compiler preference set for "verbose output"?:



Then, using a different IDE (UECIDE, same code);I could not even get it to compile, throwing numerous errors:



However I found that by placing the "case 'p'"statements in braces it would compile without errors on UECIDE.



I next defined an s[] array bsed on your "case" values and wrapped your switch statement in a "for (i=0; i<=7;i++)" loop to make it work--and added Serial.println() commands to each 'case".  Here it that code

Code: [Select]

char s[] = {'d', 'D', 'P',  'p','B', 'b', 'f', 'F'};

void setup()
{
// put your setup code here, to run once:
Serial.begin(115200);
}

void loop()
{
// put your main code here, to run repeatedly:

Serial.println("begin 'for'");
for (int i = 0; i <= 7; i++)
{
Serial.print("i = ");
Serial.print(i);

Serial.print(" : s[i]");
Serial.print(" = ");
Serial.print(s[i]);
Serial.print(" ---> ");

switch (s[i]){
case 'D':
Serial.println("case 'D'");
case 'd':
Serial.println("case 'd'");
/* Irrelevant to this problem */
break;
case 'P':
Serial.println("case 'P'");
case 'p':{ // added brace
Serial.println("case 'p'");
byte dot_map[6] = {0, 2, 3, 4, 6, 7}; // <-- This line right here
/* Stuff */
break;} // added brace
case 'B':
Serial.println("case 'B'");
case 'b':
Serial.println("case 'b'");
/* Irrelevant to this problem */
break;
case 'F':
Serial.println("case 'F'");

case 'f':
Serial.println("case 'f'");
asm volatile("nop");
asm volatile("nop");
break;
default:
Serial.println("Incorrect command");
asm volatile("nop");
asm volatile("nop");
break;
}
}

while (true) {}
}

Here is the Serial output of that code:



Notice that the 'D' case just fell through to the 'd' case, the 'P' case passed right through to 'p', and 'B' to 'b', and 'F' to 'f' inside the "for" loop.

Cool says I, next I added braces to all the case statements:

Code: [Select]

char s[] = {'d', 'D', 'P',  'p','B', 'b', 'f', 'F'};

void setup()
{
// put your setup code here, to run once:
Serial.begin(115200);
}

void loop()
{
// put your main code here, to run repeatedly:

Serial.println("begin 'for'");
for (int i = 0; i <= 7; i++)
{
Serial.print("i = ");
Serial.print(i);

Serial.print(" : s[i]");
Serial.print(" = ");
Serial.print(s[i]);
Serial.print(" ---> ");

switch (s[i]){
case 'D':{
Serial.println("case 'D'");}
case 'd':{
Serial.println("case 'd'");
/* Irrelevant to this problem */
break;}
case 'P':{
Serial.println("case 'P'");}
case 'p':{ // added brace
Serial.println("case 'p'");
byte dot_map[6] = {0, 2, 3, 4, 6, 7}; // <-- This line right here
/* Stuff */
break;} // added brace
case 'B':{
Serial.println("case 'B'");}
case 'b':{
Serial.println("case 'b'");
/* Irrelevant to this problem */
break;}
case 'F':{
Serial.println("case 'F'");
}
case 'f':{
Serial.println("case 'f'");
asm volatile("nop");
asm volatile("nop");
break;}
default:{
Serial.println("Incorrect command");
asm volatile("nop");
asm volatile("nop");
break;}
}
}

while (true) {}
}

I knew that would not change anything, but thought it made a nice "next step":



OK, now let's fix it up; add "break;" statements to all "cases" :

Code: [Select]

char s[] = {'d', 'D', 'P',  'p','B', 'b', 'f', 'F'};

void setup()
{
// put your setup code here, to run once:
Serial.begin(115200);
}

void loop()
{
// put your main code here, to run repeatedly:

Serial.println("begin 'for'");
for (int i = 0; i <= 7; i++)
{
Serial.print("i = ");
Serial.print(i);

Serial.print(" : s[i]");
Serial.print(" = ");
Serial.print(s[i]);
Serial.print(" ---> ");

switch (s[i])
{
case 'D':
{
Serial.println("case 'D'");
break;
}
case 'd':
{
Serial.println("case' d'");
/* Irrelevant to this problem */
break;
}
case 'P':
{
Serial.println("case' 'P'");
break;
}
case 'p':
{
Serial.println("case 'p'");
byte dot_map[6] = {0, 2, 3, 4, 6, 7};
/* Stuff */
break;
}
case 'B':
{
Serial.println("case' 'B'");
break;
}
case 'b':
{
Serial.println("case' 'b'");

/* Irrelevant to this problem */
break;
}
case 'F':
{
Serial.println("case' 'F'");
break;
}
case 'f':
{
Serial.println("case''f'");
asm volatile("nop");
asm volatile("nop");
break;
}
default:
{
Serial.println("Incorrect command");
asm volatile("nop");
asm volatile("nop");
break;
}
}
delay(500);
}
while (true) {};
}


Now we're cooking with gas!:



Each "case", and just that "case" executed once in the for loop:

UECIDE has a nice "Autoformat with Artistic Style" plugin which I use all the time to make code more readable.