PSA: Avoid invoking Debug formatters in release-mode Rust

[Bobby Holley]

TL;DR: To prevent code bloat, avoid {:?} in format strings for panic!(),
unreachable!(), error!(), warn!(), and info!() for Rust code that ships in
Gecko.

Longer version:

One nice thing about Rust is that you can #[derive(Debug)] for a type, and
the compiler will generate a stringification method, which may in turn be
inductively defined in terms of the Debug implementations of member types.
This is great for logging and debugging, but the implementations can be
quite large depending on the types involved.

The linker will generally eliminate unused Debug implementations as dead
code, but can't do so if they might be invoked in release builds. The most
common way this seems to happen is in panic!() messages, where it can be
tempting to include a stringified value to make the message more
informative. It can also happen for the logging macros that don't get
compiled out of release builds, which (at least for stylo) are info!(),
warn!(), and error!() [1].

To demonstrate what's at stake here, this trivial patch eliminates more
than 80K from libxul: https://github.com/servo/servo/pull/19756

Given how easy it is to mess this up and pull tons of unnecessary code into
Firefox, and given that it's rather time-consuming to notice the problem
and track down the culprit, I think we're best off categorically avoiding
this pattern.

Comments and alternative proposals welcome.

bholley


[1]
https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/servo/ports/geckolib/Cargo.toml#21

[Lars Bergstrom]

At least for Servo, should we add a check to tidy (
https://github.com/servo/servo/blob/master/python/tidy/servo_tidy/tidy.py)
immediately to catch the use of that fairly-unique formatting string, as we
do for a bunch of other random stuff? We can always exempt particular
files/folder where we think it shouldn't matter, but if it's that easy to
footgun we might as well put some simple guards in place.
- Lars

PS - welcome back!

On Fri, Jan 12, 2018 at 11:07 PM, Bobby Holley <bobby...@gmail.com>
wrote:

> _______________________________________________
> dev-servo mailing list
> dev-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-servo
>

[Anthony Ramine]

I would much rather prefer if we just checked that we didn't use the Debug impls of large types. We could also just not derive them in release mode.

In the PR you link, AFAICT you also removed some uses that were just very small enums or even integers, which may not be necessary.

[Anthony Ramine]

That being said, looking at nsCSSParser.cpp in Gecko, I just realised you never had similar code (printing values when some invariant is broken). Given how old Gecko is, I guess that means this is not even useful to begin with (I mean the printing you removed was not useful to begin with, just to be extra clear).

I wouldn't mind a tidy thing choking on the slightest appearance of "{:?}" in those files.

[Xidorn Quan]

On Sun, Jan 14, 2018, at 1:36 AM, Anthony Ramine wrote:
> I would much rather prefer if we just checked that we didn't use the
> Debug impls of large types. We could also just not derive them in
> release mode.
>

> In the PR you link, AFAICT you also removed some uses that were just
> very small enums or even integers, which may not be necessary.

Small enums may still cost extra string, I suppose, and integers should probably just use Display.

I wonder whether it is possible to have a switch to turn off all Debug impl on release mode altogether...

- Xidorn

[Bobby Holley]

(From your next message it sounds like there's no disagreement here, but I
wanted to get the reasoning written down)

On Sat, Jan 13, 2018 at 6:36 AM, Anthony Ramine <n.o...@gmail.com> wrote:

> I would much rather prefer if we just checked that we didn't use the Debug
> impls of large types.

The issue here is that it's difficult to tell at a glance how big a Debug
impl will be, especially at the call site. Generics make this extra
complicated, and even a couple of "small" stringifiers can easily add up.

Moreover, a case-by-case analysis makes it hard to do enforcement with
tooling. That said, if the tooling supports a whitelist, I'm totally fine
with allowing exceptions when we have a good reason. I'm proposing some
tooling in bug 1430678.

> We could also just not derive them in release mode.
>

I believe debug!() statements are type-checked in release-mode compiles, so
this would cause our logging code to break.

>
> In the PR you link, AFAICT you also removed some uses that were just very
> small enums or even integers, which may not be necessary.
>

> > Le 13 janv. 2018 à 06:07, Bobby Holley <bobby...@gmail.com> a écrit :
> >
> > Comments and alternative proposals welcome.
>