pgm_read_byte() does not work well without delay()

[milkpirate]

I have the following code:

PROGMEM const uint8_t table[2][3] = {
  {0x7B,0x60,0x5D},
  {0x7D,0x48,0x3E}
};

void setup() {
  Serial.begin(115200);
}

void loop() {
  uint8_t data = pgm_read_byte(&table[1][1]);
  Serial.println(data, HEX);
  delay(250);
}

which results in:

Exception (3):
epc1=0x40201c0e epc2=0x00000000 epc3=0x00000000 excvaddr=0x4022e71c depc=0x00000000

ctx: cont
sp: 3ffef200 end: 3ffef3c0 offset: 01a0

>>>stack>>>
3ffef3a0:  feefeffe feefeffe feefeffe 40202098
3ffef3b0:  feefeffe feefeffe 3ffee3a0 40100114
<<<stack<<<

 ets Jan  8 2013,rst cause:2, boot mode:(3,6)

load 0x4010f000, len 1384, room 16
tail 8
chksum 0x2d
csum 0x2d
v09f0c112

But swapping Serial.println(data, HEX); and delay(250); then works. Why?

EDIT:
Shorter delays make it work as well even a "not existing" one:

PROGMEM const uint8_t table[2][3] = {
  {0x7B,0x60,0x5D},
  {0x7D,0x48,0x3E}
};

void setup() {
  Serial.begin(115200);
}

void loop() {
  uint8_t data = pgm_read_byte(&table[1][1]);
  delayMicroseconds(0);
  Serial.println(data, HEX);
  delay(250);
}

[igrr]

Could you please upload the elf file for the code which demonstrates the crash? That would really help.

[milkpirate]

Elf file (1.8.0): http://www21.zippyshare.com/v/Vhm4iyHc/file.html (sorry could not find the option to attach files).

[igrr]

OK, thanks. The compiler sees that offset in the array is constant; so it can calculate everything that happens in the pgm_read_byte macro at compile time. It sees that in this particular case, shifting and masking is not needed (because the byte you are loading happens to be 4-byte aligned) and it optimizes away all the code inside pgm_read_byte, replacing it with a single l8ui instruction. Which obviously doesn't work because the cache port supports only 32-bit access.

I guess we can hard-code the load/shift/mask part inside an __volatile__ assembly block so that this optimization doesn't happen.

/cc @Makuna

[milkpirate]

Ahhh, OK perfect! Could you also explain why the delayMicroseconds(0); helps? Would attributing the pgm_read_byte function itself as __volatile__ suffice? As far as I know, writing correct inline assembly is pretty hard (NOT meaning you're not able to).

[Makuna]

Would also decorating the code with a #pragma that stops optimizations work?
Could you modify this line in your sketch and try it?

// uint8_t data = pgm_read_byte(&table[1][1]); // old 
uint8_t data = pgm_read_byte(&(table[1][1])); // new, take the address of the specific table entry 

[milkpirate]

Ok also tried pgm_read_byte(&(table[1][1])); (is C++ that picky?) no change. Even tried pgm_read_byte(&(1[table][1])); still no luck :P

[Makuna]

replace pgm_read_byte with...

#define pgm_read_byte(addr) 		                                           \
(__extension__({                                                               \
    PGM_P __local = (PGM_P)(addr);  /* isolate varible for macro expansion */         \
    ptrdiff_t __offset = ((uint32_t)__local & 0x00000003); /* byte aligned mask */            \
    const uint32_t* __addr32 = (const uint32_t*)((const uint8_t*)(__local)-__offset); \
    uint32_t __data32 = *__addr32; /* cache data locally as 32bits */ \
    uint8_t __result = (__data32 >> (__offset * 8));                        \
    __result;                                                                  \
}))

I will do some testing tomorrow

[igrr]

@Makuna, I think what you are trying to do is to trick the compiler while speaking C. I think this won't work for the following reason. What the compiler is doing is fully valid given the architectural model it has. The backend is not aware that there are memory regions from which byte loads are not allowed. The only way to express this (i think) is to write this part in assembly. Every C expression you write which can be evaluated at compile time can be simplified by the compiler to a byte load.
The only other way is to not let the compiler do optimization based on known arguments, that is, turn pgm_read_byte into a (non-inline) function.

[Makuna]

@igrr Actually it would be a bug if the compiler disregards sizes if defined; the original was written that the size is ambiguous giving the compiler the option to do the wrong thing. I want to try one other option, which is to just insert a cast. These aren't tricks, they are considered hints. Most developers I know would consider reverting to assembler to get the compiler to do the right thing as the last option to try.

[Makuna]

It seems that GCC requires the volatile keyword to force the hints and data size is not enough. There are many references to this when you search online about GCC over optimizing in similar cases across other domains.

[devyte]

Hi, I have two questions: 1. Why are these pgm_* implemented as #defines instead of inline functions? 2. Why can't the volatile keyword be used in declaring the intermediate variables? Apologies if the questions are noobid, I'm currently on the road, otherwise I'd take a closer look :p

…

On Apr 22, 2017 3:21 PM, "Michael Miller" ***@***.***> wrote: It seems that GCC requires the volatile keyword to force the hints and data size is not enough. There are many references to this when you search online about GCC over optimizing in similar cases across other domains. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3140 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQC6Bsu7UNhO-R4tgcYhIxfHwfmCws67ks5rykUagaJpZM4M-cum> .

[Makuna]

@devyte I don't understand the second question, as the change is to define one as volatile. could you be more specific?

I don't remember the reason other than following the Arduino originals as close as possible (they were also macros). It was almost two years ago when I wrote those ;-)

Do note, the inline prefix does not guarantee to make it inline, its just a hint to the compiler that they can be.