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

fix alignment for SendEventDest #230

Merged
merged 1 commit into from Jun 8, 2023
Merged

fix alignment for SendEventDest #230

merged 1 commit into from Jun 8, 2023

Conversation

wez
Copy link

@wez wez commented Jun 8, 2023

Use the implementation from #225 for this, rather than reimplementing the serialization logic inline.

refs: #229
refs: wez/wezterm#3838

Use the implementation from rust-x-bindings#225
for this, rather than reimplementing the serialization logic inline.

refs: rust-x-bindings#229
refs: wez/wezterm#3838
@rtbo
Copy link
Collaborator

rtbo commented Jun 8, 2023

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

wez commented Jun 8, 2023

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 rtbo merged commit dbdaa01 into rust-x-bindings:main Jun 8, 2023
2 checks passed
@rtbo
Copy link
Collaborator

rtbo commented Jun 8, 2023

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

wez added a commit to wez/xcb-imdkit-rs that referenced this pull request Jul 6, 2023
wez added a commit to wez/wezterm that referenced this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants