[cfe-dev] new -Wuninitialized implementation in Clang

[Nico Weber]

I don't have a strong opinion either way, I figured I'd just give some
feedback on my experience.

I went through the chrome codebase and looked at all the new
uninitialized warnings now reported by -Wuninitialized. Here is a
patch that suppresses them all: http://codereview.chromium.org/6368094

After last week's improvements to -Wuninitialized, about 20 warnings
are still left. Most of them are false positives. The ones that
aren't, are not very interesting or hard to understand. Examples:

http://codereview.chromium.org/6368094/diff/1/base/third_party/dmg_fp/dtoa.cc
clang complains that the variable mlo declared in line 3572 might be
read uninitialized in line 4185 (you can click "Expand all" to see all
the code after the declaration.) This may or may not be true, the
function is very long and full of gotos, and clang doesn't tell me
_why_ it thinks this might be true. My guess was that the goto in line
4040 was taken, and before that one of the two "goto one_digit;". If
this is correct, then this is indeed a false positive because in both
cases mhi is set to 0 and mlo is read only if mhi != 0.


There are many (~10) warnings because of -Wuninitialized not tracking
control dependencies (from what I understand), e.g.
http://codereview.chromium.org/6368094/diff/1/chrome/common/gpu_messages.cc

Yet code that looks very similar does not warn:

    double expiry;

    if (!state->GetBoolean("include_subdomains", &include_subdomains) ||
        !state->GetString("mode", &mode_string) ||
        !state->GetDouble("expiry", &expiry)) {
      continue;
    }


This code (not chrome code), which _does have_ a legitimate
uninitialized read, on the other hand does _not_ warn:

bool g(int* a) {
  return true;
}

int f() {
  int a;
  if (g(&a)) return a;
  return a;
}

My personal impression is that it can be difficult to understand when
this new mechanism warns, and most of the time when it warns, it's
warns about false positives.


To be fair, there are a few real issues that it found:
http://codereview.chromium.org/6368094/diff/1/ipc/ipc_message_utils.cc
This was missing a check, but the warning doesn't go away even with
the correct check because of one of the known false positives
mentioned above :-)

http://codereview.chromium.org/6368094/diff/1/webkit.patch
Here the code is basically:
int var;
if (virtualFunction()) var = 0;
if (virtualFunction()) use(var);
A subclass might return a different result for virtualFunction() on
each call, so this is a valid warning. But when I stored
virtualFunction() in a bool, the warning didn't go away either (again
due to control dependencies?)

This is valid too, I think:
http://codereview.chromium.org/6368094/diff/1/chrome/browser/dom_ui/options/browser_options_handler.cc
After some digging, I learned that chrome's LOG_FATAL can in theory
not terminate the process; but even after I made it always do that,
the warning didn't go away (maybe rightfully so – as I said, it's a
bit hard to understand _why_ these warnings happen.


If it were possible to disable -Wmaybe-uninitialized while keeping
some of -Wuninitialized around, I'd do that for chrome. If that's not
possible, I'd probably try to add the pointless initializations in
this CL and turn on -Wuninitialized.

Have people generally found that this new mechanism reports useful
things in practice?

Nico


On Thu, Feb 3, 2011 at 6:35 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Hi fellow Clangers,
>
> During the last two weeks, I have been working on implementing -Wuninitialized in Clang.  Clang currently doesn't implement this warning, and is a glaring feature deficiency when compared with GCC.
>
> Unlike GCC's implementation, which is based on the backend optimizer, Clang's implementation is based on dataflow analysis in the frontend.  This means the warning still works at -O0.
>
> Now that there is a prototype of this feature in TOT Clang, I wanted to open up the list to general discussion of this feature, its deployment, and what expectations users should have.
>
> For some background, because GCC's implementation of -Wuninitialized is based on the optimizer, the results of the warning can differ depending on the flags passed to the compiler.  For example:
>
> (1) The warnings can vary depending on the optimization level selected.
>
> (2) The warnings can vary depending on the target architecture.
>
> (3) The warnings can vary depending on which version of GCC you are using.
>
> While I am not 100% certain, I also suspect that GCC's implementation is not completely sound, and will not flag warnings in some cases.  My hypothesis is that this is done to avoid spurious false positive warnings, leaving the impression that the analysis is smarter than it actually is (while possibly missing real issues).
>
> My design goal for -Wuninitialized was threefold:
>
> (a) Works under -O0.  Users care about seeing these warnings the most when they are doing their debug builds.
>
> (b) Has consistent results that are invariant of the optimization flags, target architecture, phases of the moon, etc.
>
> (c) Provides predictable results that are (for the most part) sound and complete.
>
> (d) Has marginal impact on compile time.
>
>
> The last three goals mean that the analysis can only do limited reasoning about control-dependencies, e.g.:
>
>  int x;
>  ...
>  if (flag)
>    x = ...
>  ...
>  if (flag)
>    use(x);
>
> Inherently analyzing this code correctly requires path-sensitive analysis, which inherently has exponential cost in the general case.  There are tricks where we can mitigate such algorithmic complexity for some common cases, but handling these control-dependencies in general is something that really is in the purview of the static analyzer.  Amazingly, GCC often doesn't flag warnings in such cases, but I suspect that it is because GCC is silently dropping warnings in some cases where it deems it can't accurately reason precisely about a given variable.
>
> My proposal is that Clang's analysis errs on the side of producing more warnings instead of worrying about such control-dependencies.  This means that for the above code example that Clang would emit a warning, even when no use of an uninitialized variable is possible.  My rationale is twofold:
>
> (a) The cost of initializing a variable is usually miniscule.
>
> (b) Users get predictable results, and the compiler doesn't play games when deciding when to emit a warning in the face of control-dependencies that it cannot reason about.
>
> *** Question #1 ***
>
> Is this a reasonable level of behavior we can set for this warning that users will accept?  I have received reports from Nico and Chandler that initially this warning produced copious warnings on Chrome, but now my understand that the number of warnings is down to 11 (which seems quite manageable to me, given that 2 of the issues were cases that GCC didn't flag).
>
> As a bonus, Clang's warnings also include Fixit hints to the user on how to initialize the variable to silence the warning.  With the proper editor support, I think responding to Clang's warnings requires minimal effort from the developer.
>
> For examples of what Clang's -Wuninitialized warns about, we have several tests available now in the clang test suite:
>
>  test/Sema/uninit-variables.c
>  test/SemaObjC/uninit-variables.m
>  test/SemaCXX/uninit-variables.cpp
>
> *** Question #2 ***
>
> Another point I want to raise (which was brought up by Chandler) is whether or not uninitialized variables checking should continue to be under the -Wuninitialized flag?  If we go with the behavior that I propose, we are deviating from GCC's behavior with a more "sound" (but noisy) analysis.  Since several checks in clang are bundled under the -Wuninitialized flag, there may be some argument to splitting these under separate flags.
>
> Is there a general desire to do this?  If so, why?
>
> My personal feeling is that things should be kept simple: keep a single -Wuninitialized flag that turns on all uninitialized variables checking.  Users can then either suppress the warning by initializing their variables (which I argue is cheap and overall good for code cleanliness) or use pragmas to shut up the warning entirely in regions of their code.  The latter may be a gross solution, but it is there for those who want it.
>
> Thoughts?  Comments?
>
>
>
> Cheers,
> Ted
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>