mozilla::CondSet<T> to squelch maybe-uninitialized variable warnings

NEW
Unassigned

Status

()

Core
MFBT
--
enhancement
3 years ago
a year ago

People

(Reporter: zwol, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
We have a *lot* of "variable X may be used uninitialized" warnings in a typical build -- according to "mach warnings-summary", second only to the flood of spurious "unused local typedef" warnings caused by third party code faking static_assert().

We habitually ignore these warnings, but they're really quite dangerous: the compiler is telling us that there is at least one static control flow path on which the variable is used uninitialized, and it can't prove that that path is dynamically impossible.  As part of the patches I'm about to post, I went through all of these warnings in the layout/ directory, and in several cases, *I* couldn't prove to myself that the path was dynamically impossible, either.  If a variable does get used uninitialized, especially if it's a pointer variable, very bad things can happen.

On the other hand, there's a strong argument for *not* adding default initializations to these variables, at least not without thinking about it carefully: it can hide places where you need to change the code in the future (when something changes such that the "maybe" is now a "definitely").

The patch series I'm about to post adds a template to mfbt/ which allows us to have our cake and eat it too.  If you have

    type variable;

and it's throwing "maybe used uninitialized" errors, simply change it to

    CondSet<type> variable;

This squelches the warning and auto-injects a (debug-only, fatal) assertion at the point of use.  I have gone to some trouble to minimize overhead, especially in optimized builds.  I'll post two patches that demonstrate usage; with both of them applied, layout/ is completely warnings-free in a debug build (Linux, GCC 4.9).

This should be considered an RFC; there are some debatable design decisions in here and it's altogether possible that the whole approach could be better.
(Reporter)

Comment 1

3 years ago
Created attachment 8448929 [details] [diff] [review]
1032994-1-condset.diff

This patch is the "meat" of the project -- it adds the new header "mozilla/CondSet.h" and mozilla::CondSet<T>.
(Reporter)

Comment 2

3 years ago
Created attachment 8448930 [details] [diff] [review]
1032994-2-layout-maybeuninit-restructure.diff

This patch fixes a small number of maybe-uninitialized warnings in layout which do *not* need CondSet<T>; code restructuring is enough.
(Reporter)

Comment 3

3 years ago
Created attachment 8448933 [details] [diff] [review]
1032994-3-layout-maybeuninit-condset-simple.diff

This patch uses CondSet<T> to squelch all the remaining "simple" maybe-uninitialized warnings in layout.  By "simple" I mean anything which is a variation on the classical "conditional set, conditional use" pattern that compilers often get wrong:

   T var;
   if (cond) {
     var = value;
   }
   ...
   if (cond) {
     do_something(var);
   }

I annotated many, but not all, of these with notes on the control path that leads to an uninitialized use and whether it appears to my human brain to be impossible.  Where I didn't, it was because I was confused.  Additional kibitzing welcome.
(Reporter)

Comment 4

3 years ago
Created attachment 8448934 [details] [diff] [review]
1032994-4-layout-maybeuninit-condset-outparam.diff

This patch squelches the remaining, "not simple" cases in layout.  These are all related to inline functions in nsStyleSet.h which may or may not write their out-parameters.  By changing those out-parameters from T& to CondSet<T>&, we force the caller to acknowledge this possibility in a way that the compiler can track.
How does CondSet differ from MFBT's Maybe class?

If we are going to change many function signatures and call sites to use a template class for out-parameters, wouldn't it be cheaper and clearer to just change those call sites to (unnecessarily) initialize the maybe uninitialized variables to null or zero? (I know unnecessary variable initialization is unpopular with many module owners.)
(Reporter)

Comment 6

3 years ago
(In reply to Chris Peterson (:cpeterson) from comment #5)
> How does CondSet differ from MFBT's Maybe class?

Maybe<T> is, AFAICT, geared for class types that you may or may not need, don't want to construct unless necessary, but also want to put on the stack.  It's not transparent, all uses of the object must also be changed.

CondSet<T> is geared for scalar quantities and is transparent.

> If we are going to change many function signatures and call sites to use a
> template class for out-parameters, wouldn't it be cheaper and clearer to
> just change those call sites to (unnecessarily) initialize the maybe
> uninitialized variables to null or zero?

This way you get debug assertions if the variable is genuinely used uninitialized.
(Reporter)

Comment 7

3 years ago
I did optimized try builds (linux64 only) with and without the patch.

baseline:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/zackw@panix.com-f7342114c52a/try-linux64/ 

patched:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/zackw@panix.com-11e99077c2a1/try-linux64/

I'm pleased to report that there do not *appear* to be any substantive differences in the compiled libxul.so.  (I may have missed something; as always, there are tons and tons of *meaningless* differences, e.g. reordered functions, slightly different register allocation choices, etc.)

Comment 8

a year ago
Resetting owner to default per Zack's request.
Assignee: zackw → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.