Build warning due to switch statement

Both intrinsics try to implement a specific jump-table based approach rather than the usualy chain of comparisons to the different cases.

I've never seen this error before. Is it the compiler or the linker that produces this error? Looks like the linker.

You should post your code (at least the minimum code that will produce the error/warning if compiled). Maybe there is a cross-dependency to other compiler/linker specifics in your code.

Here's a stripped down version of the code that still gives the error. Any thoughts on the error? The console output is included at the bottom.

/*
 * main.c
 */

#include "msp430x21x2.h"

//States
#define OFF					0x00
#define ENABLE_FETS			0x02
#define PWM_START			0x04
#define OP_CHECK			0x06
#define RUN					0x08
#define SHUTDOWN			0x0A
#define RESTART				0x0C

unsigned int State;

void main(void)
{
	WDTCTL = WDTPW + WDTHOLD;                 					// Stop Watchdog Timer
	if (CAL_BC1_8MHZ ==0xFF || CAL_DCO_8MHZ == 0xFF)			// Check for declaraiton of calibrated system frequencies
	{
	   	while(1);                               				// If calibration constants erased, do not load, trap CPU!!
	}
	BCSCTL1 = CALBC1_8MHZ;                   					// Set DCO to 8MHz
	DCOCTL = CALDCO_8MHZ;
	BCSCTL2 = DIVS_3; 											// SMCLk/8 from DCO (1MHz)

	__bis_SR_register(OSCOFF);									// Turn off the external oscillator

	// Configure Timer 1
	TA0CCTL0 = CCIE;               								// Timer 1 interrupt enabled
	TA0CCR0 = 10000;											// 10000 * 1us = 10ms timer
	TA0CTL = TASSEL_2 + MC_1;									// configure Timer 1 from SMCLK, upmode
	State = OFF;
	WDTCTL = WDT_MRST_32;										// Enable Watchdog on 32ms time out
	if (!(__get_SR_register() & 0x0008)) {__bis_SR_register(GIE);}
		// Check interupt status and enable

	for(;;)	{}													// Trap forever, wait for ISR
}

// Timer0_A0 interrupt service routine
#pragma vector=TIMER0_A0_VECTOR
__interrupt void Timer_A (void)
{
	switch(State)												// Go to operating state
	{
			case OFF:
			{
				__delay_cycles(1000);	//Off_State();
			}break;
			case ENABLE_FETS:
			{
				__delay_cycles(1000);	//Enable_Fets_State();
			}break;
			case PWM_START:
			{
				__delay_cycles(1000);	//PWM_Start_State();
			}break;
			/*case OP_CHECK:
			{
				__delay_cycles(1000);	//OP_Check_State();
			}break;
			case RUN:
			{
				__delay_cycles(1000);	//Run_State();
			}break;
			case SHUTDOWN:
			{
				__delay_cycles(1000);	//Shutdown_State();
			}break;
			case RESTART:
			{
				__delay_cycles(1000);	//Restart_State();
			}break;*/
			default:
			{
				_never_executed();
				break;
			}
	}/*
	//Alternative switch statement method
	switch(__even_in_range(State,4))
	{
				case OFF:
				{
					__delay_cycles(1000);	//Off_State();
				}break;
				case ENABLE_FETS:
				{
					__delay_cycles(1000);	//Enable_Fets_State();
				}break;
				case PWM_START:
				{
					__delay_cycles(1000);	//PWM_Start_State();
				}break;
				case OP_CHECK:
				{
					__delay_cycles(1000);	//OP_Check_State();
				}break;
				case RUN:
				{
					__delay_cycles(1000);	//Run_State();
				}break;
				case SHUTDOWN:
				{
					__delay_cycles(1000);	//Shutdown_State();
				}break;
				case RESTART:
				{
					__delay_cycles(1000);	//Restart_State();
				}break;
	}*/
	WDTCTL = WDTPW + WDTCNTCL;								// Reset the 16mS watchdog Timer.
}

The console output

**** Build of configuration Debug for project Switch_Test ****

