-
-
Notifications
You must be signed in to change notification settings - Fork 134
Fix pgm_read_ptr in AVR pgmspace.h #118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Originally, `pgm_read_ptr` used to cast its argument to `const void *`, i.e. a pointer to read-only data. This is incorrect, because the argument is a pointer to a pointer in "flash memory". You cannot cast it to `const void *` and then dereference it, because `void` is not an object type. Also, the pointer itself is read-only, but the data could in theory be mutable. The correct type should be `void *const *`, i.e. a pointer to a read-only pointer to any data.
bblanchon
commented
Sep 16, 2020
I confirm that this change is necessary to fix the following error:
arduino/api/deprecated-avr-comp/avr/pgmspace.h:107:49: error: 'const void*' is not a pointer-to-object type
#define pgm_read_ptr(addr) (*(const void *)(addr))
^
Please merge this pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also looks good to me.
One nitpick: I would have a small preference to use const void ** instead of void *const *, which is a bit easier to read but AFAIU equivalent. This is also what is already used in practice in e.g. the SAMD core: https://github.com/adafruit/ArduinoCore-samd/blob/6a59e8347d01a9a7a51cd71f7981d154d851ba35/cores/arduino/avr/pgmspace.h#L106
bblanchon
commented
Sep 16, 2020
The core for ESP32 use void * const *.
https://github.com/espressif/arduino-esp32/blob/37a7fb3d6ada8a9230c96e2df999c95ef10fd4d3/cores/esp32/pgmspace.h#L53
bblanchon
commented
Sep 16, 2020
The core for ESP8266 uses const void* const*
https://github.com/esp8266/Arduino/blob/40eb5747e4b7d3458e9406c3c446948bbffa767b/tools/sdk/libc/xtensa-lx106-elf/include/sys/pgmspace.h#L115
bblanchon
commented
Sep 16, 2020
bblanchon
commented
Sep 16, 2020
"Arduino STM32" uses const void **
https://github.com/rogerclarkmelbourne/Arduino_STM32/blob/fedf4d4c3bfdc2377b20dc0d8e5339bb6c505fbe/STM32F1/cores/maple/avr/pgmspace.h#L35
bblanchon
commented
Sep 16, 2020
As you see, there are many variants out there.
However, if we go back to the original definition in avr-libc, we see that the macro casts the result to void*.
Therefore, to emulate the original macro, we should use void *const *, as @tttapa wrote.
But I'm saying that, AFAIU, const void** is exactly the same type as void *const *, it's just written in an IMHO more readable/common way. Or are you arguing that these types are different?
bblanchon
commented
Sep 16, 2020
It's not a matter of taste; they are different types:
*(const void**)isconst void**(void *const *)isvoid*
Hm, seems I was wrong indeed, thanks for insisting.
So with that out of the way, the PR as it is now is indeed correct, so I'm for merging it.
Note that this also means that the SAMD and Arduino_STM32 versions are indeed also subtly broken (returning const void* instead of void*), though not as broken as the version in the ArduinoCore-API repo right now (which does not even dereference successfully).
Thank you to both @bblanchon and @matthijskooijman 🚀 I'll merge now.
bblanchon
commented
Oct 7, 2020
Thanks !!!
Calling this macro causes the error "'const void*' is not a pointer-to-object type" Related to arduino/ArduinoCore-API#118 Fixes bblanchon/ArduinoJson#1947
Originally,
pgm_read_ptrused to cast its argument toconst void *, i.e. a pointer to read-only data.This is incorrect, because the argument is a pointer to a pointer in "flash memory". You cannot cast it to
const void *and then dereference it, becausevoidis not an object type.Also, the pointer itself is read-only, but the data could in theory be mutable.
The correct type should be
void *const *, i.e. a pointer to a read-only pointer to any data.