Forum Menu




 


Log in Problems?
New User? Sign Up!
AVR Freaks Forum Index

Post new topic   Reply to topic
View previous topic Printable version Log in to check your private messages View next topic
Author Message
wek
PostPosted: Oct 23, 2009 - 01:44 PM
Raving lunatic


Joined: Dec 16, 2005
Posts: 3218
Location: Bratislava, Slovakia

In pgmspace.h, the pgm_read_xxx()-s' parameter (the address) is explicitly cast to (uint16_t) through a macro, such as in:
Code:
#define pgm_read_byte_near(address_short) __LPM((uint16_t)(address_short))

While the parameter is supposed to be a pointer, this enables to use any garbage (read: non-pointer expression used by error) quietly.

Opposed to this are the similar accessing functions in eeprom.h.

I may be missing some reason for the different approach, but I assume this is mostly for historical reasons and better to be fixed before the next release of avr-libc/WinAVR.

Please, discuss.

Jan Waclawek
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
dl8dtl
PostPosted: Oct 23, 2009 - 04:57 PM
Raving lunatic


Joined: Dec 20, 2002
Posts: 7376
Location: Dresden, Germany

Please, file an avr-libc bug report.

_________________
Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.
Please read the `General information...' article before.
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
wek
PostPosted: Oct 25, 2009 - 09:44 PM
Raving lunatic


Joined: Dec 16, 2005
Posts: 3218
Location: Bratislava, Slovakia

Bug???

I thought we will now sit down and you and others will explain to me how well it is how it is now and how changing it would break too many things, and stuff.

... besides, I don't have a patch and I don't think I am able to come up with one good enough... Wink

Jan
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
dl8dtl
PostPosted: Oct 26, 2009 - 08:10 AM
Raving lunatic


Joined: Dec 20, 2002
Posts: 7376
Location: Dresden, Germany

That's why I didn't suggest you writing a patch report but a bug report. ;-)

_________________
Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.
Please read the `General information...' article before.
 
 View user's profile Send private message Send e-mail Visit poster's website 
Reply with quote Back to top
cmhicks
PostPosted: Oct 26, 2009 - 11:48 AM
Hangaround


Joined: Nov 20, 2008
Posts: 339
Location: Cambridge, UK

wek wrote:
Code:
#define pgm_read_byte_near(address_short) __LPM((uint16_t)(address_short))

While the parameter is supposed to be a pointer, this enables to use any garbage (read: non-pointer expression used by error) quietly.
I agree this is ugly and dangerous. My first thought is that use of an inline function
Code:

__attribute__((__inline__)) uint8_t pgm_read_byte_near(uint8_t *);

uint8_t pgm_read_byte_near(uint8_t *addr) {
    return __LPM((uint16_t)(addr));
}

instead of a macro would allow the cast to be hidden and not propagate outside the scope of the function.

However, this idea does not work for the pgm_read...() funtions that take what is effectively a far pointer.

Christopher Hicks
==
 
 View user's profile Send private message  
Reply with quote Back to top
wek
PostPosted: Oct 26, 2009 - 12:09 PM
Raving lunatic


Joined: Dec 16, 2005
Posts: 3218
Location: Bratislava, Slovakia

cmhicks wrote:
However, this idea does not work for the pgm_read...() funtions that take what is effectively a far pointer.

Did you mean pgm_read_xxx_far()?

I believe this was not the intention of the pgm_read_xxx functions.

In my copy of the avr_libc user manual, for pgmspace.h, the doxygen generated sh*t given there instead of documentation says:
Quote:
22.18.2.2 #define pgm_read_byte(address_short) pgm_read_byte_-
near(address_short)
Read a byte from the program space with a 16-bit (near) address.
Note:
The address is a byte address. The address is in the program space.

The same goes for other pgm_read_xxx() functions. There is no formal definition of "address_short" given, but I believe (and the hints throughout the document, and in section 5 indicate so) that the intention is to keep the parameter as a native, i.e 16-bit, pointer, as that's the prevailing mode of usage of these functions.

JW
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
cmhicks
PostPosted: Oct 26, 2009 - 02:05 PM
Hangaround


Joined: Nov 20, 2008
Posts: 339
Location: Cambridge, UK

