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

[Alex Crawford]

Here is a boiled down example of the issue I was seeing:

while (pgm_read_word_far(0x00000000) != 0xFFFF) {
    avr109_process();
}

1e152: 80 e0       ldi r24, 0x00 ; 0
1e154: 90 e0       ldi r25, 0x00 ; 0
1e156: dc 01       movw r26, r24
1e158: ab bf       out 0x3b, r26 ; 59
1e15a: fc 01       movw r30, r24
1e15c: c7 91       elpm r28, Z+
1e15e: d6 91       elpm r29, Z+
1e160: 02 c0       rjmp .+4       ; 0x1e166 <main+0x14>
1e162: 0e 94 31 f2 call 0x1e462 ; 0x1e462 <avr109_process>
1e166: 8f ef       ldi r24, 0xFF ; 255
1e168: cf 3f       cpi r28, 0xFF ; 255
1e16a: d8 07       cpc r29, 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


On Mon, Dec 31, 2012 at 5:36 AM, David Brown <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
>
>
>
>
>
>>
>>      ______________________________**_________________________
>>
>> File Attachments:
>>
>>
>> ------------------------------**-------------------------
>> Date: Mon 31 Dec 2012 05:54:17 AM GMT  Name: volatile.patch  Size: 13kB
>> By:
>> crawford
>> Patch for avr/pgmspace.h
>> <http://savannah.nongnu.org/**patch/download.php?file_id=**27168<http://savannah.nongnu.org/patch/download.php?file_id=27168>
>> >
>>
>>      ______________________________**_________________________
>>
>> Reply to this item at:
>>
>>    
>> <http://savannah.nongnu.org/**patch/?7909<http://savannah.nongnu.org/patch/?7909>
>> >
>>
>> ______________________________**_________________
>>    Message sent via/by Savannah
>>    http://savannah.nongnu.org/
>>
>>
>> ______________________________**_________________
>> AVR-libc-dev mailing list
>> address@hidden
>> https://lists.nongnu.org/**mailman/listinfo/avr-libc-dev<https://lists.nongnu.org/mailman/listinfo/avr-libc-dev>
>>
>>
>