This code shouldn't work, yet it does - how?

[HwAoRrDk]

So there I was, reviewing a piece of code I wrote a while ago, when I noticed that there was a bug in it. "Wait a minute", says I, "how is this code working? It shouldn't be!".

The code in question, which is for an ATmega32M1:

Code: [Select]

typedef enum {
CAN_BUS_SPEED_100,
CAN_BUS_SPEED_125,
CAN_BUS_SPEED_200,
CAN_BUS_SPEED_250,
CAN_BUS_SPEED_500
} can_bus_speed_enum_t;

typedef struct {
can_bus_speed_enum_t bus_speed;
struct {
uint8_t canbt1;
uint8_t canbt2;
uint8_t canbt3;
} reg_vals;
} can_bt_reg_t;

typedef struct {
struct {
can_bus_speed_enum_t bus_speed;
} can;
/* other members omitted for brevity */
} config_t;

config_t config = {
{
CAN_BUS_SPEED_500
}
};

static const can_bt_reg_t can_bus_speed_bt_conf[] PROGMEM = {
{ CAN_BUS_SPEED_100, { 0x08, 0x0C, 0x37 } },
{ CAN_BUS_SPEED_125, { 0x06, 0x0C, 0x37 } },
{ CAN_BUS_SPEED_200, { 0x02, 0x0E, 0x4B } },
{ CAN_BUS_SPEED_250, { 0x02, 0x0C, 0x37 } },
{ CAN_BUS_SPEED_500, { 0x00, 0x0C, 0x36 } }
};

void init_can() {
for(size_t i = 0; i < (sizeof(can_bus_speed_bt_conf) / sizeof(can_bus_speed_bt_conf[0])); i++) {
if(config.can.bus_speed == can_bus_speed_bt_conf[i].bus_speed) {
CANBT1 = pgm_read_byte(&can_bus_speed_bt_conf[i].reg_vals.canbt1);
CANBT2 = pgm_read_byte(&can_bus_speed_bt_conf[i].reg_vals.canbt2);
CANBT3 = pgm_read_byte(&can_bus_speed_bt_conf[i].reg_vals.canbt3);
break;
}
}

if(CANBT1 == 0 && CANBT2 == 0 && CANBT3 == 0) return;

/* rest of CAN peripheral initialisation... */
}

int main(void) {
init_can();
/* etc... */
}

Spotted the bug? It's in this line within init_can():

Code: [Select]

