-
Notifications
You must be signed in to change notification settings - Fork 13k
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
UB free test for CString Drop #36607
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The benchmark was in |
Source code: https://gist.github.com/matklad/87a77a7670b1b9144e09b9c4e62ea00f Two runs without
two runs with volatile write (this PR):
Will try to add a non volatile write bench later. Is there a faster way to build stdlib than |
Here's the one with non-volatile write (no perceivable difference with volatile):
|
2bba914
to
cf2879c
Compare
Also it may make sense to compare these to the cost of the allocation, because the overhead kinda amortizes between |
I don't think it's worth using a volatile write to try to fix this test, it's just an opportunistic thing we do and if LLVM decides to remove it then that seems like a good thing. (it won't do so in debug mode, especially if we If the test causes problems then I think it's safe to remove, it's probably not worth having a super elaborate test case for this. |
The test works fine without
Yes indeed. So I'll add |
Remove CString drop test. The test relies on the undefined behavior, and so may fail in some circumstances. This can be worked around by stubbing a memory allocator in the test, but it is a bit of work, and LLVM could still theoretically eliminate the write of the zero byte in release mode (which is intended). So let's just remove the test and mark the function as inline. It shouldn't be optimized away when inlined into the debug build of user's code. Supersedes rust-lang#36607 r? @alexcrichton
This implements an UB free (I hope :) ) test for #36264. Thanks to @petrochenkov and @nagisa for advice.
It also switched to
volatile_write
for zeroing per @Amanieu suggestion, which should prevent LLVM from optimizing our efforts away. @nagisa could you re-run your benchmark with this implementation to see if anything has changed?