Even nicer errors from assert_unsafe_precondition by saethlin · Pull Request #103035 · rust-lang/rust (original) (raw)

I just remembered that I have a conflict with the libs meeting, so I'm writing a lot more here. This is not so much directed at you, as it is my effort to make sure these points come up at/before a meeting.

The best user experience here involves displaying the invalid values alongside the location of the bug in the user's code, as we do in panic messages for things like array index out of bounds. Certainly I would like to have that kind of behavior because it would let me debug the code of others directly from the error message, as I often do with Miri diagnostics.

I have no idea if that is a possibility though, because of a concern about code size. But we already substantially regressed code size in #102732, the failure branch for one of these assertions is now (on x86_64):

cc79:       50                      push   %rax
cc7a:       48 8d 3d 7f 53 04 00    lea    0x4537f(%rip),%rdi        # 52000 <anon.0248cb95e8b925b4d7128f8507660a7d.0.llvm.3434341789188788945>
cc81:       be 1c 00 00 00          mov    $0x1c,%esi
cc86:       ff 15 1c 4f 06 00       call   *0x64f1c(%rip)        # 71ba8 <_GLOBAL_OFFSET_TABLE_+0x2a0>
cc8c:       0f 0b                   ud2

The previous failure path was just a 2-byte ud2, but now we are 21 bytes if I'm reading that right. The code size implications of passing the arguments to these unsafe fn to a diverging function are hard for me to speculate about because that call may interfere with other optimizations.

There are other strategies for producing a static but human-readable error message here; for example we could have every invocation of assert_unsafe_precondition! expand to a #[cold] function which takes no explicit parameters and just dispatches to panic_str_nounwind. From our current state, that would reduce code size where we use assert_unsafe_precondition!, but it's not clear to me that it couldn't grow the size of executables.

Recently, I tried profiling these checks and I identified two micro-optimizations for the two hottest checks: #103287 #103285 I suspect both of those have code size implications, but exactly what they are is not at all straightforward, because LLVM can (and as best I can tell by squinting at assembly, actually does) optimize away parts of these checks, for some callers. The implications of those optimizations on code size are entirely unclear to me, for example do the code size savings there open up the possibility to spend a code size budget elsewhere?

So I think if we're going to keep making decision on the basis of code size here, I want to do that with data. I know that Oxide has at least in the past compiled and run a bootloader with these debug assertions enabled, and found UB that way: rust-embedded/heapless#280 so I'm sure they care about code size at some level. And of course there are plenty of other embedded Rust users who may have even more stringent code size requirements. Are those users even enabling debug assertions? Embedded users of debug assertions may be able to at least provide an example codebase whose size they care about, and some context about what kinds of regressions would be concerning; we could keep an eye on that to at least have some idea.

In terms of concrete maintenance burden, we have had #100759 which touched every use of this macro, and I expect we will have the occasional PR along the lines of #103329 to add these assertions for unsafe fn that we forgot about or only recently decided to do assertions for, or new API additions such as #99136.