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

[FR] Large File Support #1725

Closed
oToToT opened this issue Jan 3, 2024 · 5 comments · Fixed by #1726
Closed

[FR] Large File Support #1725

oToToT opened this issue Jan 3, 2024 · 5 comments · Fixed by #1726

Comments

@oToToT
Copy link
Contributor

oToToT commented Jan 3, 2024

Is your feature request related to a problem? Please describe.
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

Google benchmark looks to be using libc functions like fseeko, ftello, fopen. (IIUC, std::ifstream may results in using these functions.)
On some "small" platforms, it might result in EOVERFLOW when dealing with large files.

Describe the solution you'd like
A clear and concise description of what you want to happen.
Compiles with these three flags enabled:
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE.

this is the portable/POSIX method for transparently using 64bit types everywhere. for details on each define, see:
# http://www.gnu.org/s/libc/manual/html_node/Feature-Test-Macros.html
# _LARGEFILE_SOURCE: enable support for new LFS funcs (ftello/etc...)
# _LARGEFILE64_SOURCE: enable support for 64bit variants (off64_t/fseeko64/etc...)
# _FILE_OFFSET_BITS: default to 64bit variants (off_t is defined as off64_t)

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

N/A

Additional context
Add any other context or screenshots about the feature request here.

https://issuetracker.google.com/issues/201531268

@oToToT
Copy link
Contributor Author

oToToT commented Jan 3, 2024

(I can work on this.)

oToToT added a commit to oToToT/benchmark that referenced this issue Jan 3, 2024
oToToT added a commit to oToToT/benchmark that referenced this issue Jan 3, 2024
@dmah42
Copy link
Member

dmah42 commented Jan 3, 2024

have you seen this issue anywhere? ifstream is only used in context of opening sysinfo files ("/proc/cpuinfo" and similar) none of which should be "large". ofstream is used to write out files and i suppose these could become large for larger projects, but maybe not on smaller platforms (as they couldn't run such large projects, i suppose).

@vapier
Copy link
Member

vapier commented Jan 3, 2024

LFS isn't just about reading the content of large files. inodes can be 64-bit which means any attempt to stat will fail too. and that can be tricky to reproduce as it's up to the underlying filesystem whether/when it returns an inode # that doesn't fit into 32-bits. enabling LFS costs basically nothing.

i'll also note that in order to support 64-bit time_t, you'll need to enable LFS eventually anyways.

oToToT added a commit to oToToT/benchmark that referenced this issue Jan 3, 2024
@oToToT
Copy link
Contributor Author

oToToT commented Jan 3, 2024

Thanks @vapier for clearer information and sorry for my initial vague arguments.

IIUC, for 64-bit time_t support (to prevent Y2K38), we only need to add -D_TIME_BITS=64 to the compile flag if LFS is already there.
If we want to do so, I can adjust #1726 to include it (and rename it to "Enable Large-file Support + 64-bit time_t").

@vapier
Copy link
Member

vapier commented Jan 3, 2024

let's keep the two separate for now. LFS has been around for a long time and should be easy enough. 64-bit time_t is a newer project and still semi-influx.

dmah42 added a commit that referenced this issue Jan 4, 2024
* Enable Large-file Support

This should fix #1725

* Use whitespaces instead of tab in BUILD.bazel

---------

Co-authored-by: dominic <510002+dmah42@users.noreply.github.com>
afq984 added a commit to afq984/benchmark that referenced this issue Jan 5, 2024
Follow up of google#1725.

`defines` propagates to reverse dependencies, while `local_defines`
don't. If we use `defines` then there's risk of ODR violation:

Suppose a user have a cc_library foo that depends on bar and benchmark:

    cc_library(name = "foo", deps = [":bar", "@com_github_google_benchmark//:benchmark"])

And bar has a class that has LFS-dependant ABI:

    cc_library(name = "foo")

    class Bar {
        off_t member;
    };

Bar would be compiled without LFS, but linked to foo when assuming LFS is enabled.

So we limit LFS to within the library only. benchmark does not have
LFS dependant public ABIs so it should be fine.
dmah42 pushed a commit that referenced this issue Jan 5, 2024
Follow up of #1725.

`defines` propagates to reverse dependencies, while `local_defines`
don't. If we use `defines` then there's risk of ODR violation:

Suppose a user have a cc_library foo that depends on bar and benchmark:

    cc_library(name = "foo", deps = [":bar", "@com_github_google_benchmark//:benchmark"])

And bar has a class that has LFS-dependant ABI:

    cc_library(name = "foo")

    class Bar {
        off_t member;
    };

Bar would be compiled without LFS, but linked to foo when assuming LFS is enabled.

So we limit LFS to within the library only. benchmark does not have
LFS dependant public ABIs so it should be fine.
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 a pull request may close this issue.

3 participants