if(config.can.bus_speed == can_bus_speed_bt_conf[i].bus_speed) {

Yes, I forgot to surround the access of can_bus_speed_bt_conf[​i].bus_speed with a pgm_read_byte() call, necessitated due to that variable being declared as PROGMEM. So, it shouldn't work properly, because without that, the comparison will not be against the correct value. What I think should happen is that some random value - whatever resides at the same address in RAM as where the variable actually lives in flash - will be used instead, and the comparison will fail at every loop iteration; this will leave the CANBTn registers uninitialised, and so the later test for that (registers having the default value of zero) will be true, and the function will exit prematurely, leaving the CAN peripheral unconfigured and still in the standby state.

However, the bizarre thing is, this code works! In my testing so far, the code appears to successfully set up the correct register values (or, at least, some kind of workable values) for 500kbps operation, configures the rest of the CAN peripheral, and I am receiving stuff from the bus perfectly fine. I just don't understand how...

The only reason I can think of for this to work is by accident or happenstance: that when it reads can_bus_speed_bt_conf[​i].bus_speed it's looking at a RAM area with a value of 0x04, which matches the CAN_BUS_SPEED_500 enum member's value. If the value in memory was something else that happened to match any other enum value, yes it would initialise the registers, but for the wrong CAN bus speed, and I wouldn't have a working system - which isn't the case.

So, why does this code work? How can a PROGMEM variable accessed without using a pgm_read_* function yield the correct value still?

[andersm]

GCC has supported named address spaces for some years now. If your environment is set up to use that support when available (ie. the PROGMEM macro resolves to __flash, and not the progmem attribute), then the pgm_read accessors aren't needed.

For the ultimate truth, examine the disassembly.

[HwAoRrDk]

GCC has supported named address spaces for some years now. If your environment is set up to use that support when available (ie. the PROGMEM macro resolves to __flash, and not the progmem attribute), then the pgm_read accessors aren't needed.

The PROGMEM macro resolves to __attribute__((__progmem__)), so the named address spaces stuff definitely doesn't apply in this case.

I hadn't actually heard of the named address spaces feature, seems interesting, so thanks for pointing that out.

For the ultimate truth, examine the disassembly.

I don't really know what I'm doing with regard to AVR assembler, but if I check what the compiler generated, what I can see is that there are only three instances of the LPM instruction in the entire can_init() function, which corresponds to the three instances of pgm_read_byte() I was actually using. So I would guess it is safe to assume that the code can't be accessing flash in the aforementioned offending line of code.

[HwAoRrDk]

I thought I would try and get some more detailed idea of what was going on with the code during execution by running it in a debugger. But I don't have an ICE, so the Atmel Studio simulator will have to do. But there is no simulator for the ATmega32M1 (), so what I did instead was copy the relevant sections of code into a temporary project targeting an ATmega328P, and replace the CAN register references (CANBT1, etc.) with some volatile uint8_t variables.

When I stepped through the code, it did appear that without a call to pgm_read_byte, it was accessing RAM. The structure of memory in this temp test code was such that can_bus_speed_bt_conf had an address of 0x00A8, and looking at that region showed it contained a run of zeros longer than the size of the variable; thus, the resulting value read got interpreted as CAN_BUS_SPEED_100, the first enum member, on every loop iteration.

So, is my theory about the code accidentally working correct? By a matching value just happening to be in the appropriate location in RAM? I don't see any other way.

Is there any way of finding out, without the aid of a debugger, whether this is what's happening?

[ale500]

Atmel studio used to support a simulator, did you try that ?
You can disassemble the object files using objdump -d name_of_the_object

[ajb]

When I stepped through the code, it did appear that without a call to pgm_read_byte, it was accessing RAM. The structure of memory in this temp test code was such that can_bus_speed_bt_conf had an address of 0x00A8, and looking at that region showed it contained a run of zeros longer than the size of the variable; thus, the resulting value read got interpreted as CAN_BUS_SPEED_100, the first enum member, on every loop iteration.

So, is my theory about the code accidentally working correct? By a matching value just happening to be in the appropriate location in RAM? I don't see any other way.

Seems like that's exactly what's happening.  So your title is a bit inaccurate: the code just happens to produce correct results for certain cases, but this not at all the same as actually working!

Is there any way of finding out, without the aid of a debugger, whether this is what's happening?

Simulate it, as you are doing, or step through the disassembly manually and compare to the map file. 

Side note:  since your CAN config data is fixed, you could avoid the loop entirely by simply using your enum to index into the config array.  Since this is an init function it probably has almost no impact on real performance, but it would be a more elegant solution.  Your method would be preferable if you were indexing on a non-contiguous value (like looking up a numeric baud rate in a list of fixed options, for example), but in the code shown there's no reason to not make the enum values contiguous and use them as indices.  (Unless otherwise defined, enum values will be contiguous starting from zero per the C spec, but whenever the exact value is important it's always a good idea to declare them explicitly to make your intentions clear.) 

[SiliconWizard]

If the compiler is GCC-based, use the '-S' option to generate assembly for this C source file, and see what it actually compiles to.

[T3sl4co1l]

It appears the for() loop is static, i.e., wholly known at compile time.  Is it being unrolled or hard-wired?

I don't know offhand if GCC is quite this smart/aggressive, depending on optimization level.

Speaking of, you didn't mention what optimization level if any you're using.

But in any case, the given code should indeed access RAM, and coincidences can happen.  Just because it's a bug doesn't mean it's an automatic crash!

Tim

[cv007]

./avr-objdump -d -j .data my.elf
will show the data section and initial values (which could change by the time the can init runs)
./avr-objdump -t my.elf
will show all symbols, where you should be able to find the 'can_bus_speed_bt_conf' symbol and its address

(and there are most likely another dozen variations that can get the info)

probably have to work out the array offsets to bus_speed first, then see what variables in .data match up- or maybe you end up in the bss section and are looking at 0's so get the first array matched

[richardman]

Unlikely to work "by accident".

