Use #[non_exhaustive] to make the generated sys bindings more forward compatible

[Phantomical]

There are a bunch of changes that are not breaking in C but are breaking in rust. This is inconvenient both for us and for users, since we end up needing to make frequent breaking changes of perf-event-open-sys. This carries on throughout the whole ecosystem and leads to breaking changes for any crate that publicly re-exports types from perf-event-open-sys. This will notably include perf-event, once I get around to upstreaming some of the various changes made in perf-event2.

By making a few structs #[non_exhaustive] we can make it so that most future updates that the kernel is likely to make are not breaking changes on the rust side. This needs to be combined with #48 in order to really avoid having breaking changes.

The main trade-off, of course, is that you can no longer do perf_event_attr { type_: blah, ...Default::default() }. This is a real cost, but I think the benefits of avoid breaking changes are worth it here.

WRT const, you already can't set any of the bitflags on perf_event_attr in a const context. However, you can still create a zeroed perf_event_attr in a const context by using std::mem::zeroed, like this:

const ATTR: perf_event_attr = {
  let mut attr: perf_event_attr = unsafe { std::mem::zeroed() };
  attr.size = 77;
  attr.type_ = blah;
  ...
  attr
};

A quick search on sourcegraph hasn't found anyone actually doing this, so I'm not sure this is much of concern for existing users.

[jimblandy]

My concern about this change stems from wanting perf-event-open-sys to serve as a proper sys crate for the Rust ecosystem as a whole. #[non_exhaustive] isn't something that a user can turn off if it's a problem for them; their only recourse is to generate their own bindings, or rewrite them from scratch.

It also seems to me that forward compatibility is not something that sys crates are generally expected to provide. That should usually be left to higher-level crates.

Regarding const: are you sure there isn't a way to get bindgen to generate const-qualified versions of those bitfield access functions? Rust does support functions that take &mut in const context (playground).

(I was hoping to explain this skepticism about the change before you put in the time to make a PR. I apologize for that.)

[jimblandy]

For what it's worth, crates like linux-perf-event-reader just declare their own version of the struct, with idiomatic Rust types, and then have hand-written code to parse it, in either endianness.

[jimblandy]

#[non_exhaustive] isn't something that a user can turn off if it's a problem for them

Granted, it's also not something a user can turn on if its absence is a problem for them.

[jimblandy]

The alternative I want us to consider is:

• Make higher-level crates like perf-event responsible for providing their own types, rather than exposing the -sys types. This makes for more idiomatic and well-typed Rust APIs. These crates have more design freedom to address the compromises necessary in providing stable APIs. (Note that linux-perf-event-reader exposes only idiomatic Rust types.)

• High-level crates will naturally want to provide "escape hatch" APIs that expose the underlying raw types. (This is in the spirit of the AsRawFd implementations for Counter and Group, for example.) There are two approaches we could take:

  • Exclude escape hatch functions from API stability guarantees.

  • Let users of perf-event specify which version of perf-event-open-sys they would like perf-event to use, as a crate feature.

[Phantomical]

I'm happy to spend some time discussing this PR. In the interest of unblocking changes let's focus on #48 first? That's the one that is blocking me from moving perf-event2 changes upstream, not this PR.

(I was hoping to explain this skepticism about the change before you put in the time to make a PR. I apologize for that.)

This is literally just a cherry-pick of the two commits. No need to worry here :). It's also easier to have the discussion all in one place with a PR.

My concern about this change stems from wanting perf-event-open-sys to serve as a proper sys crate for the Rust ecosystem as a whole. #[non_exhaustive] isn't something that a user can turn off if it's a problem for them; their only recourse is to generate their own bindings, or rewrite them from scratch.

I will freely admit, though, that this part confuses me. It's possible there's something I'm missing here. As far as I can tell the only thing that marking these structs as #[non_exhaustive] prevents you from doing is using struct update syntax to construct them. This is a cost, don't get me wrong, but does not fundamentally prevent anyone using the library from setting whatever fields they want. I also don't think this prevents perf-event-open-sys from serving as a proper sys crate. I also don't want to completely block us from making breaking changes, I would just like us to do them because we choose to instead of being forced to make new major versions whenever the kernel adds a new field to perf_event_attr.

I will note that the only structs I think really need to be #[non_exhaustive] are perf_event_attr and perf_event_mmap_page, along with their associated anonymous structs. If we're ok with going forward with this then I'll remove the #[non_exhaustive] attribute from everywhere else.

Make higher-level crates like perf-event responsible for providing their own types, rather than exposing the -sys types. This makes for more idiomatic and well-typed Rust APIs. These crates have more design freedom to address the compromises necessary in providing stable APIs. (Note that linux-perf-event-reader exposes only idiomatic Rust types.)

It's worth noting that this doesn't actually solve the problem. If downstream crates want to keep up to date with the current kernel version and also publicly expose their version of perf_event_attr then they will need to either make the same compromises I'm proposing here or make breaking changes at about the same rate as perf-event-open-sys will be. The more likely version is that they just don't update their version of perf_event_attr ever, which isn't great either. Either way, though, this doesn't help us with perf-event.

High-level crates will naturally want to provide "escape hatch" APIs that expose the underlying raw types. (This is in the spirit of the AsRawFd implementations for Counter and Group, for example.) There are two approaches we could take:

• Exclude escape hatch functions from API stability guarantees.
• Let users of perf-event specify which version of perf-event-open-sys they would like perf-event to use, as a crate feature.

This, on the other hand, is an alternative. Not the crate features bit, since that only works when the integration is only via traits, otherwise you need exclusive features. Re-exporting perf_event_attr and just documenting that we may change perf-event-open-sys major versions in a minor release would work. We'll probably occasionally break some downstream crates (e.g. something like perf-event-data) which isn't great, but it is definitely something we could do.

[jimblandy]

As far as I can tell the only thing that marking these structs as #[non_exhaustive] prevents you from doing is using struct update syntax to construct them.

The spec for #[non_exhaustive] says:

Non-exhaustive variants (struct or enum variant) cannot be constructed with a StructExpression (including with functional update syntax).

So it forbids both field struct expressions (S { x, y }) and functional update syntax (S { x, ..base }. ) The first is the one that worries me the most.

[jimblandy]

In the interest of unblocking changes let's focus on #48 first? That's the one that is blocking me from moving perf-event2 changes upstream, not this PR.

Sure thing - I've run out of time today, but I'll try to take a look tomorrow.

[Phantomical]

So it forbids both field struct expressions (S { x, y }) and functional update syntax (S { x, ..base }. ) The first is the one that worries me the most.

I suppose I'm less worried about this because outside of something like linux-perf-event-reader I can't actually think of a use case where someone would want to do perf_event_attr { <all the fields> }. So I'm OK with trading that off to make things nicer for everyone else. Provided, of course, that we aren't actually making anything impossible, just mildly more inconvenient.

[jimblandy]

Given the nature of perf_event_attr, it does seem like the most common style of use is going to be to pick out the fields one is interested in and ignore the rest.

The only kind of code that I can see wanting to deal with every last field would be something like that linux-perf-event-reader code: as they build their idiomatic Rust struct, they want to match exhaustively on the sys crate's perf_event_attr struct to get compile-time assurance that they've handled everything.