Skip to content
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

Misaligned struct pointer from esp camera #278

Closed
ViktorWb opened this issue Jan 19, 2024 · 22 comments
Closed

Misaligned struct pointer from esp camera #278

ViktorWb opened this issue Jan 19, 2024 · 22 comments

Comments

@ViktorWb
Copy link

ViktorWb commented Jan 19, 2024

Hi! I activated esp32-camera using

[package.metadata.esp-idf-sys]
extra_components = [
  { component_dirs = ["component"], bindings_header = "bindings.h", bindings_module = "cam" },
]

in my Cargo.toml. This worked just fine. Then I tried to do:

esp_idf_sys::cam::esp_camera_init(&camera_config)
let fb = esp_idf_sys::cam::esp_camera_fb_get();
std::slice::from_raw_parts((*fb).buf, (*fb).len)

Which also works fine, but only in release mode. When running in debug mode I get

misaligned pointer dereference: address must be a multiple of 0x8 but is 0x3f801fc4

The issue is that the fb variable is a pointer to a 4-bit aligned struct, while rust requires an 8-bit aligned struct. The struct in question is the camera_fb_t. My understanding is that the alignment checks are skipped in release mode, but that it can cause undefined behaviour.

In the generated bindings, the camera_fb_t struct has an alignment of 8 and is defined as

#[doc = " @brief Data structure of camera frame buffer"]
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct camera_fb_t {
    #[doc = "< Pointer to the pixel data"]
    pub buf: *mut u8,
    #[doc = "< Length of the buffer in bytes"]
    pub len: usize,
    #[doc = "< Width of the buffer in pixels"]
    pub width: usize,
    #[doc = "< Height of the buffer in pixels"]
    pub height: usize,
    #[doc = "< Format of the pixel data"]
    pub format: pixformat_t,
    #[doc = "< Timestamp since boot of the first DMA buffer of the frame"]
    pub timestamp: timeval,
}
@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 20, 2024

The issue is that the fb variable is a pointer to a 4-bit aligned struct, while rust requires an 8-bit aligned struct. The struct in

What architecture is that? riscv? xtensa? In any case, either of these requiring 8 byte aligned struct sounds impossible, as these are 32bit CPUs.

OK, it is likely esp32 (xtensa) or maybe s2 or s3?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 20, 2024

The issue is that the fb variable is a pointer to a 4-bit aligned struct, while rust requires an 8-bit aligned struct.

You sure it is about the fb variable? It could actually be the (*fb).buf variable. On which line exactly do you get the falure?

@ivmarkov
Copy link
Collaborator

In the generated bindings, the camera_fb_t struct has an alignment of 8 and is defined as

Where do you see this? All I see is #[repr(C)] which might just as well mean 4 byte aligned struct?

@ivmarkov
Copy link
Collaborator

Also good to mention:

  • ESP IDF version
  • Rust version

@ivmarkov
Copy link
Collaborator

Looking here it seems the alignment depends on what members the structure or array has. So it can't be the buf thing, as it has u8s = alignment 1.

