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

RFC: Implement Debug, Eq, PartialEq, and Hash for libc structs #2235

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@Susurrus

Susurrus commented Dec 6, 2017

Rendered

CC previous discussion holders from rust-lang/libc#302: @alexcrichton @kamalmarhubi @dimbleby

@Centril Centril added the T-libs label Dec 6, 2017

@kamalmarhubi

This comment has been minimized.

Contributor

kamalmarhubi commented Dec 6, 2017

I was unable to macrologize my way to the feature gated version though I don't recall why not. Iam strongly in favour of sorting this out. The rust API guidelines were not as fleshed out at the time, and now they provide more motivation to do this.

Questions:

  • what are the backwards compatibility issues with adding these derives?
  • can we quantify the compile time cost of deriving this on all libc structs?
  • are there cases where custom Debug impls would be required?
  • are there cases where Debug should not even be implemented?
@Susurrus

This comment has been minimized.

Susurrus commented Dec 7, 2017

Addressing your questions in turn:

What are the backwards compatibility issues with adding these derives?

I don't believe there should be any. These types shouldn't change size with these traits implemented on them. And there won't be a conflict from the traits being implemented multiple times, because they only be implemented within this crate.

Can we quantify the compile time cost of deriving this on all libc structs?

Yes we can. Or did you mean should we quantify this before accepting this RFC?

Are there cases where custom Debug impls would be required?

Yes, the RFC addresses this. And this will also occur for PartialEq and Hash as well.

Are there cases where Debug should not even be implemented?

I don't know what this question is driving at. Is this a technical question or a utility question? I think the utility of Debug is pretty apparent at the struct level, but at the field level these 256, 512, and 1024 byte arrays will likely not have much utility when output as part of debugging output. That being said, who are we to restrict this. It might be useful for downstream users. And I can't think of a technical reason why we shouldn't implement Debug for as many types as possible barring the overall dilemma of long compile times.

@Susurrus

This comment has been minimized.

Susurrus commented Dec 7, 2017

Regarding the long compile times as well, as part of a test we could remove all arrays that are too long from all internal datatypes and then derive all of these traits and directly compare the compile times of the crate from scratch and then when also modifying a single struct. This should give us a pretty good sense of the damage there.

@dimbleby

This comment has been minimized.

dimbleby commented Dec 21, 2017

Back in the rust-lang/libc#302 I noted that:

  • winapi does #[derive(Debug)] wherever it can

However as of 0.3 this is no longer true - I guess this change in policy was made to improve compile times.

It's terribly frustrating that we feel we have to hobble our code because the compiler is slow.

It's true that - from the individual crate's point of view - the user experience may be made better by not implementing otherwise desirable traits. But the Rust world would be so much improved if compile times could be improved so that this was a non-issue...

Therefore, I'm of the opinion that we should anyway derive Debug, etc. if we think that's the right thing for the code to do, and compile times be hanged. If that slows things down enough to annoy half of the ecosystem - well, that'll help to focus energy on making the compiler faster!

(I'm only half-joking. Rhetoric aside, it is a fact that as of today libc and winapi both don't provide many automatically derived traits. Draw your own conclusions.)

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Feb 3, 2018

LGTM, I think this can be merged as is.

However, implementation-wise, I am strongly against adding manual trait implementations to libc. They complicate the implementation, raise the barrier of entry, and increase the maintenance burden when things do change.

I think that after merging what can be done is derive the traits for all types for which this can happen automatically. That is already a huge improvement and better than nothing.

For all other types: I think time is better spent fixing deriving traits for arrays. We don't need to ship this RFC all at once. We can do this incrementally.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 7, 2018

@Susurrus do you know what the concrete impact would be on compile times to add these implementations by default? Optionally as well? I think it'd be good to evaluate the weight of the drawback here

@Susurrus

This comment has been minimized.

Susurrus commented Feb 8, 2018

@alexcrichton No I don't. I mentioned testing this above in a previous comment

@Susurrus

This comment has been minimized.

Susurrus commented Feb 8, 2018

