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.
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
Unify definitions of
siginfo_t
,statvfs
andstatfs
inmusl
targets #3261Unify definitions of
siginfo_t
,statvfs
andstatfs
inmusl
targets #3261Changes from all commits
6c0952e
e1ff5d6
adcc84d
fb7785a
b196045
d210d71
ca7eedd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could probably make the struct
#[repr(align(...))]
to get ride of the old hackThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately
sizeof::<usize>()
is 4 on 32-bit systems and 8 on 64-bit systems, so the#[repr(align(...))]
needs to be different between those two bit-sizes. You can't write#[repr(align(usize))]
(which would be cool) so you'd have to do some conditional attrs and the hack feels easier to parse.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
align = usize
would be nice, that seems worth proposing at some point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like s390x actually has its own implementation too,
unsigned
vsunsigned long
https://github.com/kraj/musl/blob/ffb23aef7b5339b8c3234f4c6a93c488dc873919/arch/s390x/bits/statfs.h#L4There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, 32-bit seems to have padding fields https://github.com/kraj/musl/blob/ffb23aef7b5339b8c3234f4c6a93c488dc873919/arch/x32/bits/statfs.h#L4. Any idea why this isn't a problem for us currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
s390x
case is simple, https://refspecs.linuxbase.org/ELF/zSeries/lzsabi0_s390.html#SCALAR shows thatunsigned int
andunsigned long
are the same size on that platform.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
x32
target is not the intel/AMD 32-bit target (that's ini386
and other folders in musl), it's this https://sites.google.com/site/x32abi/. Rust supportsgnux32
but doesn't supportmuslx32
(yet?) so we don't need to worry about this weird architecture. The padding fields are necessary on the x32 target since we're passing this structure to a 64-bit kernel so we need to shuffle the 32-bit user-space fields into the appropriate places in the kernel structure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks for the clarification!