I don't know GCCAVR for sure, but with our own AVR compiler, if a named object has the __flash attribute, it will generate the LPM instructions. I would imagine that a later release of GCC might have done the same. I know the original version didn't, but it's not a difficult feature to add.

[HwAoRrDk]

Side note:  since your CAN config data is fixed, you could avoid the loop entirely by simply using your enum to index into the config array.

I did actually start out with the code like that, but I specifically changed it to how it is now because I wanted to make it more explicit - you know, so it's obvious that "these are the register values for CAN_BUS_SPEED_500", etc. - rather than leaving it implied or only noted with a comment. Like you say, it's only within initialisation code - it only ever runs at start-up and waking from sleep - so I don't particularly care that its not the most efficient or condensed code.

It appears the for() loop is static, i.e., wholly known at compile time.  Is it being unrolled or hard-wired?

It appears not to be unrolled. As I said earlier, the disassembly of the function only contains three LPM instructions (matching the three pgm_read_byte() calls), whereas if it was unrolled I would expect 15 (3x5). What impact would an unrolled loop have on the situation?

Speaking of, you didn't mention what optimization level if any you're using.

As far as I recall, Atmel Studio defaults to -O1 for Debug level compilation, which is what I was using. I believe it's -Os (or maybe -O2?) for the Release configuration, but I've so far yet to compile this code with that.

./avr-objdump -d -j .data my.elf
will show the data section and initial values (which could change by the time the can init runs)
./avr-objdump -t my.elf
will show all symbols, where you should be able to find the 'can_bus_speed_bt_conf' symbol and its address

I had a look at this, but I've practically no idea what I'm looking at (or for).

The latter command includes this line of output with reference to that variable:

Code: [Select]

0000009c l     O .text 00000014 can_bus_speed_bt_conf

I'm clueless about what the output of the former is representing.

By the way, Atmel Studio by default outputs with compilation not only an objdump disassembler listing (-S output, I believe) but also a .map file with a ton of information in it, if that is any help.

[westfw]

Likely an accident.
However, avr-gcc WILL highly optimize access to PROGMEM structures at compile time if you forget the pgm_get_xxx() call.It's an occasionally cause of apparently mysterious bugs where, ie

Code: [Select]

Serial.print(foo[0].menu);  // print top-level menu value - works fine.
Serial.print(foo[readDecimal()].menu); // doesn't work, even when 0 is entered
(where foo is some array in PROGMEM.)You can actually use this to implement "digitalWriteFast()" in the Arduino World, for boards that do link-time optimization, since the "pin" to port/bit tables are constant arrays in progmem:

Code: [Select]

// Compiles to SBI/CBI when possible!static inline __attribute__((always_inline)) void _dwfast(int pin, int val) {
    uint8_t mask = digital_pin_to_bit_mask_PGM[pin];
    uint8_t port = digital_pin_to_port_PGM[pin];
    volatile uint8_t *outport = (volatile uint8_t*)port_to_output_PGM[port];
    if (digital_pin_to_bit_mask_PGM[0] == 0x99)  // Never true
        LTO_Not_Enabled();
    if (val) {
        *outport |= mask;
    } else {
        *outport &= ~mask;
    }
}

[T3sl4co1l]

It appears not to be unrolled. As I said earlier, the disassembly of the function only contains three LPM instructions (matching the three pgm_read_byte() calls), whereas if it was unrolled I would expect 15 (3x5). What impact would an unrolled loop have on the situation?

No, I mean unrolled as in, instead of doing a loop that does known things each time, it just goes and does those things!  There would be no LPM because that value is also known at compile time.

In this case, it's looping over an enum, and the comparison target is static, so it really only needs to ask one thing:

Code: [Select]

if(config.can.bus_speed == CAN_BUS_SPEED_500) /* true */ { ... }
I may be making more assumptions about config.can.bus_speed than the compiler would actually take, but this process is entirely reasonable if so.

So to really understand what's going on, learn the instruction set and follow the execution path.  Failing that, you might look at the listing for different optimization levels and see which statements are broken up the most.

Tim

[EEVblog]

However, the bizarre thing is, this code works! In my testing so far, the code appears to successfully set up the correct register values (or, at least, some kind of workable values) for 500kbps operation, configures the rest of the CAN peripheral, and I am receiving stuff from the bus perfectly fine. I just don't understand how...

