Re: [dev-servo] PSA: Avoid invoking Debug formatters in release-mode Rust
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
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
(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
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
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