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.

Sign up for GitHub

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

Jump to bottom

avoid unaligned loads / stores #93

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

avoid unaligned loads / stores #93

wants to merge 2 commits into from

Conversation

sezero
Copy link
Contributor

@sezero sezero commented Feb 24, 2023

Closes #88

@ccawley2011, @AliceLR, @sagamusix: Please review.

@AliceLR
Copy link
Contributor

AliceLR commented Feb 24, 2023

Will review soon when I get a chance.

AliceLR
AliceLR reviewed Feb 24, 2023
View reviewed changes
src/load_med.cpp Outdated Show resolved Hide resolved
AliceLR
AliceLR reviewed Feb 24, 2023
View reviewed changes
src/load_wav.cpp Outdated Show resolved Hide resolved
@sezero sezero closed this Feb 24, 2023
@sezero
Copy link
Contributor Author

sezero commented Feb 24, 2023

Re-opening after fixing accidental screw-up.

@sezero sezero reopened this Feb 24, 2023
@sezero
Copy link
Contributor Author

sezero commented Feb 25, 2023

@AliceLR: Pushed f93b255 to load_med.c.

And yes: The other places are crawling with similar issues.

AliceLR
AliceLR approved these changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@AliceLR AliceLR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems about as fine as it can be without a much larger overhaul.

src/load_med.cpp
Comment on lines -634 to +635
UINT nbo = pmsh->songlen >> 8;
UINT nbo = READ_BE16(pmsh + offsetof(MMD0SONGHEADER,songlen));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't a change request, but I felt like pointing out the old version here broke my brain a little bit :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. There are some other similar hacks there, all changed to use READ_BE??

@ccawley2011
Copy link

Am I correct in thinking that this PR is meant to fix big endian compatibility as well?

Also, there's a Windows CE workaround in src/libmodplug/sndfile.h that can be removed now.

@sezero
Copy link
Contributor Author

sezero commented Feb 27, 2023

This seems about as fine as it can be without a much larger overhaul.

Other loaders will need similar changes, but yes it will be a much larger patch

@sezero
Copy link
Contributor Author

sezero commented Feb 27, 2023

Am I correct in thinking that this PR is meant to fix big endian compatibility as well?

Yes (even though the comments in there say that it is big-endian-fixed)

Also, there's a Windows CE workaround in src/libmodplug/sndfile.h that can be removed now.

Looks like it

@glebm
Copy link

glebm commented Mar 20, 2023

Can we get this, #88, and #92 in please for portability?
Does anyone other than @Konstanty have merge access?

@AliceLR
Copy link
Contributor

AliceLR commented Mar 20, 2023

Can we get this, #88, and #92 in please for portability?

This patch fully replaces #88 I think, but otherwise yes, these ought to be merged ASAP.

Does anyone other than @Konstanty have merge access?

I think only @Konstanty can merge.

glebm added a commit to glebm/od-buildroot that referenced this pull request Mar 24, 2023
@glebm
[od] add libmodplug patches
68f7be0 
These are patches for libmodplug that fix various UB issues.
They come from these PRs to the upstream repository:
1. Konstanty/libmodplug#92
2. Konstanty/libmodplug#93

The upstream maintainer has not merged any PRs in over a year.

Fixes OpenDingux#105.
@glebm glebm mentioned this pull request Mar 24, 2023
Merged
glebm added a commit to glebm/od-buildroot that referenced this pull request Mar 24, 2023
@glebm
[od] add libmodplug patches
8a74499 
These are patches for libmodplug that fix various UB issues.
They come from these PRs to the upstream repository:
1. Konstanty/libmodplug#92
2. Konstanty/libmodplug#93

The upstream maintainer has not merged any PRs in over a year.

Fixes OpenDingux#105.

Signed-off-by: Gleb Mazovetskiy <[email protected]>
pcercuei pushed a commit to OpenDingux/buildroot that referenced this pull request May 6, 2023
@glebm @pcercuei
[od] add libmodplug patches
80a6c80 
These are patches for libmodplug that fix various UB issues.
They come from these PRs to the upstream repository:
1. Konstanty/libmodplug#92
2. Konstanty/libmodplug#93

The upstream maintainer has not merged any PRs in over a year.

Fixes #105.

Signed-off-by: Gleb Mazovetskiy <[email protected]>
@carstene1ns carstene1ns mentioned this pull request Oct 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AliceLR AliceLR AliceLR approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sezero @AliceLR @ccawley2011 @glebm