Power-on default register values perhaps?

[cv007]

You can work your way through this simple example to find where vars are located-

//======================================
//main.c
unsigned char mybss = 0;
unsigned char mydata1 = 0xaa;
unsigned char mydata2 = 0xbb;
__attribute__((__progmem__)) const unsigned char mypgm = 0xcc;

int main(){
  for( ; ; ){
    mybss = mydata1;
    mydata1 = mydata2;
    mydata2 = mypgm;
  }
}

//======================================

> ./avr-gcc main.c
>./avr-objdump -d -j .data a.out

a.out:     file format elf32-avr


Disassembly of section .data:

00800060 <mydata1>:
  800060:       aa                                                  .

00800061 <mydata2>:
  800061:       bb 

// mydata1 is at data address 0x60 and is init to 0xaa
// mydata2 is at data address 0x60 and is init to 0xbb

//======================================

>./avr-objdump -d -j .bss a.out

a.out:     file format elf32-avr

Disassembly of section .bss:

00800062 <mybss>:
        ...

//mybss is at 0x62 (init to 0 as its in bss)

//======================================

> ./avr-objdump -t a.out
//find mypgm
00000068 g     O _f73ce1085c30baaa      00000001 mypgm


> ./avr-objdump -d -j _f73ce1085c30baaa a.out
//I'm just using bare avr bin stuff, so seem to get
//this section name for a progmem, it maybe normal

a.out:     file format elf32-avr


Disassembly of section _f73ce1085c30baaa:

00000068 <mypgm>:
  68:   cc

//mypgm is at address 0x68 and is set to 0xcc

//======================================

> ./avr-objdump -d -j .text a.out

// main cut out from listing
0000006a <main>:
  6a:   cf 93           push    r28
  6c:   df 93           push    r29
  6e:   cd b7           in      r28, 0x3d       ; 61
  70:   de b7           in      r29, 0x3e       ; 62
  72:   80 91 60 00     lds     r24, 0x0060     ; 0x800060 <_edata>
  76:   80 93 62 00     sts     0x0062, r24     ; 0x800062 <mybss>
  7a:   80 91 61 00     lds     r24, 0x0061     ; 0x800061 <mydata2>
  7e:   80 93 60 00     sts     0x0060, r24     ; 0x800060 <_edata>
  82:   8c ec           ldi     r24, 0xCC       ; 204
  84:   80 93 61 00     sts     0x0061, r24     ; 0x800061 <mydata2>
  88:   f4 cf           rjmp    .-24            ; 0x72 <main+0x8>

//======================================


decoding main-
r24 = mydata1 (0x60)
0x62 (mybss) = r24 (mydata1)
r24 = mydata2 (0x61)
mydata1 (0x60) = r24 (mydata2)

r24 =0xCC <<-- compiler knew what mypgm const value was and simply loaded it from immediate
0x61 (mydata2) = r24 (0xCC) <<--and did not try to get from 0x68

and you can see in the end, the compiler did not incorrectly get the wrong data for the mypgm var as it used the known value instead of (incorrectly) loading from its address (which is actually a .text address).

Doesn't sound like this is happening to you, but you would have to actually see the disassembled code to know (I wouldn't take those present lpm instructions as proof of anything, I would want to figure out exactly what is happening).

What I am trying to suggest, is that you can know what vars are at what address, and even without debugging you could see which vars are being accessed incorrectly if the compiler was trying to read a var in progmem without the progmem read function (reading from data). You could then deduce what is actually being loaded incorrectly (although the data vars could change before that, you would know which vars to look for in your code and could figure out if they change prior to being accessed).

Power-on default register values perhaps?

Are all 0's for these 3 registers, which could probably be an explanation as the first is probably most important and is 0 for his desired value, but he says he checks all 3 registers for 0 before trying to further init and exits init if all 3 are 0;

Those unintended reads to data could also end up after the bss section, where a memory value could be anything, and will most likely be different at every power on. That would be fun.

[HwAoRrDk]

Thanks to cv007 for the great example showing how the different ways variables are defined relate to how they end up in different sections of the binary. Made me understand a bit more about what the .data, .bss and .text sections are about.

