search

rust-x-bindings / rust-xcb

Rust bindings and wrapper for XCB.
MIT License
161 stars 63 forks source link

fix alignment for SendEventDest #230

Closed wez closed 1 year ago

wez commented 1 year ago

Use the implementation from https://github.com/rust-x-bindings/rust-xcb/pull/225 for this, rather than reimplementing the serialization logic inline.

refs: https://github.com/rust-x-bindings/rust-xcb/issues/229 refs: https://github.com/wez/wezterm/issues/3838

rtbo commented 1 year ago

Thanks for this. I understand it is the same issue, but I don't really get it. Why is the alignment different? In my understanding (which is apparently wrong), assigning to a pointer address is the same as copying the bytes to this address.

wez commented 1 year ago

This sort of issue is more problematic on non-Intel architectures, as Intel has some stuff that compensates for alignment issues by default. I'm not sure if this panic is "just" Rust being helpful for writing more portable code across architectures or if this is a legit error on Intel. On those other architectures, or on Intel when the processor is set to the strict alignment mode, unaligned accesses generate a fault.

The issue here is with how the memory transfer gets compiled; the underlying word transfer opcode that results from a pointer indirection typically requires that the data be stored at an aligned address for both source and destination, which may not be guaranteed. In wezterm that particular stack trace was for a struct that was constructed on the stack and I suppose (didn't verify) that it doesn't include any padding that would help it to be appropriately aligned for this kind of direct access.

Taking care to add that padding/alignment to the various structs might be another way to resolve this issue, but the simplest and most robust is to use copy_from_slice which internally is able to reason about alignment and switch to byte-wide operations around the edges of the underlying memory copy, but use full machine-word size transfers for the bulk of it, without triggering the alignment concern.

I'd recommend that you take a look at the various serialize operations and adjust them all to use copy_from_slice or layer them on top of other implementations that are using it, so that you can head off any other lingering similar issues!

rtbo commented 1 year ago

Thanks a lot for the explanation.

I'd recommend that you take a look at the various serialize operations and adjust them all to use copy_from_slice or layer them on top of other implementations that are using it, so that you can head off any other lingering similar issues!

Will definitely do that