bugAVR C Runtime Library - Bugs: bug #38021, pgm_read macros don't model the...

 
 

bug #38021: pgm_read macros don't model the memory accesses

Submitted by:  Georg-Johann Lay <gjlayde>
Submitted on:  Fri 04 Jan 2013 01:05:37 PM UTC  
 
Category: HeaderSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Header files
Status: NonePercent Complete: 0%
Assigned to: NoneOpen/Closed: Open
Release: 1.8.0Fixed Release: None

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Sat 05 Jan 2013 08:56:58 AM UTC, comment #2:

Jan Waclawek wrote in comment #1:

> It is a very bad idea overall. It would increase code size
> of existing programs, with little to no benefit except of
> a negligibly small group of applications.


Would you give a test case from the real world?

I don't see how this would increase the code size in ordinary programs -- except in the case where a clear if RAMPZ is missing and must be added to comply with avr-gcc. But in that case, the code size increase is inevitable.

> 1. FLASH is inherently non-volatile; SPM out of bootloader
> context is uncommon and those who use that should be aware
> of the risks (there are much more involved anyway).
> Your suggestion is the same as to make all global variables
> volatile just because some of them may be used both in
> interrupts and "main".


No, that is not true. The pgm-macros don't model a memory access at all. Notice that the memory access can be modeled and there is no need for a volatile. Please read my example code again and try to understand it.

> Instead, I suggest documentation enhancement, and perhaps
> a new set of macros,


The problem is that it is very hard for the user to cover the remaining, uncommon cases, whereas providing proper macros does not do harm. Not even a memory barrier will help to fix the uncommon cases because there is no memory access in the asm.

> 3. is related only to the _far variety of pgm_read.
> That is used only by a minority group of users using
> the >64kB FLASH varieties (including me), and given
> the relatively large FLASH, it is likely the pessimisation
> imposed by your suggestion would be significantly
> detrimental to their existing applications.


It's not a pessimization, it's inevitable so that your code will still work with GCC 4.7. I don't see a point in code that is efficient but incorrect.

Georg-Johann Lay <gjlayde>
Fri 04 Jan 2013 10:23:48 PM UTC, comment #1:

It is a very bad idea overall. It would increase code size of existing programs, with little to no benefit except of a negligibly small group of applications.

1. FLASH is inherently non-volatile; SPM out of bootloader context is uncommon and those who use that should be aware of the risks (there are much more involved anyway). Your suggestion is the same as to make all global variables volatile just because some of them may be used both in interrupts and "main".

Instead, I suggest documentation enhancement, and perhaps a new set of macros, named perhaps pgm_read_volatile_xxx, maybe even placed into a separate header (or bootloader.h, as the holder of SPM-related stuff, maybe?)

2. in <4.7 is not an issue; in >4.7 is covered by 3.

3. is related only to the _far variety of pgm_read. That is used only by a minority group of users using the >64kB FLASH varieties (including me), and given the relatively large FLASH, it is likely the pessimisation imposed by your suggestion would be significantly detrimental to their existing applications.

On the other hand, the far memory access introduced thanks to your work on the named address space 4.7+ is still not completely mature (see the recent discussion on the linker script) and I have not heard of any user reporting about using them in a real-world application (honestly, I believe nobody uses it so far, amongst the reason being lack of comprehensive binary builds we discussed recently, and also the lack of user-grade documentation for it, sorry). I don't believe it would become the preferred method for far memory access sooner than in a year or even later. That's why I believe the existing forms of _far variety of pgm_read should not be pessimized in favour of 4.7+ features.

Again, I suggest enhancements in documentation before any other measures. The _far macros usage is far from being seamless anyway and its documentation should be enhanced (see my remarks on avrfreaks on this topic, for example). The implementation details should not be hidden from the user, contrary. Warnings could be provided through some compiler or macro magic if the two approaches clash, but I don't believe it's important at all. And again, alternative macros or an alternative header could be provided, but I personally see that as completely useless, as I don't see any point in mixing the two accesses, once the named address space methods will mature.

JW

Jan Waclawek <wek>
Fri 04 Jan 2013 01:05:37 PM UTC, original submission:

The current (1.8.0) implementation of the pgm_read inline assembler does not model the memory accesses it performs. This can lead to problems in the following situations:

1) The memory location is const volatile
2) If ELPM is issued, RAMPZ is changed unnoticed
3) Setting RAMPZ to values other than 0 is not compatible with avr-gcc 4.7+ and xmega with > 64k RAM, cf. "Handling of the RAMP* Special Function Registers" in http://gcc.gnu.org/onlinedocs/gcc/AVR-Options.html

1) Will raise problems together with code that uses SPM like the following. The code is not exactly the same as in pgmspace.h but nevertheless can serve as a test case to show the problem:

With avr-gcc 4.7 you will see code similar to this:

Notice that the inline assembler code is outside the loop. This will work with const* addresses but nit with const volatile* stuff.

A possible fix is to make the asm volatile and clobber memory. A second fix is to make the memory access explicit like so:

Notice the asm is inside the loop now:

2) and 3) can be solved by clearing RAMPZ at the end of the asm so that there is no net effect on RAMPZ.

Georg-Johann Lay <gjlayde>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by wek (Posted a comment)
  • -unavailable- added by gjlayde (Submitted the item)
  •  

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    No Changes Have Been Made to This Item

    Back to the top


    Powered by Savane 3.1-cleanup