pgm_read_xxx() parameter typecast is harmful

18 posts / 0 new
Last post
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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

#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

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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... ;-)

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

wek wrote:

#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

__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
==

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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
==

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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 (). 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:

__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

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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:
__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
==

Pages