camera_fb_t on the other hand, has timestamp of type timeval. timeval is interesting, because - putting aside pixformat_t which I don't know what it is - timeval is likely the biggest member of the struct, which would dictate the alignment of the struct. now:

  • In ESP IDF < 5, timeval used to be 4 bytes
  • In ESP IDF >= 5, timeval is now 8 bytes (hence why we have the pesky espidf_time64 flag when building under ESP IDF 5.

Strange. It looks to me as some sort of impossible miscompilation happened, where camera_fb_t at runtime is with 4 bytes timeval, while Rust thinks it is with 8 bytes timeval. What ESP IDF version are you using again?

@ViktorWb
Copy link
Author

ViktorWb commented Jan 20, 2024

Thank you for your input! The following code

let fb: *mut esp_idf_sys::cam::camera_fb_t = esp_idf_sys::cam::esp_camera_fb_get();
println!("Alignment: {}", std::mem::align_of::<esp_idf_sys::cam::camera_fb_t>());
println!("Pointer: {:p}", fb);
let deref = *fb;

prints

Alignment: 8
Pointer: 0x3f801fc4

And then panics on the line let deref = *fb (0x3f801fc4 is not a multiple of 8).

In my .cargo/config.toml I have ESP_IDF_VERSION = "v5.1.1", and in my .embuild/esp-idf I have a folder called v5.1.1, and running git describe in that folder gives v5.1.1. I can't really find anything with version 4 that could have snuck in.

My rust version is 1.75.0.

I am running on the esp32-cam, which uses an ESP32.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 21, 2024

Can you try with ESP IDF V4.4? I'm not suggesting a permanent downgrade, just trying to figure out if timeval is somehow the root of it all (it is 32 bit in 4.4).

@ivmarkov
Copy link
Collaborator

v4.4.6. And you need to remove --cfg espidf_time64 from .cargo/config.toml - and then full rebuild.

@igrr
Copy link

igrr commented Jan 21, 2024

Does Rust generally require that all data types are naturally aligned (i.e. aligned to their own size) or is this just the way the Xtensa target was defined?

For example, the way GCC is configured on Xtensa is that 2- and 4-byte types are naturally aligned, while larger types are aligned to 4-byte boundaries.

@ivmarkov
Copy link
Collaborator

Does Rust generally require that all data types are naturally aligned (i.e. aligned to their own size) or is this just the way the Xtensa target was defined?

Great question. I don't know but I assume rustc would generally follow the alignment specified in the target. Which would mean that there is probably something wrong here w.r.t. types > 32bit for the ESP IDF target, as well as here - for the baremetal ESP32 target (and likely the same for s2 and s3).

According to docu, these are LLVM alignments, actually.

Let me try to decipher this to figure out what it does say for types > 32 bits (if anything).

For example, the way GCC is configured on Xtensa is that 2- and 4-byte types are naturally aligned, while larger types are aligned to 4-byte boundaries.

@ViktorWb
Copy link
Author

On v.4.46, the alignment of camera_fb_t becomes 4, and the problem appears to be gone.

In the document Type layout:

Most primitives are generally aligned to their size, although this is platform-specific behavior. In particular, on x86 u64 and f64 are only aligned to 32 bits.

@ViktorWb
Copy link
Author

ViktorWb commented Jan 21, 2024

The timeval struct is defined as

#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
pub struct timeval {
    pub tv_sec: time_t,
    pub tv_usec: suseconds_t,
}

Turns out there are a bunch of different time_t definitions, and I was hoping that a discrepancy somewhere could have caused the issue. But all of the below gives 8.

println!("i64 alignment: {}", std::mem::align_of::<i64>());
println!("time_t alignment: {}", std::mem::align_of::<esp_idf_sys::time_t>());
println!("esp32-camera time_t alignment: {}", std::mem::align_of::<esp_idf_sys::cam::time_t>());
println!("esp OS time_t alignment: {}", std::mem::align_of::<std::os::espidf::raw::time_t>());
println!("esp_idf_svc time_t alignment: {}", std::mem::align_of::<esp_idf_svc::sys::time_t>());
println!("ffi long long alignment: {}", std::mem::align_of::<std::ffi::c_longlong>());

@ivmarkov
Copy link
Collaborator

The timeval struct is defined as

#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
pub struct timeval {
    pub tv_sec: time_t,
    pub tv_usec: suseconds_t,
}

Turns out there are a bunch of different time_t definitions, and I was hoping that a discrepancy somewhere could have caused the issue. But all of the below gives 8.

println!("i64 alignment: {}", std::mem::align_of::<i64>());
println!("time_t alignment: {}", std::mem::align_of::<esp_idf_sys::time_t>());
println!("esp32-camera time_t alignment: {}", std::mem::align_of::<esp_idf_sys::cam::time_t>());
println!("esp OS time_t alignment: {}", std::mem::align_of::<std::os::espidf::raw::time_t>());
println!("esp_idf_svc time_t alignment: {}", std::mem::align_of::<esp_idf_svc::sys::time_t>());
println!("ffi long long alignment: {}", std::mem::align_of::<std::ffi::c_longlong>());

Did you re-test this with ESP IDF < 5? On ESP IDF < 5 it should give you 4, not 8.

@ivmarkov
Copy link
Collaborator

ivmarkov commented Jan 21, 2024

See here. ESP IDF < 5 does not have espidf_time64 defined when compiling with rustc.

@ViktorWb
Copy link
Author

Did you re-test this with ESP IDF < 5? On ESP IDF < 5 it should give you 4, not 8.

Indeed for v.4.4.6 it gives

i64 alignment: 8
time_t alignment: 4
esp32-camera time_t alignment: 4
esp OS time_t alignment: 4
esp_idf_svc time_t alignment: 4
ffi long long alignment: 8

@ivmarkov
Copy link
Collaborator

@igrr You are totally right!

In this spec we have for i64: i64:64, and for i128: i128:128. I think we should instead have - for compat with GCC -

  • i64: i64:32
  • i128: i128:32

@MabezDev I think we should fix this in all xtensa targets.

@ivmarkov
Copy link
Collaborator

@ViktorWb Can you try compiling on ESP IDF 5+ with this JSON target instead
(I generated it with rustc -Z unstable-options --print target-spec-json --target xtensa-esp32-espidf and then applied the fix from above ^^^):

{
  "arch": "xtensa",
  "cpu": "esp32",
  "crt-objects-fallback": "false",
  "data-layout": "e-m:e-p:32:32-i64:32-i128:32-n32",
  "emit-debug-gdb-scripts": false,
  "env": "newlib",
  "linker": "xtensa-esp32-elf-gcc",
  "linker-flavor": "gnu-cc",
  "llvm-target": "xtensa-none-elf",
  "max-atomic-width": 32,
  "os": "espidf",
  "panic-strategy": "abort",
  "relocation-model": "static",
  "target-family": [
    "unix"
  ],
  "target-pointer-width": "32",
  "vendor": "espressif"
}

Here, the relevant info w.r.t. custom targets.

@ViktorWb
Copy link
Author

ViktorWb commented Jan 21, 2024

Sure! Adding the JSON data to xtensa-esp32-espidf.json in the root of my project and then changing my .cargo/config.toml to:

[build]
target = "xtensa-esp32-espidf.json"

[target.xtensa-esp32-espidf]
linker = "ldproxy"
runner = "espflash flash --monitor" # Select this runner for espflash v2.x.x
rustflags = ["--cfg", "espidf_time64"]

[unstable]
build-std = ["std", "panic_abort"]

[env]
MCU="esp32"
ESP_IDF_VERSION = "v5.1.1"

works! The example given earlier now prints:

i64 alignment: 4
time_t alignment: 4
esp32-camera time_t alignment: 4
esp OS time_t alignment: 4
esp_idf_svc time_t alignment: 4
ffi long long alignment: 4

camera_fb_t is now 4-byte aligned:

println!("Alignment: {}", std::mem::align_of::<esp_idf_sys::cam::camera_fb_t>());

gives Alignment: 4.

@ivmarkov
Copy link
Collaborator

Alright! I'll PR the change to the Rust fork delivered with rustup. Until there is a new release of the toolchain with rustup, please use the JSON target.

@ViktorWb
Copy link
Author

Fantastic, thank you!

@ivmarkov
Copy link
Collaborator

Closing as we now have this PR opened

@ivmarkov
Copy link
Collaborator

Just closed my PR because - as it turned out - there is an earlier (and more correct in that it treats the f64 type too) PR here.

me-no-dev added a commit to espressif/esp32-camera that referenced this issue Jan 22, 2024
me-no-dev added a commit to espressif/esp32-camera that referenced this issue Jan 22, 2024
* Align the frame buffers to the structure alignment 

cc: esp-rs/esp-idf-sys#278
cc: esp-rs/rust#195

* Include stdalign.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants