-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
debug_assert to ensure that from_raw_parts is only used properly aligned #52972
Conversation
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
So just to be clear, this can probably not be merged right now because it fails the ripgrep test suite on Windows:
However, I was unable to reproduce this on Linux. Cc @BurntSushi |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Oh dang, we test codegen even with debug assertions and somehow this used to work but now it cannot optimize away these alignment tests? This is basically the inverse problem of not having debug assertions in user builds -- with |
@RalfJung Is there a way to get a full backtrace? I don't think I can do much with the given info. And this is only failing on Windows but not other platforms? I'm trying to think where there might even be a I can't immediately spot the error by inspection, but I can try to debug this later. |
I'd love to have a backtrace.^^ But CI doesn't give us one and indeed I have been unable to reproduce.
So
|
Regarding the codegen issue, I got it to optimize better by adding some
which has a test saying "there must not be a |
@RalfJung It looks like the only implementations of |
Also all of these have I am a little surprised by the |
The return value in my code can be misaligned if the slice is length-0. I'd assumed that was OK. |
Ah, you mean if the inner Now I wonder if |
Okay, |
21b76b7
to
6fb97a6
Compare
I proposed a fix for |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
llogiq/bytecount#42 has been merged. What has to happen for the Rust test suite to pick it up when testing |
We need a new ripgrep release that picks up the new version. Then someone needs to upgrade ripgrep. I'll send a ripgrep PR. |
Once re-compiling std becomes easier, this will help more, so don't be let down by this, I think adding |
I updated the ripgrep commit fetched by cargotest. Is anything else needed for updates like this? Also, there still is the problem with the codegen tests. It seems wrong to expect codegen tests to still pass when libstd has debug assertions turned on. What is the best way to ignore some/call codegen tests when |
We probably want to have a way to disable certain tests when the debug assertions are enabled, by, for example, just adding: // ignore-debug-assertions-on or similar. I don't think compiletest (https://github.com/rust-lang/rust/tree/master/src/tools/compiletest) supports this, but maybe that logic could be added somewhere around here: rust/src/tools/compiletest/src/header.rs Line 600 in c63bb1d
|
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@kennytm informs me that all CI runners have debug assertions enabled, except for the ones building the release artifacts with do not run tests. (So we don't test the actual thing we are shipping? oO) So,
|
@bors: r+ |
📌 Commit 1001b2b has been approved by |
⌛ Testing commit 1001b2b with merge 580ca3e50ef75f129ad5b138e37a1ed3024a8c21... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I guess this is not 9696 since it is a macOS...
|
debug_assert to ensure that from_raw_parts is only used properly aligned This does not help nearly as much as I would hope because everybody uses the distributed libstd which is compiled without debug assertions. For this reason, I am not sure if this is even worth it. OTOH, this would have caught the misalignment fixed by #42789 *if* there had been any tests actually using ZSTs with alignment >1 (we have a CI runner which has debug assertions in libstd enabled), and it seems to currently [fail in the rg testsuite](https://ci.appveyor.com/project/rust-lang/rust/build/1.0.8403/job/v7dfdcgn8ay5j6sb). So maybe it is worth it, after all. I have seen the attribute `#[rustc_inherit_overflow_checks]` in some places, does that make it so that the *caller's* debug status is relevant? Is there a similar attribute for `debug_assert!`? That could even subsume `rustc_inherit_overflow_checks`: Something like `rustc_inherit_debug_flag` could affect *all* places that change the generated code depending on whether we are in debug or release mode. In fact, given that we have to keep around the MIR for generic functions anyway, is there ever a reason *not* to handle the debug flag that way? I guess currently we apply debug flags like `cfg` so this is dropped early during the MIR pipeline? EDIT: I learned from @eddyb that because of how `debug_assert!` works, this is not realistic. Well, we could still have it for the rustc CI runs and then maybe, eventually, when libstd gets compiled client-side and there is both a debug and a release build... then this will also benefit users.^^
☀️ Test successful - status-appveyor, status-travis |
📣 Toolstate changed by #52972! Tested on commit 8928de7. 💔 book on windows: test-pass → test-fail (cc @carols10cents @steveklabnik, @rust-lang/infra). |
Tested on commit rust-lang/rust@8928de7. Direct link to PR: <rust-lang/rust#52972> 💔 book on windows: test-pass → test-fail (cc @carols10cents @steveklabnik, @rust-lang/infra). 💔 book on linux: test-pass → test-fail (cc @carols10cents @steveklabnik, @rust-lang/infra).
This line: (the same error affects the second edition as well) (We also need to update nomicon to remove |
The nomicon is already fixed, but not updated in rustc yet. |
Submitted rust-lang/book#1492 |
…r=alexcrichton Add a debug_assert to Vec::set_len Following the precedent of rust-lang#52972, which found llogiq/bytecount#42. (This may well make a test fail; let's see what Travis says.)
…r=alexcrichton Add a debug_assert to Vec::set_len Following the precedent of rust-lang#52972, which found llogiq/bytecount#42. (This may well make a test fail; let's see what Travis says.)
This does not help nearly as much as I would hope because everybody uses the distributed libstd which is compiled without debug assertions. For this reason, I am not sure if this is even worth it. OTOH, this would have caught the misalignment fixed by #42789 if there had been any tests actually using ZSTs with alignment >1 (we have a CI runner which has debug assertions in libstd enabled), and it seems to currently fail in the rg testsuite. So maybe it is worth it, after all.
I have seen the attribute
#[rustc_inherit_overflow_checks]
in some places, does that make it so that the caller's debug status is relevant? Is there a similar attribute fordebug_assert!
? That could even subsumerustc_inherit_overflow_checks
: Something likerustc_inherit_debug_flag
could affect all places that change the generated code depending on whether we are in debug or release mode. In fact, given that we have to keep around the MIR for generic functions anyway, is there ever a reason not to handle the debug flag that way? I guess currently we apply debug flags likecfg
so this is dropped early during the MIR pipeline?EDIT: I learned from @eddyb that because of how
debug_assert!
works, this is not realistic. Well, we could still have it for the rustc CI runs and then maybe, eventually, when libstd gets compiled client-side and there is both a debug and a release build... then this will also benefit users.^^