wek wrote:
Did you mean pgm_read_xxx_far()?
Yes, exactly. Those take an address >64k, so the address cannot be conveyed in an avr-gcc pointer type.
Quote:
There is no formal definition of "address_short" given, but I believe (and the hints throughout the document, and in section 5 indicate so) that the intention is to keep the parameter as a native, i.e 16-bit, pointer, as that's the prevailing mode of usage of these functions.
Yes, that's my reading of it too, but this is not extensible to addresses >64k. The decision has been taken to make the near and far access macros analogous to each other, rather than to utilise the pointer type checking that could be provided by the compiler for the near access methods (but not the far).

It's messy, but using 16-bit pointers on an address space that spans more than 64k is bound to be messy. I'm not defending the status quo, but admitting that I can't see a particularly neat alternative.

CH
==
PS I suppose the EEPROM access methods don't suffer from the same problem because the EEPROM is always smaller than 64kBytes.

PPS As an off-topic, but related aside, what happens if one's code calls for a function pointer, the target of which lies above 64k? What if said pointer is a pointer to a C++ virtual function, and is therefore in the vtable of a C++ object?
 
 View user's profile Send private message  
Reply with quote Back to top
wek
PostPosted: Oct 27, 2009 - 10:20 AM
Raving lunatic


Joined: Dec 16, 2005
Posts: 3218
Location: Bratislava, Slovakia

Maybe the appropriate approach would be to:

  • leave pgm_read_xxx_near() and pgm_read_xxx_far() as they are now
  • replace the pgm_read_xxx() #defines by the inline (or maybe always_inline) "wrapper" as per Christopher's suggestion
That would impose the "checking" on the most used functions, intended for the potentially unaware users; whereas the "advanced" users of the _near and _far functions may still freely shoot themselves in their respective feet as per their needs.

Does this make any sense?

JW
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
cmhicks
PostPosted: Oct 27, 2009 - 10:32 AM
Hangaround


Joined: Nov 20, 2008
Posts: 339
Location: Cambridge, UK

wek wrote:
Does this make any sense?
Yes, that seems a good compromise. All the _P string function variants are limited to strings in the first 64k of FLASH and make use of pointer types, so to have the default raw FLASH reading functions implemented similarly seems eminently sensible.

CH
==
 
 View user's profile Send private message  
Reply with quote Back to top
wek
PostPosted: Nov 19, 2009 - 06:34 PM
Raving lunatic


Joined: Dec 16, 2005
Posts: 3218
Location: Bratislava, Slovakia

I would like to take this issue a bit further towards actual implementation, to be able to submit a possible solution before the new WinAVR issue. I need to make things clear for myself, too.

As we might have agreed, the pgm_read_xxx() functions might need to be wrapped to an inline function, to facilitate parameter checking. However, I have trouble with adding a "real" function into a header (<avr/pgmspace.h>). On the other hand, there's no point to declare a library function inline.

I was thinking of adding this modified Christopher's code in the following way:
Code:

__attribute__((__always_inline__)) static uint8_t pgm_read_byte(uint8_t *);

static uint8_t pgm_read_byte(uint8_t *addr) {
    return __LPM((uint16_t)(addr));
}

