Skip to content

/ ArduinoCore-API

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

Jump to bottom

Fix pgm_read_ptr in AVR pgmspace.h #118

Merged
merged 1 commit into from Oct 7, 2020
Merged

Fix pgm_read_ptr in AVR pgmspace.h #118

merged 1 commit into from Oct 7, 2020

Conversation

@tttapa
Copy link
Contributor

@tttapa tttapa commented Jul 27, 2020

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.
@tttapa
Fix pgm_read_ptr in AVR pgmspace.h
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
cd4ba4c 
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
bblanchon approved these changes Sep 16, 2020
View changes
@bblanchon
Copy link

@bblanchon 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.
@matthijskooijman
matthijskooijman approved these changes Sep 16, 2020
View changes
Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment

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
Copy link

@bblanchon 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
Copy link

@bblanchon 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 bblanchon mentioned this pull request Sep 16, 2020
Closed
@bblanchon
Copy link

@bblanchon bblanchon commented Sep 16, 2020

stm32duino uses void * const *
https://github.com/stm32duino/Arduino_Core_STM32/blob/70a6928ef65d7a8881b05e12f866a214898ac48e/cores/arduino/avr/pgmspace.h#L115
@bblanchon
Copy link

@bblanchon 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
Copy link

@bblanchon 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.
@matthijskooijman
Copy link
Collaborator

@matthijskooijman matthijskooijman commented Sep 16, 2020

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
Copy link

@bblanchon bblanchon commented Sep 16, 2020

It's not a matter of taste; they are different types:

  • *(const void**) is const void*
  • *(void *const *) is void*
@matthijskooijman
Copy link
Collaborator

@matthijskooijman matthijskooijman commented Sep 16, 2020

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).
@aentinger
Copy link
Contributor

@aentinger aentinger commented Oct 7, 2020

Thank you to both @bblanchon and @matthijskooijman 🚀 I'll merge now.
@aentinger aentinger merged commit 76e931e into arduino:master Oct 7, 2020
@bblanchon
Copy link

@bblanchon bblanchon commented Oct 7, 2020

Thanks !!!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthijskooijman matthijskooijman

@bblanchon bblanchon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@tttapa @bblanchon @matthijskooijman @aentinger
You can’t perform that action at this time.