On 31/12/12 22:21, Alex Crawford wrote:
Here is a boiled down example of the issue I was seeing:while (pgm_read_word_far(0x00000000) != 0xFFFF) { avr109_process(); } 1e152:80 e0 ldir24, 0x00; 0 1e154:90 e0 ldir25, 0x00; 0 1e156:dc 01 movwr26, r24 1e158:ab bf out0x3b, r26; 59 1e15a:fc 01 movwr30, r24 1e15c:c7 91 elpmr28, Z+ 1e15e:d6 91 elpmr29, Z+ 1e160:02 c0 rjmp.+4 ; 0x1e166 <main+0x14> 1e162:0e 94 31 f2 call0x1e462; 0x1e462 <avr109_process> 1e166:8f ef ldir24, 0xFF; 255 1e168:cf 3f cpir28, 0xFF; 255 1e16a:d8 07 cpcr29, r24 1e16c:d1 f7 brne.-12 ; 0x1e162 <main+0x10> As seen in the assembly, the compiler chooses to elpm only one time before the loop (because it is not volatile). The avr109_process() function in the example is a bootloader (surprise!) and will write to flash location 0x00. Maybe I misunderstood the purpose of the pgm_read macros, but I feel like these should be marked volatile since the data they are reading is subject to change without their knowing. Please correct me if I am wrong (I'm pretty new to this stuff). David, you mentioned that flash memory is not volatile (and is treated as such by avr-libc). Can you explain why this assumption is made? I agree that flash memory will remain constant in the vast majority of cases, but in the few cases that it does change, the program is probably checking itself (via LPM calls) for changes. I may be way off base, but it seems that these pgm_read macros would lend themselves most useful for self modifying programs (where the inline assembly would need to be marked volatile in order to work properly). I am very interested to hear what others think about this. As I said, I'm new to this. -Alex
Well, some of this is a matter of opinion - and that's why it's good to discuss it on this mailing list. But as you note, in the vast majority of cases flash memory is constant. I don't think it is appropriate to mark accesses as volatile and de-optimise for normal use, just because of a single odd usage here. It would be much better to find a better way of handling this particular case, such as by adding a memory barrier (asm volatile("":::"memory")), or by using a different mechanism for the bootloader code (which is presumably interrupt based) to communicate with the loop. A normal volatile boolean or byte ram variable would seem appropriate.
Self-modifying code is more evil that using gotos - you really do not want to do that. Self-modifying makes it so hard to guarantee code correctness that you can pretty much guarantee code incorrectness. Bootloaders, however, are not self-modifying code in the normal sense - they are for downloading new main application code. And while you are right that pgm_read and other related macros and calls are often used in bootloaders (though they are not necessary), their main usage is for reading static data and tables.
But as you say, we will hear what others here have to say. mvh., David
On Mon, Dec 31, 2012 at 5:36 AM, David Brown <da...@westcontrol.com <mailto:da...@westcontrol.com>> wrote: On 31/12/12 06:54, Alex Crawford wrote: URL: <http://savannah.nongnu.org/__patch/?7909 <http://savannah.nongnu.org/patch/?7909>> Summary: Adding __volatile__ to __asm__ within pgmspace header Project: AVR C Runtime Library Submitted by: crawford Submitted on: Mon 31 Dec 2012 05:54:17 AM GMT Category: None Priority: 5 - Normal Status: None Privacy: Public Assigned to: None Originator Email: Open/Closed: Open Discussion Lock: Any _________________________________________________________ Details: I noticed that gcc was optimizing out calls to pgm_read_word_far() that were within a while loop. Adding the volatile directive fixes the underlying problem. I've added the directive to each of the LPM and ELPM calls (20 in all). The patch was generated from avr-libc-1.8.0. -Alex Could you post an example of the original C code and generated assembly code demonstrating that there is actually a problem? I would very much hope that the compiler will optimise out calls to read program memory within a loop if it does not have to repeat them. Program memory is not volatile (baring the unusual case of re-programming), and forcing the compiler to re-read data is a waste of time and space. On the other hand, if the compiler is making an illegal optimisation here then perhaps this patch is just hiding a bigger problem. mvh., David
_______________________________________________ AVR-libc-dev mailing list AVR-libc-dev@nongnu.org https://lists.nongnu.org/mailman/listinfo/avr-libc-dev
