maxgerhardt / platform-raspberrypi

Raspberry Pi: development platform for PlatformIO
Apache License 2.0
94 stars 46 forks source link

Deadlock when accessing constant data through misaligned pointer generated at compile time #34

Closed elominp closed 11 months ago

elominp commented 11 months ago

Hello,

Thanks for your awesome platform integration of the RP2040 core earlephillower (until now that I discovered it I was simply building & uploading through the Arduino CLI and editing code + debugging with custom configuration in VS Code x)).

I have an issue though I noticed with a small pet project I started sunday, freshly installing everything (so it's latest version of PlatformIO + your platform + earlephillower core & C++ compilers on Windows).

I'm not sure if I was only lucky until now and it's just the way I handle constant values that is wrong, but I have an issue now where randomly the compiler emits pointers on constant values with the LSB set to 1 instead of 0 (causing the program to crash).

To illustrate simply, I have in a .h file something like this (the content doesn't matter from what I noticed):

static const unsigned char myBigConstantAsset[] = { /* generatedBytes */ };

Then I later retrieve a pointer on it to feed it to some library or do some processing, for example just doing this:

const uint16_t * data = (const uint16_t *) myBigConstantAsset; // pointer on 16-bit 565 RGB image data for example
uint16_t color = data[0]; // Can crash immediately here, using the pointer without doing anything in-between that could corrupt or overwrite it

Will work if data has LSB to 0 (which is normal) but crash if the LSB is randomly 1 (which is wrong for data access, I remember a long time ago doing some trickery to manually set the LSB to 1 on an ARM application to get a function pointer - does that mean by the way the compiler sometime treats my data pointer as a function pointer ?!).

I use this configuration in my platform.ini file that should be fairly basic:

[env]
platform = https://github.com/maxgerhardt/platform-raspberrypi.git
framework = arduino
board_build.core = earlephilhower
board_build.filesystem_size = 0m
board_build.mcu = rp2040
board_build.f_cpu = 150000000L
lib_deps = 
    SPI
    bodmer/TFT_eSPI@^2.5.31
build_flags = 
    -O2
    -fpermissive
    -DUSER_SETUP_LOADED=1
    -DST7735_DRIVER=1
    -DST7735_GREENTAB3=1
    -DTFT_RGB_ORDER=TFT_BGR
    -DTFT_WIDTH=128
    -DTFT_HEIGHT=128
    -DRP2040_PIO_SPI=1
    -DRP2040_PIO_CLK_DIV=1
    -DTFT_DC=3
    -DTFT_SCLK=4
    -DTFT_MOSI=5
    -DTFT_MISO=-1
    -DTFT_CS=7
    -DTFT_RST=8
    -DTFT_BL=9
    -DTFT_BACKLIGHT_ON=1

[env:pico]
board = pico

I'm confused because I used several times embedded constant values (I also use a lot of contexpr computed values by the way) instead of emulated FS (I know I'm lazy on this one) and it's the first time I have this issue :/

And it's not super easy to reproduce to compare with other platforms as just cleaning sometime or anything that change the emitted binary also change the emitted address of the pointers and can produce them as correctly usable.

For now I settled with this workaround to always remove the LSB if it's set to 1:

const uint16_t * myPointer = (const uint16_t *)((size_t) sheet.data & 0xFFFFFFFE) // Casting pointer to size_t + applying binary mask to remove LSB and cast back to original type

Which seems to work since now my code runs without anymore issue even when I still saw the original address having the LSB set to 1

I'll still try to reproduce a custom configuration on the Arduino IDE with a custom board definition reusing the core of earlephillower and as much as possible the same compilation options to see if I'm able to also reproduce the issue.

elominp commented 11 months ago

Workaround 2: simply using alignas(sizeof(void *)) (depending on the context such as DMA, other types of aligment might be more suitable) so I'm simply doing my own PROGMEM definition

maxgerhardt commented 11 months ago
const uint16_t * data = (const uint16_t *) myBigConstantAsset; // pointer on 16-bit 565 RGB image data for example
uint16_t color = data[0]; // Can crash immediately here, using the pointer without doing anything in-between that could corrupt or overwrite it

Pretty sure the Cortex-M0+ can't handle unaligned access. Accessing a uint16_t (2 byte value) needs a 2-byte alignment. Adding that to the myBigConstantAsset definition should fix the issue as you already confirmed.

elominp commented 11 months ago
const uint16_t * data = (const uint16_t *) myBigConstantAsset; // pointer on 16-bit 565 RGB image data for example
uint16_t color = data[0]; // Can crash immediately here, using the pointer without doing anything in-between that could corrupt or overwrite it

Pretty sure the Cortex-M0+ can't handle unaligned access. Accessing a uint16_t (2 byte value) needs a 2-byte alignment. Adding that to the myBigConstantAsset definition should fix the issue as you already confirmed.

Exact, it can't.

Still it's pretty strange that GCC emitted this type of code, something must have re-enabled it as per the documentation it shouldn't:

-mno-unaligned-access

Enables (or disables) reading and writing of 16- and 32- bit values from addresses that are not 16- or 32- bit aligned.
By default unaligned access is disabled for all pre-ARMv6, all ARMv6-M and for ARMv8-M Baseline architectures, and enabled for all other architectures.
If unaligned access is not enabled then words in packed data structures are accessed a byte at a time.

The ARM attribute Tag_CPU_unaligned_access is set in the generated object file to either true or false, depending upon the setting of this option.
If unaligned access is enabled then the preprocessor symbol __ARM_FEATURE_UNALIGNED is also defined.

Anyway at least I learned something with this issue and it doesn't cost much to adapt the code to avoid architecture dependants issues