C:\TI\ccsv5\utils\bin\gmake -k all 
'Building file: ../main.c'
'Invoking: MSP430 Compiler'
"C:/TI/ccsv5/tools/compiler/msp430_4.0.2/bin/cl430" -vmsp --abi=coffabi -g --include_path="C:/TI/ccsv5/ccs_base/msp430/include" --include_path="C:/TI/ccsv5/tools/compiler/msp430_4.0.2/include" --define=__MSP430F2122__ --diag_warning=225 --display_error_number --printf_support=minimal --preproc_with_compile --preproc_dependency="main.pp"  "../main.c"
"C:\Users\SWOOLA~1\AppData\Local\Temp\0664411", WARNING! at line 234:
 [W0000]
         Section .text:Timer_A already has .RETAIN specified - ignoring .CLINK
           directive

		.clink

No Assembly Errors, 1 Assembly Warning
'Finished building: ../main.c'
' '
'Building target: Switch_Test.out'
'Invoking: MSP430 Linker'
"C:/TI/ccsv5/tools/compiler/msp430_4.0.2/bin/cl430" -vmsp --abi=coffabi -g --define=__MSP430F2122__ --diag_warning=225 --display_error_number --printf_support=minimal -z -m"Switch_Test.map" --stack_size=80 --heap_size=80 -i"C:/TI/ccsv5/ccs_base/msp430/include" -i"C:/TI/ccsv5/tools/compiler/msp430_4.0.2/lib" -i"C:/TI/ccsv5/tools/compiler/msp430_4.0.2/include" --reread_libs --warn_sections --rom_model -o "Switch_Test.out"  "./main.obj" -l"libc.a" "../lnk_msp430f2122.cmd" 
<Linking>
'Finished building target: Switch_Test.out'
' '

**** Build Finished ****

I don't see why this warning comes. It refers to line 234, but your code has no line 234. Is it possible that you have two functions called Timer_A, maybe one in a library? (you don't get a double define error because your code will suppress the linking of the library function, but maybe the retain directive is fetched from both).

Trey renaming your ISR.

However...

Steve W said:

            default:
            {
                _never_executed();
                break;
            }

Having a break in a default case makes no sense. And having a break (whcih is actually a statement) after a _never_executed() intrinsic makes even less sense.
It apparently doesn't trigger an error, but maybe generates superfluous dead code.

Steve W said:

    switch(__even_in_range(State,4))

It has to read

switch(__even_in_range(State,0x0c)

personally., I suggest using an enumeration for states instead of separate defines. All named with 'xxx_yyy' where 'xxx' describest the enumeration. Such as STATE_yyy. And the last element then is STATE_END. Enumerations are word values and increase with 2 (unless explicitely specified). You can then use STATE_END in the __even_in_range intrinsic and it will automatically grow if you add or even insert more states. Even if you don't add more cases to the switch.

Thanks for the reply.

Regarding the break statement, on my stripped down version of the code removing the break doesn't cause any errors, but on the full code i get a warning 'No break at end of case' which is why it was in originally. I can't understand why it would be different.

Regarding the _even_in_range(State, 0x04) that was how I had left the code when testing the number of different options in the switch table. For State options of 0 or 2 (and the appropriate case statements commented out) the switch worked correctly with no linker errors. As soon as I increase it to 3 options (0,2 & 4) or more, the linker brought up the warning.

Does the linker warning actually matter?

Thanks

Steve

Steve W said:

Regarding the break statement, on my stripped down version of the code removing the break doesn't cause any errors, but on the full code i get a warning 'No break at end of case' which is why it was in originally. I can't understand why it would be different.

case 1:
case 2:
    code;

and do not notice that this is nothing different than

case 1:
     ; // code executed for case 1 only (no code right now); fallthrough to next case
case 2:
     ;  // code executed for Case 1 and case 2.

A 'break' is nothing else than a 'skip the rest of the switch statement' and there is no rule that forces you to use it at all.

However, your notation is a bit weird 8and supports this false understanding of the break.

You place a code block after the case and then the break. so it looks liek case and break are something like a do-while loop. This is not the case. break is a normal instruction and inlined liek any other (just that the code behind it and before the next case 'label' is unreachable)

switch(a){
  case 1:
    ...
    break;
  case 2:
    ...
    break;
}

can be also written as

if (a==1) goto CASE_1;
if (a==2) goto CASE_2;
goto CASE_END;
CASE_1:
  ...
  goto CASE_END;
CASE_2:
  ...
  goto CASE_END;
CASE_END:

Steve W said:

Does the linker warning actually matter?

Jens-Michael Gross said:

I don't see why this warning comes. It refers to line 234, but your code has no line 234.

It's an Assembly warning -- who knows what the line numbers are in the .S file.

Jens-Michael Gross said:

Having a break in a default case makes no sense.

Here I have to disagree. "default" is just like any other case -- if you don't want to /*FALLTHRU*/, you need a break.

It is correct that the Final case doesn't need a break, but leaving it out is one of those things that comes back to bite you later when you're not looking.

Jens-Michael Gross said:

And having a break (whcih is actually a statement) after a _never_executed() intrinsic makes even less sense.

Maybe -- I don't know what _never_executed() does exactly. If it's like /*NOTREACHED*/, maybe the compiler recognizes it (and the break is unnecessary) and maybe it doesn't. (and it will fuss about it not being there).

Bruce McKenney said:

It's an Assembly warning -- who knows what the line numbers are in the .S file.

Bruce McKenney said:

Having a break in a default case makes no sense.

Here I have to disagree. "default" is just like any other case -- if you don't want to /*FALLTHRU*/, you need a break.[/quote]Yes, but the default case is per definition the last case. It cannot fall-through to anywhere but out of the switch. Any break would be a 'goto next statement' or maybe a 'skip the rest of the code in default case' (if there's any code following the break). Just a big, slow NOP.

