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

[David Brown]

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 <address@hidden
> <mailto:address@hidden>> 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