(the pointer might be also declared to point specifically into a PROGMEM byte, although I know there's no value in doing so now, but maybe PROGMEM might get deeper meaning in the future?)

Although this is still violating "no code in header", the __always_inline__ attribute should ensure that this won't ever compile to a callable function, and static should ensure this won't be visible outside the module where pgmspace.h is included.

Might this be viable enough?


Jan Waclawek
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
cmhicks
PostPosted: Nov 19, 2009 - 07:46 PM
Hangaround


Joined: Nov 20, 2008
Posts: 339
Location: Cambridge, UK

wek wrote:
the pointer might be also declared to point specifically into a PROGMEM byte, although I know there's no value in doing so now, but maybe PROGMEM might get deeper meaning in the future?)
At very least it should be const:
Code:
__attribute__((__always_inline__)) static uint8_t pgm_read_byte(const uint8_t *);

Quote:
Although this is still violating "no code in header", the __always_inline__ attribute should ensure that this won't ever compile to a callable function, and static should ensure this won't be visible outside the module where pgmspace.h is included.

Might this be viable enough?
It looks OK to me, but I don't feel sufficiently expert to give a definitive answer. For one thing, I am not sure what happens if one makes a function pointer point to a function which has the __always_inline__ attribute. I expect a callable copy gets linked in (despite the __always_inline__), but this is probably the desired behaviour.

CH
==
 
 View user's profile Send private message  
Reply with quote Back to top
clawson
PostPosted: Nov 20, 2009 - 10:24 AM
10k+ Postman


Joined: Jul 18, 2005
Posts: 71677
Location: (using avr-gcc in) Finchingfield, Essex, England

Quote:

Although this is still violating "no code in header",

See AVR-LibC's existing eeprom.h - "static __inline__" is already used widely in that.

_________________
 
 View user's profile Send private message  
Reply with quote Back to top
wek
PostPosted: Nov 20, 2009 - 10:38 AM
Raving lunatic


Joined: Dec 16, 2005
Posts: 3218
Location: Bratislava, Slovakia

As per Joerg's suggestion, submitted as an avr-libc bug.

I've added the PROGMEM attribute to the pointer; I also suggested the __pure__ attribute after having a peek at eeprom.h (thanks Cliff). I also think that __always_inline__ is a more adequate attribute than just __inline__ in this context.

My latest project threw 60 warnings when I changed my pgmspace.h... (but let me say, that the file where the violating statements are located, is almost 2 years old) Wink

JW
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
SprinterSB
PostPosted: Jul 02, 2011 - 11:47 PM
Posting Freak


Joined: Dec 21, 2006
Posts: 1738
Location: Saar-Lor-Lux

Using uint8 is no good idea. What you want is const void* or otherwise you won't manage to compile the following code without warnings:
Code:
#include <avr/pgmspace.h>

#undef pgm_read_byte

static uint8_t __attribute__((__always_inline__,__unused__,__pure__))
pgm_read_byte (const void * addr)
{
    return __LPM((uint16_t) addr);
}

char get1 (const char * p)
{
    return pgm_read_byte (&p[4]);
}

char get2 (const uint8_t * p)
{
    return pgm_read_byte (&p[4]);
}

char get3 (const int8_t * p)
{
    return pgm_read_byte (&p[4]);
}


wek wrote:
the pointer might be also declared to point specifically into a PROGMEM byte, although I know there's no value in doing so now,...
I don't know why everyone is using attribute progmem in typedef. It's not documented anywhere, it should throw an error! It's unspecified.
Quote:
but maybe PROGMEM might get deeper meaning in the future?
No. If there were named address spaces in avr-gcc, the name will not be PROGMEM. And a named address space identifier will behave like a qualifier, not like an attribute.
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
js
PostPosted: Jul 03, 2011 - 12:59 AM
10k+ Postman


Joined: Mar 28, 2001
Posts: 22743
Location: Sydney, Australia (Gum trees, Koalas and Kangaroos, No Edelweiss)

Is this still the case even after 1 and 1/5 years after the last post? Wink

_________________
John Samperi
Ampertronics Pty. Ltd.
www.ampertronics.com.au
* Electronic Design * Custom Products * Contract Assembly
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
SprinterSB
PostPosted: Jul 03, 2011 - 09:25 AM
Posting Freak


Joined: Dec 21, 2006
Posts: 1738
Location: Saar-Lor-Lux

js wrote:
Is this still the case even after 1 and 1/5 years after the last post?
Here is the current avr/pgmspace.h.
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
theusch
PostPosted: Jul 03, 2011 - 04:21 PM
10k+ Postman


Joined: Feb 19, 2001
Posts: 29178
Location: Wisconsin USA

Quote:

js wrote:
Is this still the case even after 1 and 1/5 years after the last post?
Here is the current avr/pgmspace.h.


Is that a yes/no answer? For those not into the technical details, examining the file gives us no answer to the question.

Lee
 
 View user's profile Send private message  
Reply with quote Back to top
SprinterSB
PostPosted: Jul 03, 2011 - 04:40 PM
Posting Freak


Joined: Dec 21, 2006
Posts: 1738
Location: Saar-Lor-Lux

The file shows that pgm_read_* are still implemented as macros.

So if there are plans to implement them as inline function to provide some type checking, their parameter should be "const void*" and not "uint8_t*".

Moreover, such functions need to be attributed "unused" or otherwise you will get complaints like
Code:
warning: 'uint8_t pgm_read_byte(const void*)' defined but not used [-Wunused-function]
 
 View user's profile Send private message Visit poster's website 
Reply with quote Back to top
Display posts from previous:     
Jump to:  
All times are GMT + 1 Hour
Post new topic   Reply to topic
View previous topic Printable version Log in to check your private messages View next topic
Powered by PNphpBB2 © 2003-2006 The PNphpBB Group
Credits