After sitting down and poring over the disassembly of my can_init() function, I think T3sl4co1l might be on to something: that the compiler might have optimised away the accesses to can_bus_speed_bt_conf.

Here is the section of code of interest:

Code: [Select]

     2ee: 80 91 64 01 lds r24, 0x0164 ; 0x800164 <__data_start+0x64>
     2f2: 88 23        and r24, r24
     2f4: a1 f0        breq .+40      ; 0x31e <can_init+0x6a>
     2f6: 81 30        cpi r24, 0x01 ; 1
     2f8: 49 f0        breq .+18      ; 0x30c <can_init+0x58>
     2fa: 82 30        cpi r24, 0x02 ; 2
     2fc: 51 f0        breq .+20      ; 0x312 <can_init+0x5e>
     2fe: 83 30        cpi r24, 0x03 ; 3
     300: 59 f0        breq .+22      ; 0x318 <can_init+0x64>
     302: 84 30        cpi r24, 0x04 ; 4
     304: 21 f5        brne .+72      ; 0x34e <can_init+0x9a>

It starts off with what I believe is loading the value of config.can.bus_speed into r24. Then there is a logical-AND-ing of r24 with itself, which I'm not sure of the reason for. After that, it is doing a sequence of copying a constant value to r24 and then branching if r24 is equal to something.

The constant values immediately jump out at me as being the exact implicit values the can_bus_speed_enum_t members will have! So, what I think has happened here is that the for-loop has been unrolled, and the compiler has 'hard-coded' the values of can_bus_speed_bt_conf[​i].bus_speed!

The AND instruction still puzzles me though. Although, because it's also followed by a branch instruction, I guess it's somehow the test for the first enum member (which has a value of zero)?

Also puzzling is the locations the branch instructions jump to. Because config.can.bus_speed is set to 500kbps, i.e. a value of 4, I would assume that last branch to 0x34e would be the path chosen. However, that goes straight to a piece of code that appears to be the CANBT1 == 0, etc. test (some loading of those registers and comparing/skipping). That doesn't make any sense, as it would be omitting the loading and assignment of the registers altogether! Maybe that branch test is never true? Kind of makes sense, as if that path is not taken, it drops straight into some code that again loads a constant 16-bit value of 4 into two adjacent registers (r24 & r25) and then jumps into the code that features the three LPM instructions which sets the CANBTn registers.

Here's the whole section of assembler for reference, in case anyone has a different interpretation:

Code: [Select]

     2ee: 80 91 64 01 lds r24, 0x0164 ; 0x800164 <__data_start+0x64>
     2f2: 88 23        and r24, r24
     2f4: a1 f0        breq .+40      ; 0x31e <can_init+0x6a>
     2f6: 81 30        cpi r24, 0x01 ; 1
     2f8: 49 f0        breq .+18      ; 0x30c <can_init+0x58>
     2fa: 82 30        cpi r24, 0x02 ; 2
     2fc: 51 f0        breq .+20      ; 0x312 <can_init+0x5e>
     2fe: 83 30        cpi r24, 0x03 ; 3
     300: 59 f0        breq .+22      ; 0x318 <can_init+0x64>
     302: 84 30        cpi r24, 0x04 ; 4
     304: 21 f5        brne .+72      ; 0x34e <can_init+0x9a>
     306: 84 e0        ldi r24, 0x04 ; 4
     308: 90 e0        ldi r25, 0x00 ; 0
     30a: 0b c0        rjmp .+22      ; 0x322 <can_init+0x6e>
     30c: 81 e0        ldi r24, 0x01 ; 1
     30e: 90 e0        ldi r25, 0x00 ; 0
     310: 08 c0        rjmp .+16      ; 0x322 <can_init+0x6e>
     312: 82 e0        ldi r24, 0x02 ; 2
     314: 90 e0        ldi r25, 0x00 ; 0
     316: 05 c0        rjmp .+10      ; 0x322 <can_init+0x6e>
     318: 83 e0        ldi r24, 0x03 ; 3
     31a: 90 e0        ldi r25, 0x00 ; 0
     31c: 02 c0        rjmp .+4      ; 0x322 <can_init+0x6e>
     31e: 80 e0        ldi r24, 0x00 ; 0
     320: 90 e0        ldi r25, 0x00 ; 0
     322: 88 0f        add r24, r24
     324: 99 1f        adc r25, r25
     326: 88 0f        add r24, r24
     328: 99 1f        adc r25, r25
     32a: fc 01        movw r30, r24
     32c: e3 56        subi r30, 0x63 ; 99
     32e: ff 4f        sbci r31, 0xFF ; 255
     330: e4 91        lpm r30, Z
     332: e0 93 e2 00 sts 0x00E2, r30 ; 0x8000e2 <__TEXT_REGION_LENGTH__+0x7e00e2>
     336: fc 01        movw r30, r24
     338: e2 56        subi r30, 0x62 ; 98
     33a: ff 4f        sbci r31, 0xFF ; 255
     33c: e4 91        lpm r30, Z
     33e: e0 93 e3 00 sts 0x00E3, r30 ; 0x8000e3 <__TEXT_REGION_LENGTH__+0x7e00e3>
     342: fc 01        movw r30, r24
     344: e1 56        subi r30, 0x61 ; 97
     346: ff 4f        sbci r31, 0xFF ; 255
     348: 84 91        lpm r24, Z
     34a: 80 93 e4 00 sts 0x00E4, r24 ; 0x8000e4 <__TEXT_REGION_LENGTH__+0x7e00e4>
     34e: 80 91 e2 00 lds r24, 0x00E2 ; 0x8000e2 <__TEXT_REGION_LENGTH__+0x7e00e2>
     352: 81 11        cpse r24, r1
     354: 0b c0        rjmp .+22      ; 0x36c <can_init+0xb8>
     356: 80 91 e3 00 lds r24, 0x00E3 ; 0x8000e3 <__TEXT_REGION_LENGTH__+0x7e00e3>
     35a: 81 11        cpse r24, r1
     35c: 09 c0        rjmp .+18      ; 0x370 <can_init+0xbc>
     35e: 80 91 e4 00 lds r24, 0x00E4 ; 0x8000e4 <__TEXT_REGION_LENGTH__+0x7e00e4>
     362: 88 23        and r24, r24
     364: 09 f4        brne .+2      ; 0x368 <can_init+0xb4>
     366: a5 c0        rjmp .+330    ; 0x4b2 <__LOCK_REGION_LENGTH__+0xb2>
     368: 80 e0        ldi r24, 0x00 ; 0
     36a: 03 c0        rjmp .+6      ; 0x372 <can_init+0xbe>
     36c: 80 e0        ldi r24, 0x00 ; 0
     36e: 01 c0        rjmp .+2      ; 0x372 <can_init+0xbe>
     370: 80 e0        ldi r24, 0x00 ; 0


[T3sl4co1l]

Ah, nice.

AND regN, regN is shorthand for putting the register into flags: in particular the zero flag is desired here.  On x86, TEST reg, reg is more common (same thing, but doesn't write result back to dest -- not that the result has changed in this case!).  CMP reg,0 probably isn't any slower or longer, the compiler just chose to emit AND instead.  OR regN, regN would do just as well, of course (on any instruction set I'm aware of, pretty much all logical operations take the same time and space).

So, it's just an if(0) else if (1) else if (2) else... statement.

This is also a common structure for switch() statements: below a certain length, it's just built as an if-elseif chain.  Above a certain length, it's bounds-checked then indexed to a jump table.  The jump table can be quite costly in space (i.e., as many words of PROGMEM as elements between bounds), but relatively fast (in particular, O(1) rather than O(N)).

Tim

[cv007]

I could be wrong, but I think what is happening is this line is simply eliminated-

if(config.can.bus_speed == can_bus_speed_bt_conf.bus_speed)

as the enums match up to the array indexes AND the values are restricted to just those enums
so it simply becomes a matter of getting which index is in config.can.bus_speed, then compute the offsets into can_bus_speed_bt_conf and load the values from flash and store them in the canbt registers.

which also means you cannot fail to get a value so the test for all canbt==0 is not needed (unless you have a stored set with all 0's I guess)
(that is one reason enums are useful- you restricted the values without having to check them- the compiler does it)

change those enum values to something else and you will probably not recognize the resulting assembly from the previous
(or change them to #defines and see what happens)