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

Additional fix for 32-bit systems. #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mr-c
Copy link

@mr-c mr-c commented Mar 29, 2024

Thank you for a476126 ; I had to add the contents of this PR as well to fix the 32-bit builds of Phylonium for Debian.

Possibly this was also needed because we define -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 on armel/armhf?

@kloetzl
Copy link
Collaborator

kloetzl commented Mar 29, 2024

Thanks for your continued effort on this. Could you give me the error you get on arm? I was certain that with my change it should have been fixed.

@kloetzl
Copy link
Collaborator

kloetzl commented Apr 12, 2024

Ah, so I think the problem here is the following. On 32bit systems saidx64_t is bigger than size_t. Hence the shift is larger than the underlying type and produces a warning. (Not sure why debian gets a build failure without seeing the error. I hope they don't compile with -Werror?) The better fix would be to shift left depending on min(sizeof(size_t), sizeof(saidx64_t)).

Then there is also the more subtle issue of trying to read a sequence that doesn't fit into memory/size_t. Not sure what is the best way to prevent that.

@kloetzl
Copy link
Collaborator

kloetzl commented May 11, 2024

Hi, I've pushed my version of a fix. Could you check if that solves it the Debian side?

@mr-c
Copy link
Author

mr-c commented May 11, 2024

@kloetzl f1463b7 ? Okay, I'll do a test later this weekend, thanks!

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.

2 participants