So I implemented a quick test on x86_64-unknown-linux-gnu. The compile time tripled when commenting out all struct fields that couldn't auto-derive Debug, Eq, Hash, and PartialEq.

Just did a test here locally. I checked out master (bbda50d20937e570df5ec857eea0e2a098e76b2d).

$ cargo clean
$ cargo build
   Compiling libc v0.2.36 (file:///home/susurrus/Projects/libc)
warning: #[derive] can't be used on a non-Copy #[repr(packed)] struct (error E0133)
   --> src/macros.rs:41:22
    |
41  |               #[derive(Debug, Eq, Hash, PartialEq)]
    |                        ^^^^^
    | 
   ::: src/unix/notbsd/mod.rs
    |
13  | / s! {
14  | |     pub struct sockaddr {
15  | |         pub sa_family: sa_family_t,
16  | |         pub sa_data: [::c_char; 14],
...   |
179 | |     }
180 | | }
    | |_- in this macro invocation
    |
    = note: #[warn(safe_packed_borrows)] on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

warning: #[derive] can't be used on a non-Copy #[repr(packed)] struct (error E0133)
   --> src/macros.rs:41:33
    |
41  |               #[derive(Debug, Eq, Hash, PartialEq)]
    |                                   ^^^^
    | 
   ::: src/unix/notbsd/mod.rs
    |
13  | / s! {
14  | |     pub struct sockaddr {
15  | |         pub sa_family: sa_family_t,
16  | |         pub sa_data: [::c_char; 14],
...   |
179 | |     }
180 | | }
    | |_- in this macro invocation
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

warning: #[derive] can't be used on a non-Copy #[repr(packed)] struct (error E0133)
   --> src/macros.rs:41:39
    |
41  |               #[derive(Debug, Eq, Hash, PartialEq)]
    |                                         ^^^^^^^^^
    | 
   ::: src/unix/notbsd/mod.rs
    |
13  | / s! {
14  | |     pub struct sockaddr {
15  | |         pub sa_family: sa_family_t,
16  | |         pub sa_data: [::c_char; 14],
...   |
179 | |     }
180 | | }
    | |_- in this macro invocation
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>

    Finished dev [unoptimized + debuginfo] target(s) in 2.36 secs

Then after resetting a clean compile took:

$ cargo clean
$ cargo build
   Compiling libc v0.2.36 (file:///home/susurrus/Projects/libc)
    Finished dev [unoptimized + debuginfo] target(s) in 0.93 secs
@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Feb 8, 2018

Is this something we want to enable in an opt-in way anyways?

If so, let's just add a feature to libc that we disable by default. That way those who want to implement or test this can add it without affecting others, and their work is not lost (that is, these things can be upstreamed to libc).

Once we reach close to complete implementations on tier 1, somebody just has to provide a table of compile-times for all tier 1 platforms here, with and without auto-deriving traits. And we will be able to make a more informed decision about whether to enable this by default for everybody or not. But in any case, those who need it will still be able to use it by opt-in into it, so if they put in some work to help here this work won't be lost.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 8, 2018

@Susurrus I'd imagine the compile time doesn't change much with an opt-in feature, but can you confirm that? If so then I think we should switch to an opt-in feature by default and I think we can land this.

@Susurrus

This comment has been minimized.

Susurrus commented Feb 9, 2018

That is a good question when talking about this as a feature, are we going to make this opt-in or opt-out. I'd suggest opt-out so that the largest number of people will use use it and the people who get slow compiles can opt-out as needed.

I'll whip something up in the next few days, I didn't save my work from the previous tests for this. I wasn't expecting there to be so much implementation for an RFC to actually do the implementation!

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Feb 9, 2018

@Susurrus I would make it opt-in at first (do not enable it by default, but extend the build scripts to test this configuration). By making it opt-in the feature is initially cfg'ed out, so it should have zero-impact in compile-times.

We can then see how much enabling the feature impacts libc's build times. Libc compiles pretty fast, so if it has a 2-3x compile-time increase, then that might still be ok. However, if it has a 100x compile-time increase, then that would not be ok. We would all agree that this would then be a technical problem to be solved in rustc, but until the problem is solved we wouldn't be able to enable the feature by default without making Rust worse for everybody using it. In any case, those who want to use the feature would still be able to do so.

@Susurrus

This comment has been minimized.

Susurrus commented Feb 12, 2018

So I just tested this locally with an opt-out feature flag, and it's within the noise margin for a build without any of these changes. So if we use an off-by-default feature flag for this, there should be minimal cost.

I'll also push a branch for my testing that has this code and link to it from the RFC so people can confirm my findings.

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Feb 12, 2018

@Susurrus What's the compile-time cost when these are enabled? Could you provide any numbers?

@Susurrus

This comment has been minimized.

Susurrus commented Feb 12, 2018

@gnzlbg I already posted those numbers earlier in this thread.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Feb 12, 2018

@Susurrus my worry with an opt-out flag is that it'll basically be impossible to opt-out for heavily depended on crates like libc, where opt-in would increase the chanes of you being able to opt-out

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Feb 12, 2018

So it goes from 0.9s to 2.3s, I think that's acceptable and that we should try to enable this by default once the feature is done.

@Susurrus Susurrus force-pushed the Susurrus:libc_derives branch 2 times, most recently from 0181bff to 7567306 Mar 3, 2018

@Susurrus

This comment has been minimized.

Susurrus commented Mar 3, 2018

Alright, I've revised this RFC a bit and tried to put all suggestions under the Alternatives section. Please take a look and let me know any other comments.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Mar 5, 2018

Ah sorry I think I missed the original location of where the compile times were measured, did that also include optimized builds?

@Susurrus

This comment has been minimized.

Susurrus commented Mar 10, 2018

@alexcrichton Could you be more specific? It's pretty easy for me to run compilation tests with my libc PoC branch. What specific commands would you like me to time?

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Mar 11, 2018

@Susurrus just try them all. Supposing that the feature that conditionally enables deriving impls is called derive_impls, then that would be:

without std, debug, w/o deriving impls

cargo build --no-default-features
cargo build --no-default-features --features derive_impls

without std, release, w/o deriving impls

cargo build --no-default-features --release
cargo build --no-default-features --release --features derive_impls

with std, debug, w/o deriving impls

cargo build --no-default-features --features use_std
cargo build --no-default-features --features=use_std,derive_impls

with std, release, w/o deriving impls

cargo build --no-default-features --release --features use_std
cargo build --no-default-features --release --features=use_std,derive_impls

Maybe you could add these results to the RFC so that people don't have to scan the thread to look for them.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Mar 12, 2018

@gnzlbg's suggestions are exactly (and more!) what I would have recommended!

@Susurrus Susurrus force-pushed the Susurrus:libc_derives branch from 7567306 to c5a82cf Apr 1, 2018

@Susurrus

This comment has been minimized.

Susurrus commented Apr 1, 2018

Alright, cleaned it up a bit more. Makes the clear argument for a singular default-off feature flag for these types. Let see what you guys think!

@Susurrus

This comment has been minimized.

Susurrus commented Apr 1, 2018

@asomers You might also find this RFC interesting as it affects nix.

@Susurrus

This comment has been minimized.

Susurrus commented Jun 8, 2018

I believe all issues are addressed with this RFC. Is there anything I can do to push this along. If this is blocking on other people, any idea when this might get looked at. I understand things are busy with the 2018 edition, but given the size and scope of this RFC I wanted to see if anyone could take a look at this enough to move it along.

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Jun 8, 2018

@Susurrus I also believe all issues are addressed.

@alexcrichton did you check the last commit? It contains the timings that I've requested and they look great: 0.8s for libc without debug impls, and ~2s for libc with the debug impls. That is, enabling the debug impls costs ~1 second of compile time.

Honestly, I don't see any issues with just implementing this on libc as an opt-in feature (disabled by default), since that doesn't impact compile-times at all unless ones opt-into it. Also, for 1 second compile-time, we might just as well enable this by default once the implementation is finished (that is, keep it opt-in at first, but consider changing it to opt out like std if no new problems are discovered by nix in a couple of months).

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Nov 22, 2018

@alexcrichton could we merge this, and @Susurrus could you send a PR to libc ?

@Susurrus

This comment has been minimized.

Susurrus commented Nov 22, 2018

I'll be happy to implement this once this gets approved.

@Centril Centril added the A-libc label Nov 23, 2018

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 27, 2018

Ok sorry for the delay here, and thanks for doing all the analysis! This looks good to me, so I'm going to propose to merge this:

@rfcbot fcp merge

As a slight alternative, perhaps we could have features like:

[features]
trait-debug = []
trait-eq = []
trait-hash = []
traits = ['trait-debug', 'trait-eq', 'trait-hash']

and then eventually we could change default if necessary? That way they can also be selectively opted-in to if necessary

@rfcbot

This comment has been minimized.

rfcbot commented Nov 27, 2018

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@dtolnay

This comment has been minimized.

Member

dtolnay commented Nov 27, 2018

+1 for the approach in #2235 (comment). derive_all is not the right name -- there is no reason to expose in the API whether traits are being derived or implemented by hand.

@rfcbot concern traits = ['trait-debug', 'trait-eq', 'trait-hash']

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Nov 28, 2018

and then eventually we could change default if necessary? That way they can also be selectively opted-in to if necessary

Please note that for a default feature to be disabled, every single dependent crate needs to opt out. For a crate as widely depended-on as libc, adding a default feature is almost indistinguishable from adding always-on functionality without a feature flag.

So the only two options worth considering IMO are:

  • Opt-in / non-default feature flags (with more or less granularity)
  • Don’t bother with feature flags and always have those impls.
@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Nov 28, 2018

@alexcrichton

There does not appear to be full consensus about the "features" situation yet, and it might be impossible to agree on this till everyone can try the implementation and see how it impacts them.

So I would suggest to just leave this as an "Unresolved Question". Whoever implements this (probably @Susurrus ) should just gate this behind one experimental-derive-traits or many experimental-derive-eq|debug|... features at their choosing initially.

Once the implementation is merged into libc, we should all just revisit this and properly evaluate the impact on all targets (for some targets libc is bigger than for others), and make a decision via a mini-FCP in the libc repo (the libs team should sign on this, and we should advertise that in this RFC comments as well so that everybody gets notified and can give it a try).

As @SimonSapin mentions, gating this behind features also has a cost. If the compile-time costs turn out to be as minimal as in @Susurrus PoC, it might not even be worth it to have a feature at all.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Nov 28, 2018

My point is that default features are not effective. Having these impls behind opt-in/non-default features sounds fine to me.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 28, 2018

@gnzlbg my conclusion is the same as @SimonSapin's, we should have these as off-by-default features for now. I agree with @SimonSapin that on-by-default features for libc are practically permanently on-by-default.

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Nov 28, 2018

So I chatted with @alexcrichton on Zulip and they convinced me that we should definitely put this behind a feature. Compile times might be a non-issue right now, but libc continues to grow and it might well be that it grows up to a point were generating all these traits cause problems. If we don't put them behind a feature right now, we'd have to add opt-in "negative" features to disable them in the future while preserving backwards compatibility.

I still don't know how I feel about adding one feature per trait to derive like @dtolnay suggests.

I believe that we would have to at least test that libc builds properly for all these configurations, particularly because some derive(...) interact poorly with some repr features used in C FFI (e.g. one cannot derive Debug for a packed struct, etc.) So I kind of build prefer to have just a single feature for all these derives - it might well be that is enough to test libc only once with all features enabled, don't know.

Arguably, @alexcrichton argument applies here as well. If libc grows enough that deriving all traits is a problem, and you just want to use one, then this all-or-nothing approach is not perfect. But this is something that AFAICT we can fix in the future in a backwards compatible way, by changing traits = [] to traits = [ "trait-eq", "trait-debug", ...] and adding these new features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment