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 devstat items #2565

Merged
merged 2 commits into from
Dec 3, 2021
Merged

Add devstat items #2565

merged 2 commits into from
Dec 3, 2021

Conversation

GuillaumeGomez
Copy link
Member

No description provided.

@rust-highfive
Copy link

r? @JohnTitor

(rust-highfive has picked a reviewer for you, use r? to override)

pub tk_nin: ::c_long,
pub tk_nout: ::c_long,
pub dinfo: *mut devinfo,
pub snap_time: ::c_double,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be noted: this is supposed to be a long double, but I have absolutely no idea how to bind such a type... And the C spec is wonderful about it: https://gcc.gnu.org/onlinedocs/gcc/Floating-Types.html

@GuillaumeGomez GuillaumeGomez changed the title Add procstat items Add devstat items Nov 22, 2021
@bors
Copy link
Contributor

bors commented Nov 23, 2021

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

@GuillaumeGomez
Copy link
Member Author

I'm having trouble with errors like:

error: incompatible pointer types returning 'devstat_type_flags *' from a function with result type 'struct devstat_type_flags *'

They are C enums but the comparison considers them as structs? Just in case, the C enums are defined here.

@bors
Copy link
Contributor

bors commented Nov 24, 2021

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

@GuillaumeGomez
Copy link
Member Author

@JohnTitor Any idea about why I'm getting this error when using enums?

@GuillaumeGomez
Copy link
Member Author

Fixed the conflict.

@bors
Copy link
Contributor

bors commented Dec 1, 2021

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

@GuillaumeGomez
Copy link
Member Author

Rebased.

@GuillaumeGomez
Copy link
Member Author

r? @Amanieu

@rust-highfive rust-highfive assigned Amanieu and unassigned JohnTitor Dec 3, 2021
} else {
pub type c_longdouble = [u8; 16];
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you checked that this actually works? I imagine that from a calling-convention point of view, a long double is passed/returned differently from an array of u8.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how to check it? I tried it on the C API and valgrind seemed fine with it but no clue if it isn't just plain luck at this point...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a few tests on godbolt:

  • On x86 & x86_64, long doubles are passed in 80-bit x87 float registers. There is no way to represent this with normal Rust code.
  • On aarch64, long doubles are passed in 128-bit SIMD registers. You can probably emulate this by using uint64x2_t from the (currently unstable) stdarch intrinsics.

In conclusion, I don't think it is practical to bind these API using Rust at the moment. I would recommend using a wrapper written in C to convert these values to a normal double.

In the longer term, I plan on adding arch-specific types such as f80, f128, f16, etc to stdarch, but this is unlikely to happen any time soon.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make it private and remove the double functions using long double and keeping it for the statinfo struct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might work, but you need to make sure that you have the correct size/alignment for c_longdouble on all arches. My recommendation would be to search for LongDouble[Size|Align] in clang/lib/Basic/Targets/ to get the correct values for all targets that FreeBSD supports.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only test FreeBSD on one arch? :o

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only test it on x86_64. But we do test that it builds on x86_64, i686, aarch64 and powerpc64.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean: are the types actually checked on the arch you mentioned or is it simply running a cargo build?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The types are only checked on x86_64 I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's pretty bad. :-/

@GuillaumeGomez
Copy link
Member Author

@Amanieu In the end, I simply commented everything related to long double. For the time being, I think it's the easiest solution unfortunately.

@Amanieu
Copy link
Member

Amanieu commented Dec 3, 2021

I would prefer if the code was properly deleted, it's messy to leave commented code around. Also, the build.rs changes related to long double should also be removed.

@GuillaumeGomez
Copy link
Member Author

Ok, done as well.

libc-test/build.rs Outdated Show resolved Hide resolved
@Amanieu
Copy link
Member

Amanieu commented Dec 3, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Dec 3, 2021

📌 Commit ac6e16b has been approved by Amanieu

@bors
Copy link
Contributor

bors commented Dec 3, 2021

⌛ Testing commit ac6e16b with merge 92b4680...

@bors
Copy link
Contributor

bors commented Dec 3, 2021

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: Amanieu
Pushing 92b4680 to master...

@bors bors merged commit 92b4680 into rust-lang:master Dec 3, 2021
@GuillaumeGomez GuillaumeGomez deleted the procstat branch December 3, 2021 19:05
@lnicola
Copy link
Member

lnicola commented Dec 15, 2021

Hello! As you might know, rust-analyzer is included as a submodule in rust-lang/rust.

We generally try to keep our dependencies up to date, and one of those is libc, so we often use a newer version than rustc itself. This isn't a problem, but sometimes libc implements new APIs for FreeBSD and requires linking to extra libraries, which are not available in the rust-lang/rust CI image. I've seen this happen for libkvm, libprocstat and libdevstat, see for example rust-lang/rust#91909.

Adding new APIs is fine, of course, but would you mind pinging me next time you link to a new library so I can update the FreeBSD toolchain script? Even if we don't do this for RA, it still needed before upgrading libc in rust-lang/rust. Thanks!

EDIT: ideally we'd run FreeBSD on CI, but I'm not sure how to do it, and it's still not easy to keep our list of libraries in sync with what rust-lang/rust has.

bors added a commit that referenced this pull request Jan 28, 2022
Gate PartialEq and Eq on freebsd objects behind extra_traits

This fixes the failure in rust-lang/rust#93351 (comment).

These derives were recently added in #2565. Other PartialEq/Eq derives in the project (and this file) are all behind the `extra_traits` gate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants