On Wed, Mar 23, 2016 at 1:17 AM, Ted Mielczarek <t...@mielczarek.org> wrote: > On Tue, Mar 22, 2016, at 06:51 PM, Brian Smith wrote: >> On Tue, Mar 22, 2016 at 3:03 AM, Henri Sivonen <hsivo...@hsivonen.fi> >> wrote: >> >> > It seems that the Rust MP4 parser is run a new Rust-created thread in >> > order to catch panics. >> > >> >> Is the Rust MP4 parser using panics for flow control (like is common in >> JS >> and Java with exceptions), or only for "should be impossible" situations >> (like MOZ_CRASH in Gecko)? >> >> IMO panics in Rust should only be used for cases where one would use >> MOZ_CRASH and so you should configure the rust runtime to abort on >> panics. >> >> I personally don't expect people to write correctly write unwinding-safe >> code—especially when mixing non-Rust and Rust—any more than I expect >> people >> to write exception-safe code (i.e. not at all), and so abort-on-panic is >> really the only acceptable configuration to run Rust code in. > > I think I agree with this assessment.
I do, too, but I was initially shy to suggest that we shouldn't be able to have Rust subsystems that catch panics within themselves. Since we don't seem to have a compelling use cases for such Rust subsystems, since Rust code that intermingles with C++ should clearly abort on panic, since it seems that it's not easy for a panic handler to tell which case it's dealing with and since compiling without unwinding support results in a smaller code footprint. > We'd just also like to make sure > that the specific way that the Rust code aborts triggers our Breakpad > exception handler, as we've had problems with this in the past (calling > abort() does not reliably do so, except in Gecko code where we override > the symbol), hence the repeated refrain of "MOZ_CRASH". I think for release builds, we should have the following: 1) Rust panic!() causes a crash that's MOZ_CRASH()-compatible for crash-reporting purposes. (See https://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#269 and particularly https://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#184 ) 2) All Rust code in Gecko, even std, is compiled without unwinding support. 3) The panic!() reason strings should not end up in the binary. (Neither reason strings in std nor reason strings from elsewhere.) #1 Is important so that we learn if something that wasn't supposed to be able to happen happened and we see where it happened. If there's anything to learn about ICU, I think we should try to get the footprint benefits of not having unwinding code and not having the reason strings in the binary *before* Fennec product management asks whether there's a way to make the APK size impact of the first Rust code smaller. For debug builds, passing the panic!() reason string to MOZ_CRASH() would be a nice touch, but since the panic!() reason strings are not zero-teminated and MOZ_CRASH() expects zero-terminated reason strings, creating a zero-terminated copy of a string when terminating might not be a good idea. Dunno. (Of course, in theory, rustc could special-case literals as panic!() reasons and add a zero-terminator when compiling in whatever mode Gecko ends up using.) Right now, https://github.com/rust-lang/rfcs/pull/1513 doesn't seem to accommodate the above points. I've cross-posted there. On Wed, Mar 23, 2016 at 7:38 PM, Ralph Giles <gi...@mozilla.com> wrote: > so unwinding through FFI is > something we need to think about. I'd much prefer we didn't have to think about it. Since the semantics of panic!() are "game over", it makes sense to map it to MOZ_CRASH() to remove the possibility of cross-FFI unwinding. -- Henri Sivonen hsivo...@hsivonen.fi https://hsivonen.fi/ _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