Bruce McKenney said:

And having a break (whcih is actually a statement) after a _never_executed() intrinsic makes even less sense.

Maybe -- I don't know what _never_executed() does exactly.[/quote]Well, it tells the compiler that this case is never executed. So it makes no difference what else you put there. It is per your own definition never executed. If it makes a difference in the program flow, the code is executed somehow, which contradicts the expressed intention and therefore is a coding bug. So unless there is a compiler bug, the only difference that might happen is additional, dead code in the binary.

I posted on the compiler forum as well and the .clink error has been raised as a bug by a TI employee as it shouldn't do what it is doing. The error report # is SDSCM00044118.

Jens-Michael Gross said:

Well, a line should be a line. With a good editor (e.g. UltraEdit) it shouldn't be a problem to figure out the exact error position.

Jens-Michael Gross said:

Yes, but the default case is per definition the last case. It cannot fall-through to anywhere but out of the switch. Any break would be a 'goto next statement' or maybe a 'skip the rest of the code in default case' (if there's any code following the break). Just a big, slow NOP.

Jens-Michael Gross said:

Well, it tells the compiler that this case is never executed.

Bruce McKenney said:

One just needs to find the (obscure) compiler option to save the .S file, and my guess is that (after all that) line 234 will just say ".clink" (as seen in the error message).

Bruce McKenney said:

the default case need not be the final case

Yes, I can imagine a construc tlike
switch(){
default:
  ...
case 1:
  ...
}

which menas that all but case 1 will execute teh default code first, then all including case 1 will execute the case 1 code. I'm not sure whether this will be portable.
However, if the default case is the last case in the source code, there is no code inside the switch following it and a break is superfluous in each and every case.
(s I said, it won't hurt, just waste code and time)

Bruce McKenney said:

My compiler doesn't recognize (or describe) it.

It is meant to switch teh compiler into a specific mode for generating the case checking.

Normally, a switch statement is converted into something like that:

if(case 1) goto CASE_1;
if(case 2) goto CASE_2:
CASE_1: ...
CASE_2: ...

However, if you use the 'even_in_range() intrinsic or the _never_executed, the compiler generates a jumptable approach, where the switch argument is simply added to the PC, which moves program execution into a jumptable that jumps to the proper case code. This is much faster if you know that the function argument is even and below a specific value (__even_in__range).
In case of the _never_executed intrinsic, teh compiler tries to figure out whether this jumptable approach is more efficient based on the cases explicitely described, knowing that no other cases (and therefore no default case) exists. So if you e.g. have only the cases 1,2 and 4, it can shift the argument left by one (so you get an even number) and create a jumptable with 5 entries (0 and 3 unused), rather than doing three comparisons and conditional jumps.

As all intrinsics, this is not inside the scope of the C++ language specification and usually not portable.