refactor: fix definition of LFS64-related types in emscripten#5142
Open
dybucc wants to merge 1 commit into
Open
refactor: fix definition of LFS64-related types in emscripten#5142dybucc wants to merge 1 commit into
dybucc wants to merge 1 commit into
Conversation
Contributor
Author
|
Style check fails for reasons outside the changes introduced in this PR. The formatting checks seem |
The definitions for types that were made available under the LFS64 spec were being exposed even when the right feature test macro for that is now `FILE_OFFSET_BITS64`. This has been patched by deprecating the current definition when building without the existing `gnu_file_offset_bits64` `cfg` option. Even though the definitions are exposed when both the afore mentioned macro and the `_GNU_SOURCE` macro are defined, no `cfg` has been added for the latter.
21a8c5d to
9cdfb82
Compare
Collaborator
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses bit-width representation issues under emscripten concerned with the LFS64 "spec."
The current interface we expose unconditionally makes the symbols available, even though the
upstream musl fork for emscripten does define the
_GNU_SOURCEfeature test macro by default. Thishas been patched by conditionally compiling whenever it is that the
gnu_file_offset_bits64cfgoption is detected at build time.
All of the routines concerned with LFS64 were already in a module of their own but were always
exposed even though they are not always available in emscripten. According to [1], the
_LARGEFILE64_SOURCEfeature test macro is considered "deprecated" and the gated functionalityshould instead be provided by setting
_FILE_OFFSET_BITS=64. At present, thelibccrate aleadyhas a
cfgfor that. Still, the definition in emscripten (and in upstream musl) also requires the_GNU_SOURCEfeature test macro to be enabled. This symbol is not made available by default inLinux unless either explicitly defined in source code or as part of the compilation pipeline, and
makes available a number of GNU extensions. The
libccrate does not have at the time of writingany
cfgs gating this functionality, but this PR does not introduce it. Further discussion on thismay be necessary.
This patch deprecates all such symbols (both suffixed routines and types) unless the
cfgoption isset. These changes, though, may prove to be useless. That's why no greater effort has been made into
fixing the exposed interface, unless further confirmation from somebody else is provided. Tests
don't pass with the module availability changes as there's overlapping definitions between parent
modules to the
emscriptenmodule, and those in theemscriptenmodule itself. This is caused bythe modified link names of a few routines when running under
gnu_file_offset_bits64. Were this PRto be viable, further changes will be made to fully fix this.
Sources
Upstream emscripten sources showing both the final
alltypes.hheader with theoff_tdefinitionand the input to the
sedscript that generates it.Upstream emscripten fork of musl showing what has been assumed to be the "main" definition of
off64_tand related types when both the_GNU_SOURCEand_LARGEFILE64_SOURCEfeature testmacros are present.
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI