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

Add statx #1181

Closed
wants to merge 7 commits into from
Closed

Add statx #1181

wants to merge 7 commits into from

Conversation

ariasuni
Copy link

Fix #1178

According to this website, syscall number for statx on arm64 should be 291 but it’s already the value of SYS_syscalls, is it an error?

pub const SYS_syscalls: ::c_long = 291;

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ariasuni
Copy link
Author

I don’t understand from the log why some of the Travis builds failed, especially since one of them is the exact platform I use and did the test on (Rust stable, x86_64-unknown-linux-gnu).

Note that I still got, before and after my changes, some errors (which might be related to a more recent than average glibc?):

bad ucontext_t size: rust: 936 (0x3a8) != c 968 (0x3c8)
bad NFT_MSG_MAX value at byte 0: rust: 22 (0x16) != c 25 (0x19)

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 23, 2018

especially since one of them is the exact platform I use and did the test on

You are probably just running the tests on a Linux version that differs from the one in libc's Docker container.

@ariasuni
Copy link
Author

@gnzlbg do I need to change something, and if yes, what?

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 23, 2018

@ariasuni the "problem" is that the linux version on your machine, and the linux version that libc "supports" / "targets" are slightly different, which is why we see these errors in CI, but you don't see them in your machine.

The error message is not 100% clear, but it basically tells you that:

  • bad NFT_MSG_MAX value at byte 0: rust: 22 (0x16) != c 25 (0x19), that is, the value of the first byte of NFT_MSG_MAX is 0x16 in libc, but should be 0x19 for CI. So that needs to be changed.

  • bad ucontext_t size: rust: 936 (0x3a8) != c 968 (0x3c8), that is, the size of ucontext_t differs between the one in libc and the one in CI. This can be due to many factors, you might want to check the linux headers of the version of linux used in libc's ci in ci/docker/x86_64-unknown-linux-gnu, and check whether that type is missing some fields, or whether it has some C attributes that might affect its alignment, field padding, etc.

I might have some time tomorrow to look into these in more detail, but in the mean time that should get you started. If you have docker installed, you can "reproduce" libc's CI on your linux machine by using the ci/run_docker.sh script. Check the .travis.yml file for how to invoke it (you need to in general just pass it the TARGET=x86_64-unknown-linux-gnu and that's it).

@ariasuni
Copy link
Author

Note that the errors I get locally are very different from the ones on the CI, and I should probably not change things that are working on CI and not related to my changes.

@bors
Copy link
Contributor

bors commented Jan 3, 2019

☔ The latest upstream changes (presumably #1195) made this pull request unmergeable. Please resolve the merge conflicts.

@ariasuni ariasuni force-pushed the add-statx branch 2 times, most recently from 33cca7e to 8d91a9b Compare January 3, 2019 14:03
@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 21, 2019

Any progress on this?

@ariasuni
Copy link
Author

Well I don’t think I can change anything without more feedback. I just don’t break what’s already passing on the CI and would like someone to confirm my addition doesn’t add errors and is complete (I am mostly unsure about the syscall numbers, see my first comment).

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 22, 2019

According to this website, syscall number for statx on arm64 should be 291 but it’s already the value of SYS_syscalls, is it an error?

It is often best to just check out the source of the kernel itself. While there are "generic" syscall numbers, each architecture often has different ones. For arm64, this file (https://github.com/torvalds/linux/blob/f1c2f8857c5aa6c92aa903bc06437503422e5925/arch/arm64/include/asm/unistd32.h#L818) shows that:

#define __NR_statx 397

the syscall number for statx for arm64 targets might be 397 (i'm not very familiar with the kernel - each arch appears to define these in a different way - AFAIK syscalls can have different number in different architectures).

The easiest way to be sure is to just write a small program that performs the syscall and tests whether it behaves correctly.

Note that the errors I get locally are very different from the ones on the CI

To reproduce the CI environment locally (same version of the OS, libraries, etc.), if you are using Linux, you can just use the same Docker containers that libc uses, e.g., ./ci/run_docker.sh x86_64-unknown-linux-gnu should do that (otherwise check the .travis.yml file to see how the run_docker script is invoked).

If you are not using Linux, your best bet is to just modify the PR and inspect the output on travis. As mentioned above, the travis error messages tell you which byte value differs and which value each byte should have, so you can go from the decimal number to hex, change the appropriate hex number to the value on travis, convert back to decimal, and modify the PR.

If you need interactive help, I am on Rust's Zulip and Discord, and there are also a bunch helpful users there that can help if you get stuck.

@ariasuni
Copy link
Author

Sadly I don’t have a ARM64 machine where I could try this syscall, and I’m not familiar enough with the Linux kernel to understand everything (though I tried to skim through the Linux source code first).

Also, all the builds that are not allowed to fail, fail with error: invalid application of 'sizeof' to incomplete type 'struct statx' and lot of related errors, and I don’t know how to fix this.

@bors
Copy link
Contributor

bors commented Jan 23, 2019

☔ The latest upstream changes (presumably #1228) made this pull request unmergeable. Please resolve the merge conflicts.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 3, 2019

I'm closing this due to lack of activity, please feel free to reopen if you want to keep working on this.

@gnzlbg gnzlbg closed this Mar 3, 2019
@ariasuni ariasuni deleted the add-statx branch October 2, 2021 23:05
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.

4 participants