[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [avr-libc-dev] [patch #7909] Adding __volatile__ to __asm__ within p

From: David Brown
Subject: Re: [avr-libc-dev] [patch #7909] Adding __volatile__ to __asm__ within pgmspace header
Date: Tue, 1 Jan 2013 16:25:10 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120912 Thunderbird/15.0.1

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) {

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.


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.



On Mon, Dec 31, 2012 at 5:36 AM, David Brown <address@hidden
<mailto:address@hidden>> wrote:

    On 31/12/12 06:54, Alex Crawford wrote:


                           Summary: Adding __volatile__ to __asm__
        within pgmspace
                           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



        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
        problem. I've added the directive to each of the LPM and ELPM
        calls (20 in

        The patch was generated from avr-libc-1.8.0.


    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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]