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

Align trampoline to nearest 16 bytes to prevent crash #78

Closed
wants to merge 1 commit into from

Conversation

bottiger1
Copy link
Contributor

@bottiger1 bottiger1 commented May 31, 2024

Not having the trampoline aligned by 16 bytes seems to cause random crashes/memory corruption on 32bit. I did not run into any weird issues like this on 64bits with a bunch of hooks strangely.

I was trying to figure out why it was crashing and after staring at the assembly for an entire day, I finally thought about alignment. The 2 hooks I was having problems with had trampoline sizes of 15 bytes.

@bottiger1
Copy link
Contributor Author

bottiger1 commented Jun 1, 2024

Never mind it seems like it was memory corruption caused by something else which this randomly happened to bypass. However I've read that it is recommended to align jumps to 16 bytes for speed purposes so maybe this is still worth doing?

@angelfor3v3r
Copy link
Collaborator

Never mind it seems like it was memory corruption caused by something else which this randomly happened to bypass. However I've read that it is recommended to align jumps to 16 bytes for speed purposes so maybe this is still worth doing?

I'm unsure about the 16-byte alignment stuff (I assume CPU will fetch up to a cache line anyway), but your changes here are making the CI checks fail, somehow. Those will have to be fixed before a review happens.

@bottiger1 bottiger1 closed this Jun 4, 2024
@A1mDev
Copy link

A1mDev commented Dec 16, 2024

Why is PR closed? I think the problem is still relevant - shqke/sourcetvsupport#48

@angelfor3v3r
Copy link
Collaborator

Why is PR closed? I think the problem is still relevant - shqke/sourcetvsupport#48

If someone has a nice way to test this and can make a PR which passes CI, then a PR can be opened and reviewed. I personally don’t have a way to test this (I primarily target x86-64 Windows and didn’t have much issue with 32-bit in the past, but I’m unsure of linux). Seems like the original PR creator closed it. If you have more details or a way to reproduce it, that’ll be great!

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.

3 participants