Re: [dev-servo] PSA: Avoid invoking Debug formatters in release-mode Rust

[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.

> Le 13 janv. 2018 à 15:36, Anthony Ramine  a écrit :
> 
> 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.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Avoid invoking Debug formatters in release-mode Rust

[Chris Peterson]

On 2018-01-12 9:07 PM, Bobby Holley wrote:

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.


Just a friendly reminder: panic messages that are parameterized to 
include debug data might expose PII in Firefox crash reports. Patches 
that add new parameterized panic messages should probably be reviewed by 
a data steward:


https://wiki.mozilla.org/Firefox/Data_Collection
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: [dev-servo] PSA: Avoid invoking Debug formatters in release-mode Rust

[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  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  a écrit :
> >
> > Comments and alternative proposals welcome.
>
> ___
> dev-servo mailing list
> dev-se...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-servo
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: [dev-servo] PSA: Avoid invoking Debug formatters in release-mode Rust

[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 
wrote:

> 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/7fb999d1d39418fd331284fab909df
> 076b967ac6/servo/ports/geckolib/Cargo.toml#21
> ___
> dev-servo mailing list
> dev-se...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-servo
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform