Skip to content

BUG: Use large file fallocate on 32 bit linux platforms#25631

Merged
mattip merged 1 commit intonumpy:mainfrom
snogge:fallocate64
Jan 24, 2024
Merged

BUG: Use large file fallocate on 32 bit linux platforms#25631
mattip merged 1 commit intonumpy:mainfrom
snogge:fallocate64

Conversation

@snogge
Copy link
Contributor

@snogge snogge commented Jan 19, 2024

In spite of all of the required preprocessor directives set to use UNIX Large File Support, multiarray/convert.c still used the fallocate version with 32bit offset_t, at least with glibc.

glibc provides both 32bit and 64bit fallocate by silently using the fallocate64 symbol instead of fallocate when -D_FILE_OFFSET_BITS=64 is set. Using a local prototype for fallocate instead of the fcntl.h header in multiarray/convert.c circumvented the glibc redirect.

A similar but not identical PR #25630 has been pushed for 1.26.X.

Using local prototypes instead of the header files meant that the
redirects triggered by -D_FILE_OFFSET_BITS=64 were not triggered for
fallocate.

The prototypes in feature_detection_stdio.h are not used since the
switch to meason, and might trigger false positives in the feature
discovery code.
@snogge
Copy link
Contributor Author

snogge commented Jan 23, 2024

Please note that this patch is slightly different from the one in #25630 (meant for 1.26 branch). This patch wont work on 1.26, while the 1.26 version probably works on main.

@ngoldbaum
Copy link
Member

That's fine, @charris can take note after this is merged.

I think this hasn't gotten any review so far because the PR description doesn't contain any context for what the issue is and what exactly this is fixing. There also isn't a test.

@snogge
Copy link
Contributor Author

snogge commented Jan 24, 2024

I've updated the description, I hope that helps.
I don't know how to write a test for this. I discovered the problem when scanning all binaries built with the Yocto Project for files that still used the 32bit symbols even with the LFS defines set globally. I don't know of any actual program that have problems because of this.

@ngoldbaum ngoldbaum added the triage review Issue/PR to be discussed at the next triage meeting label Jan 24, 2024
@ngoldbaum
Copy link
Member

Thanks for the context, I'll make sure this gets brought up at the next triage meeting.

@mattip
Copy link
Member

mattip commented Jan 24, 2024

We discussed this at a triage meeting, reviewed the code as well as we could with no test, and decided it makes sense.

@mattip mattip merged commit aa8def5 into numpy:main Jan 24, 2024
@mattip
Copy link
Member

mattip commented Jan 24, 2024

Thanks @snogge

@snogge
Copy link
Contributor Author

snogge commented Jan 24, 2024

Thank you for quick turnaround.

@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

00 - Bug triage review Issue/PR to be discussed at the next triage meeting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants