(dormant account) Reporter
	Description • 15 years ago

      No description provided.

	Benjamin Smedberg
	Comment 1 • 15 years ago

In particular, class members which are not ininitized in the class initializer list and are also not set in the class constructor. This will require an ESP analysis, I think, since there are some cases where we do:

Foo::Foo()
{
  if (something)
    mA = 1;
  else
    mA = 2;
}

I'd like this to be an error not a warning if we can manage it. Perhaps with a NS_UNINITIALIZED annotation if necessary for the truly few cases where we do want to leave a member uninitialized.

	Jeff Walden [:Waldo]
	Comment 2 • 15 years ago

...and also where the class doesn't have a custom ::operator new, I assume.

	Benjamin Smedberg
	Comment 3 • 15 years ago

You mean a memset(0) operator new? That's a special case we should think about, yeah.

	Jeff Walden [:Waldo]
	Comment 4 • 15 years ago

I was thinking of checking more bluntly to avoid false positives, but sure, something more specific, maybe some form of annotation, is all cool too.

	Varsha J Hegde
	Comment 5 • 14 years ago

This is Varsha from SJCE,Mysore.This javascript detects uninitialized class members and shows error.

	Benjamin Smedberg
	Comment 6 • 14 years ago

Varsha, when you run this on the tree what kind of results do you find?

	Varsha J Hegde
	Comment 8 • 14 years ago

I have attached the result. Please give feedback

	Benjamin Smedberg
	Comment 9 • 14 years ago

Is that result just for js/src?

This is a false-positive:

/home/hegde/src/js/src/jsemit.h:272: error: In class js::detail::HashTable<js::HashMap<JSAtom*, int, js::DefaultHasher<JSAtom*>, js::ContextAllocPolicy>::Entry, js::HashMap<JSAtom*, int, js::DefaultHasher<JSAtom*>, js::ContextAllocPolicy>::MapHashPolicy, js::ContextAllocPolicy> uninitialized field JSTreeContext::decls

JSTreeContext::decls is not a primitive or POD type, it is a JSAtomList which has a constructor. (ditto for lexdeps, and a lot of the stuff in jstracer.h)

/home/hegde/src/js/src/jsemit.h:279: error: In class js::detail::HashTable<js::HashMap<JSAtom*, int, js::DefaultHasher<JSAtom*>, js::ContextAllocPolicy>::Entry, js::HashMap<JSAtom*, int, js::DefaultHasher<JSAtom*>, js::ContextAllocPolicy>::MapHashPolicy, js::ContextAllocPolicy> uninitialized field D_24149

This is also incorrect: the constructor initializes scopeChain(NULL).

	Jeff Muizelaar [:jrmuizel]
	Comment 12 • 9 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> Created attachment 8674308 [details] [diff] [review]
> A sketch of what this could look like

Some problems with this are:
1. It may look at all fields (including base classes) when it should only look at a class's own fields
2. It doesn't handle fields that are default constructable
3. Maybe something else that I forget.

	Andrea Marchesini [:baku]
	Comment 14 • 9 years ago

can I still this bug? I have a patch for it.

	Andrea Marchesini [:baku]
	Comment 15 • 9 years ago

better approach.

	Andrea Marchesini [:baku]
	Comment 16 • 9 years ago

This second patch is useful to mark variables that we know are correctly initialized (ipdl stuff for instance).

	(no longer active)
	Comment 17 • 9 years ago

Comment on attachment 8677602 [details] [diff] [review]
part 1 - clang plugin

Review of attachment 8677602 [details] [diff] [review]:
-----------------------------------------------------------------

This is a great start!

::: build/clang-plugin/clang-plugin.cpp
@@ +1490,5 @@
> +  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
> +
> +  unsigned nonCTORID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Warning,
> +      "%0 should be initialized in an explicit CTOR for %1");

Nit: constructor.

@@ +1495,5 @@
> +
> +  const CXXRecordDecl *decl = Result.Nodes.getNodeAs<CXXRecordDecl>("decl");
> +
> +  // We care just about classes at the moment.
> +  if (decl->getTagKind() != TTK_Class) {

See below.

@@ +1511,5 @@
> +    // We have an implicit CTOR.
> +    if (decl->ctor_begin() == decl->ctor_end()) {
> +      Diag.Report(decl->getLocation(), nonCTORID) << *field << decl;
> +      continue;
> +    }

I'm not quite sure why you need to do this separately.

If the compiler doesn't end up generating an implicit constructor, there is nothing to check!  For a case like:

class Foo {
  int field;
};
void f() {
  Foo f; // Make the compiler generate the ctor
}

you'll find a CXXConstructorDecl that would return true from isImplicit().

Please unify the two checkers.

@@ +1520,5 @@
> +    const MatchFinder::MatchResult &Result) {
> +  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
> +  unsigned nonInitializedMemberID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Warning,
> +      "%0 is not correctly initialialized in the CTOR of %1");

Nit: constructor

@@ +1527,5 @@
> +      Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
> +
> +  // No system headers.
> +  FullSourceLoc loc(ctor->getLocation(), *Result.SourceManager);
> +  if (loc.isInSystemHeader()) {

We usually try to do as much of the filtering on the AST as we can in the matcher.  You can replace this here with unless(isInSystemHeader()) in the matcher.

@@ +1543,5 @@
> +  }
> +
> +  const CXXRecordDecl *decl = ctor->getParent();
> +  if (!decl) {
> +    return;

I think it's better to create a custom matcher for these three checks, similar to isInterestingImplicitCtor(), for example.

@@ +1547,5 @@
> +    return;
> +  }
> +
> +  // We care just about classes at the moment.
> +  if (decl->getTagKind() != TTK_Class) {

Hmm, why?  classes and structs are not different in any way.

If you're trying to cut down the number of things to fix right now, I think it would be better to look for non-POD types first.

@@ +1551,5 @@
> +  if (decl->getTagKind() != TTK_Class) {
> +    return;
> +  }
> +
> +  // For each field, if they are buildinType and or if they are pointer...

Nit: builtin.

@@ +1553,5 @@
> +  }
> +
> +  // For each field, if they are buildinType and or if they are pointer...
> +  CXXRecordDecl::field_iterator field = decl->field_begin(), fieldEnd = decl->field_end();
> +  for (; field != fieldEnd; ++field) {

Nit: please use a range-based for loop, like:

for (auto& field : decl->fields())

@@ +1555,5 @@
> +  // For each field, if they are buildinType and or if they are pointer...
> +  CXXRecordDecl::field_iterator field = decl->field_begin(), fieldEnd = decl->field_end();
> +  for (; field != fieldEnd; ++field) {
> +    QualType type = field->getType();
> +    if (!type->isBuiltinType() && !type->isPointerType()) {

Please add a comment explaining that it's OK to ignore reference types here since they are mandated to be initialized by the language.

@@ +1561,5 @@
> +    }
> +
> +    Stmt* body = ctor->getBody();
> +    if (!body) {
> +      continue;

I think this should be hoisted outside of the loop.

@@ +1566,5 @@
> +    }
> +
> +    // Check if this member is initialized.
> +    CXXConstructorDecl::init_const_iterator init = ctor->init_begin(), initEnd = ctor->init_end();
> +    for (; init != initEnd; ++init) {

Nit: Please use a range-based for loop.

@@ +1579,5 @@
> +    }
> +
> +    // Maybe it's initialized in the body.
> +    Stmt::child_iterator child = body->child_begin(), childEnd = body->child_end();
> +    for (; child != childEnd; ++child) {

Ditto.

@@ +1585,5 @@
> +      if (!isa<BinaryOperator>(*child)) {
> +        continue;
> +      }
> +
> +      BinaryOperator* binOp = dyn_cast<BinaryOperator>(*child);

You don't need the isa check above separately, you can just dyn_cast_or_null() and check the result against null, which should be more efficient.

@@ +1586,5 @@
> +        continue;
> +      }
> +
> +      BinaryOperator* binOp = dyn_cast<BinaryOperator>(*child);
> +      if (binOp->getOpcode() != BO_Assign) {

What about cases like this?

class Foo {
  char str[100];
  Foo() {
    memset(str, 0, sizeof(str));
  }
};

I think you may want to look for MemberExpr's that are lvalue instead of looking for assignments.  What do you think?

@@ +1609,5 @@
> +        break;
> +      }
> +    }
> +
> +    if (child != childEnd) {

With this implementation strategy, you end up doing all of the work for each field.  I think we can be more efficient by collecting all fields in an unordered_set or some such, and then do this work only once, for each member that we find that is correctly initialzied, we'd remove it from the set.  At the end, we can iterate over the set and print out the diagnostic for any remaining fields.  What do you think?

	(no longer active)
	Comment 18 • 9 years ago

Comment on attachment 8677975 [details] [diff] [review]
part 2 - MOZ_IGNORE_INITIALIZATION

Review of attachment 8677975 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Attributes.h
@@ +515,5 @@
>   * MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS: Applies to template class
>   *   declarations where an instance of the template should be considered, for
>   *   static analysis purposes, to inherit any type annotations (such as
>   *   MOZ_MUST_USE and MOZ_STACK_CLASS) from its template arguments.
> + * MOZ_IGNORE_INITIALIZATION: Skip the CTOR initialization check for this

Nit: constructor.

	(no longer active)
	Comment 19 • 9 years ago

Two more things: please issue an error instead of a warning.  Also, please run your patches through clang-format.  Thanks!

	(no longer active)
	Comment 20 • 9 years ago

Closing the bug since bug 1217756 is marked as security and one can easily find the information in that bug by looking at these patches...

	Andrea Marchesini [:baku]
	Comment 21 • 9 years ago

> @@ +1527,5 @@
> > +      Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
> > +
> > +  // No system headers.
> > +  FullSourceLoc loc(ctor->getLocation(), *Result.SourceManager);
> > +  if (loc.isInSystemHeader()) {
> 
> We usually try to do as much of the filtering on the AST as we can in the
> matcher.  You can replace this here with unless(isInSystemHeader()) in the
> matcher.

I tried, but I didn't figure out how to use this unless().

> I think it's better to create a custom matcher for these three checks,
> similar to isInterestingImplicitCtor(), for example.

any particular reason?

> Hmm, why?  classes and structs are not different in any way.

Yes. step by step. Just with this simple code we have 11 patches (and more coming) of fixing...


> > +  // For each field, if they are buildinType and or if they are pointer...
> > +  CXXRecordDecl::field_iterator field = decl->field_begin(), fieldEnd = decl->field_end();
> > +  for (; field != fieldEnd; ++field) {
> 
> Nit: please use a range-based for loop, like:

If I do, then I have to use an external variable to know if the loop is finished or not.


> What about cases like this?
> 
> class Foo {
>   char str[100];
>   Foo() {
>     memset(str, 0, sizeof(str));
>   }
> };

Not supported yet. Follow up?

> With this implementation strategy, you end up doing all of the work for each
> field.  I think we can be more efficient by collecting all fields in an
> unordered_set or some such, and then do this work only once, for each member
> that we find that is correctly initialzied, we'd remove it from the set.  At
> the end, we can iterate over the set and print out the diagnostic for any
> remaining fields.  What do you think?

This method is called when a new constructor is found. There is just 1 loop between all the fields.
But maybe I didn't get what you are suggesting.

The reason why it's warning only, is because I want to fix the main issues, then, when this code is 'stable' enough, we can enable it in error more. How does it sound?

	(no longer active)
	Comment 23 • 9 years ago

(In reply to Andrea Marchesini (:baku) from comment #21)
> > @@ +1527,5 @@
> > > +      Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
> > > +
> > > +  // No system headers.
> > > +  FullSourceLoc loc(ctor->getLocation(), *Result.SourceManager);
> > > +  if (loc.isInSystemHeader()) {
> > 
> > We usually try to do as much of the filtering on the AST as we can in the
> > matcher.  You can replace this here with unless(isInSystemHeader()) in the
> > matcher.
> 
> I tried, but I didn't figure out how to use this unless().

As we talked on IRC, I'd like to know what broke.

> > I think it's better to create a custom matcher for these three checks,
> > similar to isInterestingImplicitCtor(), for example.
> 
> any particular reason?

To make it possible to look at the matcher and figure out what the checker acts on without having to read the code for the checker.

> > Hmm, why?  classes and structs are not different in any way.
> 
> Yes. step by step. Just with this simple code we have 11 patches (and more
> coming) of fixing...

My point is that singling out structs is completely arbitrary.  I don't want to land the analysis in such a shape.  It's OK to do it step by step and land the dependencies before we land the analysis, but the thing that we land in this bug needs to make sense to developers.  Making a struct not go through these checks is not a good solution.

In the other cases where I have landed analyses that break all sorts of things, I've had to land individual fixes for a few weeks before the analysis was ready to land.

> > > +  // For each field, if they are buildinType and or if they are pointer...
> > > +  CXXRecordDecl::field_iterator field = decl->field_begin(), fieldEnd = decl->field_end();
> > > +  for (; field != fieldEnd; ++field) {
> > 
> > Nit: please use a range-based for loop, like:
> 
> If I do, then I have to use an external variable to know if the loop is
> finished or not.

Yes.  :-)  Still better than writing iterators manually.

> > What about cases like this?
> > 
> > class Foo {
> >   char str[100];
> >   Foo() {
> >     memset(str, 0, sizeof(str));
> >   }
> > };
> 
> Not supported yet. Follow up?

Well, not doing this sort of things results in false positives.  Like I said above, I would like the analysis that lands in this bug to be as free from false positives as we can in the cases that it handles.

The other case you need to handle is:

class Foo {
  int field;
  Foo() {
    Init(&field);
  }
};

> > With this implementation strategy, you end up doing all of the work for each
> > field.  I think we can be more efficient by collecting all fields in an
> > unordered_set or some such, and then do this work only once, for each member
> > that we find that is correctly initialzied, we'd remove it from the set.  At
> > the end, we can iterate over the set and print out the diagnostic for any
> > remaining fields.  What do you think?
> 
> This method is called when a new constructor is found. There is just 1 loop
> between all the fields.
> But maybe I didn't get what you are suggesting.

My point was that we're doing things such as looking at the initializer list and the body of the ctor once per field.  Basically what you have now is O(n*(m+k)), n being the number of fields, m the number of initializers and k the number of statements in the ctor body.  I'm asking for a O(n+m+k) solution.

> The reason why it's warning only, is because I want to fix the main issues,
> then, when this code is 'stable' enough, we can enable it in error more. How
> does it sound?

The problem with doing that is that in some directories we treat warnings as errors, so this will break the build in some code but not others.  I really think it's better to not be in a hurry to land this, and fix the stuff it finds and land the analysis once we are happy with it!

	(no longer active)
	Comment 24 • 9 years ago

This should be an interesting read: <http://www.viva64.com/en/b/0354/>

	Andi [:andi] Assignee
	Comment 26 • 9 years ago

This is WIP!!
the order in witch the patches must be applied is self explanatory: patch 2 must be applied after patch 1

Should add support for:
1. vectors, pointers
2. initialization in function where field is passed by reference or address.
3. initialization of structs field by field would be nice.

Now we have all what Andrea did plus support for scenarios like:

class A
{
  int p;
  int x;

  void f() {
    p = 10;
    memset(&x, 0, sizeof(x));
  }

public:
  A() {
    f();
  }
};

	Andi [:andi] Assignee
	Comment 29 • 8 years ago

Added support for
1. pass by address and reference
2. look recursively in each function that gets executed from constructor in order to search for attributions. If function is not member search in it only if it's has in it's signature variables that are passed through reference or address, if it's member search in it no matter what.

Till now we have support for the following scenario:

>>class Test {
>>  int a;
>>  int b;
>>  int c;
>>  int d = 2;
>>  int *e;
>>  int *f;
>>  int g;
>>  int h;
>>
>>  void InitPtrByPtr(int **var) {
>>    *var = nullptr;
>>  }
>>  void InitPtr(int *&var) {
>>    var = nullptr;
>>  }
>>
>>  void InitByMemset(int *var) {
>>    memset(var, 0, sizeof(int));
>>  }
>>
>>  void InitByMemset(int& var) {
>>    memset(&var, 0, sizeof(var));
>>  }
>>
>>  void InitByRef(int& var) {
>>    var = 2;
>>  }
>>  int Init(int *var1, int& var2, int& var3, int *&var4, int **var5, int& var6) {
>>    *var1 = 2;
>>    InitByRef(var2);
>>    InitByMemset(var3);
>>    InitPtr(var4);
>>    InitPtrByPtr(var5);
>>    InitByMemset(&var6);
>>  }
>>  Test() {
>>    Init(&a, b, c, e, &f, g);
>>  }
>>};

	Andi [:andi] Assignee
	Comment 30 • 8 years ago

Added support for binary operator = CallExpr from RHS like:

>>int f(int& o) {
>>  o = 2;
>>  return 1;   
>>}
>>class Test {
>>int a;
>>  Test() {
      int result;
>>    result = f(a);
>>  }
>>}

This sort of expression is used when function f allocated something and return the success factor.

	Andrea Marchesini [:baku]
	Comment 31 • 8 years ago

Comment on attachment 8729568 [details] [diff] [review]
clang-plugin-uninitialized-members-check.patch

Review of attachment 8729568 [details] [diff] [review]:
-----------------------------------------------------------------

When ready, send a review request to ehsan directly. Thanks!

::: build/clang-plugin/clang-plugin.cpp
@@ +147,5 @@
> +  public:
> +    virtual void run(const MatchFinder::MatchResult &Result);
> +  private:
> +    void runThroughBody(Stmt* body,
> +        unordered_map<string, bool>& variablesMap,

strange indentation.

@@ +1089,5 @@
>            .bind("node"),
>        &refCountedCopyConstructorChecker);
> +
> +  astMatcher.addMatcher(recordDecl().bind("class"),
> +                          &nonInitializedMemberChecker);

indentation

@@ +1549,5 @@
> +{
> +  auto varFromMap = resolverMap.find(varName);
> +  if (varFromMap != resolverMap.end()) {
> +    // variable found in resolve map that means we must use it's corespondent
> +    // -> second in variablesMap

Right, but you are not using variablesMap at all in this method.

@@ +1645,5 @@
> +  // resolveMap
> +  bool isValid = false;
> +
> +  if (!method) {
> +    Log("Cannot get FunctionDecl from callExpr");

return; ?

@@ +1656,5 @@
> +        param->getType()->isReferenceType()) {
> +
> +      Expr *argument = callExpr->getArg(i);
> +      if (!argument) {
> +        Log("Cannot get Expr from ArgList");

return; ?

or MOZ_ASSERT?

@@ +1715,5 @@
> +    }
> +    varFromExpr = memberExpr->getMemberDecl()->getName();
> +    updateVarMap(variablesMap, varFromExpr);
> +    return;
> +  } else {

no }else{ after a return.

@@ +1775,5 @@
> +    unordered_map<string, string>& resolverMap)
> +{
> +  static struct externFuncDefinition {
> +    string funcName;
> +    uint8_t  indexInArgList;

2 spaces before indexInArgList;

@@ +1778,5 @@
> +    string funcName;
> +    uint8_t  indexInArgList;
> +  } funcSchemaArray[] = {
> +      // for memset
> +      {"memset", 0}

sometimes you indent with 4 spaces, sometimes 2.

@@ +1873,5 @@
> +        // in StdC like memset
> +        runThroughDefaultFunctions(funcCall, variablesMap, resolverMap);
> +      }
> +    } else {
> +    }

else what? :)

@@ +1904,5 @@
> +    return;
> +  }
> +
> +  // No system headers.
> +  FullSourceLoc loc(decl->getLocation(), *Result.SourceManager);

ehsan suggested to move this check in the definition of the matcher.

@@ +1917,5 @@
> +
> +  // For each field, if they are builtinType and or if they are pointer add
> +  // to table
> +  for (auto field = decl->field_begin(); field != decl->field_end(); ++field) {
> +    // Ignore fields if MOZ_IGNORE_INITIALIZATION is present.

add a test for this, if we don't have it yet.

::: build/clang-plugin/tests/moz.build
@@ +34,5 @@
>  ]
>  
>  DISABLE_STL_WRAPPING = True
>  NO_VISIBILITY_FLAGS = True
> +#FINAL_LIBRARY = 'xul-gtest'

Get rid of this

	Sylvestre Ledru [:Sylvestre]
	Comment 33 • 8 years ago

Andi is taking the lead here.

Ehsan, do you have an eta for the review? (no pressure, just to organize our work)

	(no longer active)
	Comment 34 • 8 years ago

Sorry, I'm currently focusing most of my attention on a different project, so I'm afraid my reviews will starve for a while...

Michael, do you have cycles for this review?

	Nika Layzell [:nika] (ni? for response)
	Comment 35 • 8 years ago

Comment on attachment 8730613 [details] [diff] [review]
clang-plugin-uninitialized-members.diff

Review of attachment 8730613 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +15,5 @@
>  #include "llvm/ADT/DenseMap.h"
>  #include "llvm/Support/FileSystem.h"
>  #include "llvm/Support/Path.h"
>  #include <memory>
> +#include <unordered_map>

Most code in the plugin uses DenseMap<> from llvm rather than std::unordered_map.

@@ +781,5 @@
>  }
> +
> +AST_MATCHER(CXXRecordDecl, isNotInSystemHeader) {
> +  const CXXRecordDecl *decl = Node.getCanonicalDecl();
> +   // No system headers.

Indentation seems weird here

@@ +1081,5 @@
>            .bind("node"),
>        &refCountedCopyConstructorChecker);
> +
> +  astMatcher.addMatcher(cxxRecordDecl(isNotInSystemHeader(), isClassType())
> +      .bind("class"), &nonInitializedMemberChecker);

Why are you treating class X {} differently from struct X {}. Aren't they basically the same except that one has private-by-default members and the other has public-by-default members? I would think this analysis should be based on whether the class/struct has a constructor rather than what keyword was used to define it.

@@ +1556,5 @@
> +      return false;
> +    }
> +    varName =  memberExpr->getMemberDecl()->getName();
> +    return true;
> +  } else {

no need for } else { after a return true.

@@ +1557,5 @@
> +    }
> +    varName =  memberExpr->getMemberDecl()->getName();
> +    return true;
> +  } else {
> +    // case 2 - normal variable declaration

I think normal variable reference would be a better wording

@@ +1577,5 @@
> +        if (!varExp) {
> +          return false;
> +        }
> +        return getVarNameFromExprWithThisCheck(varExp, varName);
> +      } else {

Probably use Expr::IgnoreImplicit() or Expr::IgnoreImpCasts() before the branches.

@@ +1611,5 @@
> +  FunctionDecl *method = callExpr->getDirectCallee();
> +  // optimization for extern function to limit their call only if their,
> +  // arguments are passed by address or reference and are already referenced in
> +  // resolveMap
> +  bool isValid = false;

isValid seems like a strange name for this variable. It seems more like it's a variable to determine if a parameter passed could initialize a value we're interested in. Maybe isInteresting or similar?

@@ +1625,5 @@
> +        param->getType()->isReferenceType()) {
> +
> +      Expr *argument = callExpr->getArg(i);
> +      if (!argument) {
> +        return false;

This seems like a strange return false based on an interaction specific value.

@@ +1672,5 @@
> +      return;
> +    }
> +    varFromExpr = memberExpr->getMemberDecl()->getName();
> +    updateVarMap(variablesMap, varFromExpr);
> +  } else {

This function feels like it could be flattened a lot by some early returns.

@@ +1696,5 @@
> +        Expr *varExp = unaryOp->getSubExpr();
> +        if (!varExp) {
> +          return;
> +        }
> +        return checkValueDecl(varExp, variablesMap, resolverMap);

Returning a `void` value feels weird.

@@ +1701,5 @@
> +      } else {
> +        ImplicitCastExpr *implicitCast =
> +          dyn_cast_or_null<ImplicitCastExpr>(expr);
> +        if (implicitCast) {
> +          Expr *varExpr = implicitCast->getSubExpr();

Probably use Expr::IgnoreImplicit() or Expr::IgnoreImpCasts() before the branches.

@@ +1748,5 @@
> +}
> +
> +void DiagnosticsMatcher::NonInitializedMemberChecker::evaluateExpression(
> +  Stmt *stmtExpr, unordered_map<string, bool>& variablesMap,
> +  unordered_map<string, string>& resolverMap) {

You probably want to strip any implicit expressions before looking at the type of the expression AST node.

@@ +1797,5 @@
> +          }
> +        }
> +      } else {
> +        // we are dealing with some default function that are implemented
> +        // in StdC like memset

We could also be looking at some function which is declared in a different cpp file.

@@ +1807,5 @@
> +void DiagnosticsMatcher::NonInitializedMemberChecker::runThroughBody(
> +  Stmt *body, unordered_map<string, bool>& variablesMap,
> +  unordered_map<string, string>& resolverMap) {
> +
> +  for (auto child = body->child_begin(); child != body->child_end();

for (auto child : body->children()) {

@@ +1830,5 @@
> +  }
> +
> +  // For each field, if they are builtinType and or if they are pointer add
> +  // to table
> +  for (auto field = decl->field_begin(); field != decl->field_end(); ++field) {

for (auto field : decl->fields()) {

@@ +1848,5 @@
> +  }
> +
> +  // loop through all constructors and search for initializations or
> +  // attributions
> +  for (auto firstCstr = decl->ctor_begin(); firstCstr != decl->ctor_end();

for (auto firstCstr : decl->ctors()) {

@@ +1862,5 @@
> +    if (!body) {
> +      continue;
> +    }
> +    // first look to see if it's a C++11 declaration Ex. int a = 2 or a(2)
> +    for (auto init = ctor->init_begin(); init != ctor->init_end(); init++ ){

I think you can use `for (auto init : ctor->inits()) {` which might read nicer

::: build/clang-plugin/tests/TestMemberInitialization.cpp
@@ +18,5 @@
> +  int *e;
> +  int *f;
> +  int g;
> +  int h;
> +  int x; // should trigger error for not being initialized

Add a expected-error {{XXX}} comment here so that the clang tests can detect this error.

@@ +50,5 @@
> +
> +    return 3;
> +  }
> +  Test() {
> +    h = Init(&a, b, c, e, &f, g);

Try making sure that it also works with expressions which construct temporaries with destructors. They create a expression node around the statement to run the destructor, and could cause things to break.

	Andi [:andi] Assignee
	Comment 36 • 8 years ago

So i did the modification suggested by you, except i kept using the unordered_map as we've discussed on irc this is a better approach than using the DenseMap

(In reply to Michael Layzell [:mystor] from comment #35) 
> @@ +50,5 @@
> > +
> > +    return 3;
> > +  }
> > +  Test() {
> > +    h = Init(&a, b, c, e, &f, g);
> 
> Try making sure that it also works with expressions which construct
> temporaries with destructors. They create a expression node around the
> statement to run the destructor, and could cause things to break.

Can you give me an example of this case?

After we clarify this i will re-update the patch.

	Nika Layzell [:nika] (ni? for response)
	Comment 37 • 8 years ago

(In reply to Andi-Bogdan Postelnicu from comment #36)
> So i did the modification suggested by you, except i kept using the
> unordered_map as we've discussed on irc this is a better approach than using
> the DenseMap
> 
> (In reply to Michael Layzell [:mystor] from comment #35) 
> > @@ +50,5 @@
> > > +
> > > +    return 3;
> > > +  }
> > > +  Test() {
> > > +    h = Init(&a, b, c, e, &f, g);
> > 
> > Try making sure that it also works with expressions which construct
> > temporaries with destructors. They create a expression node around the
> > statement to run the destructor, and could cause things to break.
> 
> Can you give me an example of this case?
> 
> After we clarify this i will re-update the patch.

An example would be:

struct Obj {
  ~Obj();
};

int do_something_with_obj(const Obj& o);

struct Test {
  int h;
  Test() {
    h = do_something_with_obj(Obj());
  }
}

The statement containing the assignment is wrapped with another node in the AST to represent the execution of the destructor for the temporary Obj() object. I believe that that could potentially mess up your patch, though I'm not sure. At the least I think it should be in the test.

	Andi [:andi] Assignee
	Comment 38 • 8 years ago

I've checked your example and it's supported, i've also added it in the test header.
Regarding the analysis of functions that are in other cpps, when they get called we have AST nodes but empty bodies, they are resolved during linkage. As a first step i think that we can address this issue later on in a separate bug.

	Andi [:andi] Assignee
	Comment 39 • 8 years ago

I've checked your example and it's supported, i've also added it in the test header.
Regarding the analysis of functions that are in other cpps, when they get called we have AST nodes but empty bodies, they are resolved during linkage. As a first step i think that we can address this issue later on in a separate bug.

	Nika Layzell [:nika] (ni? for response)
	Comment 40 • 8 years ago

Comment on attachment 8740407 [details] [diff] [review]
clang-plugin-uninitialized-members.diff

Review of attachment 8740407 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM other than the minor comments below.

::: build/clang-plugin/clang-plugin.cpp
@@ +1729,5 @@
> +  Stmt *stmtExpr, unordered_map<string, bool>& variablesMap,
> +  unordered_map<string, string>& resolverMap) {
> +  stmtExpr = stmtExpr->IgnoreImplicit();
> +  BinaryOperator *binOp = dyn_cast_or_null<BinaryOperator>(stmtExpr);
> +  if ( binOp && binOp->getOpcode() == BO_Assign) {

weird spacing

@@ +1749,5 @@
> +      FunctionDecl *method = funcCall->getDirectCallee();
> +      if (!method) {
> +        return;
> +      }
> +      if ( method->hasBody() ) {

no space around condition

@@ +1769,5 @@
> +          CXXMemberCallExpr *memberFuncCall =
> +              dyn_cast_or_null<CXXMemberCallExpr>(stmtExpr);
> +
> +          if (memberFuncCall && isa<CXXThisExpr>(
> +              memberFuncCall->getImplicitObjectArgument())) {

I suppose it's fairly uncommon for a member to be initialized by a call to a method on another object, so we don't necessarily need to support that.

@@ +1856,5 @@
> +
> +    // look through the map for unmarked variables and pop error message, also
> +    // reset the ones that are set for the following ctor to add it
> +    for(auto varIt =  variablesMap.begin(); varIt != variablesMap.end();
> +        varIt++) {

Can you not do `for (auto& varIt : variablesMap)` or something similar?

::: build/clang-plugin/tests/TestNoDuplicateRefCntMember.cpp
@@ +1,5 @@
>  class C1 {};
>  
>  class RC1 {
>  public:
> +  RC1() : mRefCnt(0) {}

I don't think these should be necessary, as we ignore classes & structs with no constructors now.

::: build/clang-plugin/tests/TestNonMemMovableStd.cpp
@@ +10,5 @@
>  typedef basic_string<char> string;
> +template<class T, class U>
> +class pair {
> +public:
> +  pair() {} // expected-warning-re 3 {{'{{(.*)}}' is not correctly initialialized in the constructor of 'pair'}}

Probably not necessary as we ignore classes without constructors now.

Also, I'm not sure how expected-warning-re is matching the error produced in clang-plugin.cpp. Can you make sure that this test is being run?

::: build/clang-plugin/tests/TestRefCountedCopyConstructor.cpp
@@ +1,4 @@
>  // Implicit copy construct which is unused
>  class RC1 {
> +public:
> +  RC1() : mRefCnt(0) {}

We ignore classes without constructors now.

::: mfbt/Attributes.h
@@ +526,5 @@
>   *   declarations where an instance of the template should be considered, for
>   *   static analysis purposes, to inherit any type annotations (such as
>   *   MOZ_MUST_USE and MOZ_STACK_CLASS) from its template arguments.
> + * MOZ_IGNORE_INITIALIZATION: Skip the constructor initialization check for this
> + *   member.

This could probably use a bit more explanation, such as some information about what the constructor initialization check is.

	Nika Layzell [:nika] (ni? for response)
	Comment 41 • 8 years ago

Comment on attachment 8756288 [details] [diff] [review]
clang-plugin-uninitialized-members-update1.diff

Review of attachment 8756288 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see the patch after new tests are added for fields with more interesting types :).

::: build/clang-plugin/clang-plugin.cpp
@@ +1900,5 @@
> +
> +        if (ctr != ctor) {
> +          ctor = ctr;
> +          break;
> +        }

This seems wrong to me. Shouldn't it be more along the lines of:

// If we aren't the definition, look through the redecls for the definition
if (!ctor->isThisDeclarationADefinition()) {
  for (auto ctorFromList : ctor->redecls()) {
    if (CXXConstructorDecl *newCtor = dyn_cast<CXXConstructorDecl>(ctorFromList)) {
      if (newCtor->isThisDeclarationADefinition()) {
        ctor = newCtor;
        break;
      }
    }
  }
}

// If we didn't find a definition, skip this declaration.
if (!ctor->isThisDeclarationADefinition()) {
  continue;
}

::: build/clang-plugin/tests/TestMemberInitialization.cpp
@@ +1,1 @@
> +#include "TestMemberInitialization.h"

You shouldn't need to split this into two files. The behavior should be the same whether it is in one file or two

@@ +5,5 @@
> +}
> +
> +void Test::InitPtrByPtr(int **var) {
> +  *var = 0;
> +}

Consistent whitespace after this line

::: build/clang-plugin/tests/TestMemberInitialization.h
@@ +1,1 @@
> +

Unnecessary whitespace at start of file.

@@ +18,5 @@
> +int do_something_with_obj(const Obj& o);
> +
> +void InitByRef(int& var);
> +
> +class Test {

Can you have another test where you use the struct keyword, but produce an error?

e.g.
struct T {
  int x; // expected-error {{...}}
  
  T() {}
};

@@ +27,5 @@
> +  int *e;
> +  int *f;
> +  int g;
> +  int h;
> +  int x; // expected-error {{'x' is not correctly initialialized in the constructor of 'Test'}}

What happens if I have a field

SomeType t;

which I don't explicitly initialize in the constructor, but is not a primitive C-style type? This should have a test. What if it's a struct type like TestStruct?

	Nika Layzell [:nika] (ni? for response)
	Comment 42 • 8 years ago

Comment on attachment 8756283 [details] [diff] [review]
clang-plugin-uninitialized-members.diff

Review of attachment 8756283 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +21,5 @@
>  #define CLANG_VERSION_FULL (CLANG_VERSION_MAJOR * 100 + CLANG_VERSION_MINOR)
>  
>  using namespace llvm;
>  using namespace clang;
> +using namespace std;

Uses of std:: in this file have mostly been prefixed. I would prefer it if we didn't using namespace std in clang-plugin.

@@ +1625,5 @@
> +    unordered_map<string, string>& resolverMap,
> +    unordered_map<string, string>& newResolverMap) {
> +
> +  // use callExp to see what it needs to be pushed by address or reference,
> +  // if we are in any of these 2 situation lookup variable in both resolveMap

situation => situations

@@ +1631,5 @@
> +  // of passed variables directly from variablesMap
> +  // It's costly but in this way we can avoid a push/ pop algorithm where
> +  // at the beggining of the function we would have pushed the needed variables
> +  // from resoveMap to newResolveMap and at the end of the function we would
> +  // have poped them back thus needing to have more map for tracking results

poped => popped

::: ipc/ipdl/ipdl/cxx/cgen.py
@@ +115,5 @@
>          else:
>              d.type.accept(self)
>  
>          if d.name:
> +            self.write(' MOZ_IGNORE_INITIALIZATION '+ d.name)

I'm not sure I can review changes to ipdl codegen, You should get someone else to review this change.

::: mfbt/Attributes.h
@@ +527,5 @@
>   *   declarations where an instance of the template should be considered, for
>   *   static analysis purposes, to inherit any type annotations (such as
>   *   MOZ_MUST_USE_TYPE and MOZ_STACK_CLASS) from its template arguments.
> + * MOZ_IGNORE_INITIALIZATION: Skip the constructor initialization check for this
> + *   member.

Can we have a bit more documentation here? This doesn't really explain very much about the analysis, and "constructor initialization check" sounds ambiguous.

@@ +562,5 @@
>  #  define MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS \
>      __attribute__((annotate("moz_inherit_type_annotations_from_template_args")))
>  #  define MOZ_NON_AUTOABLE __attribute__((annotate("moz_non_autoable")))
> +#  define MOZ_IGNORE_INITIALIZATION \
> +    __attribute__((annotate("moz_ignore_initialization")))

Can the adding of the attribute to MFBT be split out into a different patch? It should be reviewed by someone who is a peer on MFBT rather than me.

	Andi [:andi] Assignee
	Comment 43 • 8 years ago

is the same as Attachment 8740407 [details] [diff] with update in order to avoid rejects on m-c

	Andi [:andi] Assignee
	Comment 44 • 8 years ago

Update 1:

I've noticed that case where definition of the constructor was not done inline but outside of RecordDecl was lacking the CXXCtorInitializer list. This update addresses this issue. By looking through the ctor redecls and using the definitions instead of declaration.

	Andi [:andi] Assignee
	Comment 45 • 8 years ago

Update 1:

1. Fixed an issue where definition of the constructor was not done inline but outside of RecordDecl thus lacking the CXXCtorInitializer list. This update addresses this issue. By looking through the ctor redecls and using the definitions instead of declaration.

2. Added distinct header and cpp for TestMemberInitialization in this way tests that are done better reflect the real case scenarios regarding the difference between declaration and definition.

	Andi [:andi] Assignee
	Comment 46 • 8 years ago

Hello,

Could you please review this change we plan to use this annotation to member variables that we want to be skipped by initialization checker plugin. 

For example:

>>class Test {
>>  int a;
>>  MOZ_IGNORE_INITIALIZATION int b;
>>  Test() 
>>    : a(0) {
>>  }
};

In this scenario we want |b| be to be skipped during analysis so no error message like this will be present:

'b' is not correctly initialialized in the constructor of 'Test'

We plan on having this feature because for some variables we don't want this checker to be performed.

	Andi [:andi] Assignee
	Comment 48 • 8 years ago

I've updated the patch with the desired modifications. If you want to add more test scenarios just let me know.

	Nathan Froyd [:froydnj]
	Comment 49 • 8 years ago

Comment on attachment 8756741 [details] [diff] [review]
clang-plugin-Skip-Attribute.diff

Review of attachment 8756741 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Attributes.h
@@ +527,5 @@
>   *   declarations where an instance of the template should be considered, for
>   *   static analysis purposes, to inherit any type annotations (such as
>   *   MOZ_MUST_USE_TYPE and MOZ_STACK_CLASS) from its template arguments.
> + * MOZ_IGNORE_INITIALIZATION: skip the initialization check, for member variable, 
> + *   performed in the corresponding constructor

This comment needs to clearly explain what entity it applies to (for example, the "Applies to..." sentences in the nearby documentation for other attributes).  The "initialization check" needs some explaining: does it apply to list-initialization or initialization in the body of the constructor?  What is the "initialization check" actually checking?   And it's not especially clear what motivates this attribute, so I think a "This is intended to be used..." similar to the MOZ_NON_AUTOABLE attribute below would be useful.

	Andi [:andi] Assignee
	Comment 50 • 8 years ago

Hope this is better.

	Nika Layzell [:nika] (ni? for response)
	Comment 53 • 8 years ago

Comment on attachment 8758756 [details] [diff] [review]
clang-plugin-Uninitialized-members.diff

Review of attachment 8758756 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +1875,5 @@
> +  const MatchFinder::MatchResult &Result) {
> +  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
> +  unsigned nonInitializedMemberID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Error,
> +      "%0 is not correctly initialialized in the constructor of %1");

As this test might produce false positives, it might be nice to refer people who hit the error to some documentation comment somewhere in the tree which explains how the analysis works, and what to do to fix it.

::: build/clang-plugin/tests/TestMemberInitialization.cpp
@@ +133,5 @@
> +  for (int i = 0; i < 10; i++) {
> +    if (!i)
> +      a = 0;
> +  }
> +  

Unnecessary Whitespace

@@ +157,5 @@
> +};
> +
> +TestDoWhile::TestDoWhile() {
> +  int i = 0;
> +  do { 

Trailing Whitespace

	Nathan Froyd [:froydnj]
	Comment 54 • 8 years ago

Comment on attachment 8677975 [details] [diff] [review]
part 2 - MOZ_IGNORE_INITIALIZATION

Review of attachment 8677975 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/ipdl/ipdl/cxx/cgen.py
@@ +108,5 @@
>          else:
>              d.type.accept(self)
>  
>          if d.name:
> +            self.write(' MOZ_IGNORE_INITIALIZATION '+ d.name)

This change adds MOZ_IGNORE_INITIALIZATION to a billion places in codegen, including places where it has no business being, such as function parameters; I highly doubt this is what we want to use.

	Nathan Froyd [:froydnj]
	Comment 55 • 8 years ago

Comment on attachment 8758717 [details] [diff] [review]
clang-plugin-Skip-Attribute.diff

Review of attachment 8758717 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for the delay in reviewing this.

Where is this attribute intended to be used?  The lone patch in this bug that features non-test uses of this attribute is busted.

r=me if we have actual uses for the attribute.

::: mfbt/Attributes.h
@@ +530,5 @@
> + * MOZ_IGNORE_INITIALIZATION: Applies to class member declaration. It disables 
> + *   the list and body initialization check when a constructor is defined. The in-body
> + *   initialization checks assignments for the corresponding member variable. This 
> + *   is intended to be used when we specifically know that logic surrounding the 
> + *   variable is correct. 

Thanks for updating this comment.  I think it needs a little wordsmithing, though.  How about:

"Applies to class member declarations.  Occasionally there are class members that are not initialized in the constructor, but logic elsewhere in the class ensures they are initialized prior to use.  Using this attribute on a member disables the check that this member must be initialized in constructors via list-initialization, in the constructor body, or via functions called from the constructor body."

I think we might also want to rename the macro (and perhaps the attribute, too?): we're not ignoring the initialization of the member, we're indicating to the static analysis that the member is correctly initialized outside of constructor.  So perhaps MOZ_INITIALIZED_OUTSIDE_CONSTRUCTOR is more appropriate?

Please also ensure there's no trailing whitespace in this patch.

	Andi [:andi] Assignee
	Comment 56 • 8 years ago

About where we could use it, just as you comment at this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1278796
It is a good example on how we can eliminate from the analysis these types of situations. One more example is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1278242

	Andi [:andi] Assignee
	Comment 57 • 8 years ago

Attachment #8761208 [details] [diff] - pushed to m-i -> https://hg.mozilla.org/integration/mozilla-inbound/rev/3d04a9a8e9f07bccda71892f73e9571f0abe1e9e

	Carsten Book [:Tomcat]
	Comment 58 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/3d04a9a8e9f0

	Andi [:andi] Assignee
	Comment 59 • 8 years ago

This must be left open because more work needs to be done for the clang analyzer.

	Andi [:andi] Assignee
	Comment 60 • 8 years ago

This new update brings the possibility to eliminate from the matcher specific libraries like: skin, engle, harfbuzz etc:

>>AST_MATCHER(CXXRecordDecl, isInterestingForUninitializedInCtor) {
>>  const CXXRecordDecl *decl = Node.getCanonicalDecl();
>>   // No system headers.
>>  auto &SourceManager = Finder->getASTContext().getSourceManager();
>>  FullSourceLoc loc(decl->getLocation(), SourceManager);
>>
>>  if (loc.isInSystemHeader()) {
>>    // is in system header so skip it
>>    return false;
>>  } else {
>>    // check to see if it's in a package path
>>    // that should be ignored
>>    SmallString<1024> fileName = SourceManager.getFilename(loc);
>>    llvm::sys::fs::make_absolute(fileName);
>>    auto begin = llvm::sys::path::rbegin(fileName);
>>    auto end = llvm::sys::path::rend(fileName);
>>
>>    for (; begin != end; ++begin) {
>>      if (begin->compare_lower(StringRef("skia")) == 0 ||
>>          begin->compare_lower(StringRef("angle")) == 0 ||
>>          begin->compare_lower(StringRef("harfbuzz")) == 0 ||
>>          begin->compare_lower(StringRef("hunspell")) == 0 ||
>>          begin->compare_lower(StringRef("scoped_ptr.h")) == 0 ||
>>          begin->compare_lower(StringRef("graphite2")) == 0) {
>>        return false;
>>      }
>>    }
>>    return true;
>>  }
>>}

I think i should also include a case for switch statement.

	Nika Layzell [:nika] (ni? for response)
	Comment 61 • 8 years ago

Comment on attachment 8762061 [details] [diff] [review]
clang-plugin.diff

Review of attachment 8762061 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming the only change is the isInteresting check, lgtm.

	Andi [:andi] Assignee
	Comment 62 • 8 years ago

MozReview-Commit-ID: GPQY8b2OM2V

	Andi [:andi] Assignee
	Comment 63 • 8 years ago

MozReview-Commit-ID: GPQY8b2OM2V

	Andi [:andi] Assignee
	Comment 64 • 8 years ago

(In reply to Andi-Bogdan Postelnicu from comment #63)
> Created attachment 8765889 [details] [diff] [review]
> renamed MOZ_INITIALIZED_OUTSIDE_CONSTRUCTOR -> MOZ_INIT_OUTSIDE_CTOR
> 
> MozReview-Commit-ID: GPQY8b2OM2V

I've decided to rename it since many developers complained that it is to long.

	Nathan Froyd [:froydnj]
	Comment 65 • 8 years ago

Comment on attachment 8765889 [details] [diff] [review]
Landed

Review of attachment 8765889 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: mfbt/Attributes.h
@@ +527,4 @@
>   *   declarations where an instance of the template should be considered, for
>   *   static analysis purposes, to inherit any type annotations (such as
>   *   MOZ_MUST_USE_TYPE and MOZ_STACK_CLASS) from its template arguments.
> + * MOZ_INIT_OUTSIDE_CTOR: Applies to class member declarations. Occasionally 

Nit: trailing whitespace.

	Andi [:andi] Assignee
	Comment 66 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9ab4f97769

	Carsten Book [:Tomcat]
	Comment 67 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/bf9ab4f97769

	Andi [:andi] Assignee
	Comment 68 • 8 years ago

- added support for then and else branches comparison
- modified call for buildResolverMap in order to get rid of useless variablesMap
- modified updateVarMap and add any variables that is given a value to the map, in this way we can avoid having two copies of variablesMap given to thenMap and elseMap in IfStmt

	Nika Layzell [:nika] (ni? for response)
	Comment 69 • 8 years ago

Comment on attachment 8766732 [details] [diff] [review]
clang-plugin update 1.diff

Review of attachment 8766732 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +1714,5 @@
>    std::unordered_map<std::string, bool>& variablesMap,
>    StringRef varName) {
>  
> +  variablesMap[varName] = true;
> +

Unnecessary blank line

@@ +1832,5 @@
> +      // Loop through the thenMap and elseMap and look for the same variables
> +      // set to true  and add to variablesMap only the elements that are present
> +      // in both maps set to true
> +      for (auto& item: thenMap) {
> +        if (item.second && elseMap[item.first]) {

I believe operator[] always inserts into the map if the entry is not present. Ideally, extra resizing of elseMap wouldn't be occurring at this point, but that being said, it is not a big deal.

::: build/clang-plugin/tests/TestMemberInitialization.cpp
@@ +146,4 @@
>    while (i < 10) {
>      if (i % 5)
>        a = 0;
> +    else { 

Trailing whitespace

	Andi [:andi] Assignee
	Comment 70 • 8 years ago

(In reply to Michael Layzell [:mystor] from comment #69)
> Comment on attachment 8766732 [details] [diff] [review]
> clang-plugin update 1.diff
> 
> Review of attachment 8766732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/clang-plugin/clang-plugin.cpp
> @@ +1714,5 @@
> >    std::unordered_map<std::string, bool>& variablesMap,
> >    StringRef varName) {
> >  
> > +  variablesMap[varName] = true;
> > +
> 
> Unnecessary blank line
> 
> @@ +1832,5 @@
> > +      // Loop through the thenMap and elseMap and look for the same variables
> > +      // set to true  and add to variablesMap only the elements that are present
> > +      // in both maps set to true
> > +      for (auto& item: thenMap) {
> > +        if (item.second && elseMap[item.first]) {
> 
> I believe operator[] always inserts into the map if the entry is not
> present. Ideally, extra resizing of elseMap wouldn't be occurring at this
> point, but that being said, it is not a big deal.
> 
You are right, otherwise you couldn't have statements like:

elseMap["Something"] = true;

This being an unordered_map the average insert complexity is O(1), where as if we had used normal maps,
that are ordered, the insert complexity is O(logN) where N is the size of the map. 
Still i will modify it in the next patch, it better to have the best implementation possible. 

> ::: build/clang-plugin/tests/TestMemberInitialization.cpp
> @@ +146,4 @@
> >    while (i < 10) {
> >      if (i % 5)
> >        a = 0;
> > +    else { 
> 
> Trailing whitespace

	Andi [:andi] Assignee
	Comment 71 • 8 years ago

- added depth to analysis, if it's reached return from the analysed Stmt
 - get item from elseMap without first inserting it

	Nika Layzell [:nika] (ni? for response)
	Comment 72 • 8 years ago

Comment on attachment 8766783 [details] [diff] [review]
clang-plugin update 2.diff

Review of attachment 8766783 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +40,4 @@
>  #define cxxRecordDecl recordDecl
>  #endif
>  
> +#define MAX_DEPTH 100

Maybe make this a constant on the particular checker? This file has many analyses, and MAX_DEPTH doesn't really describe which analysis it is referring to very well.

It would be nicer if it was, say, NonInitializedMemberChecker::MAX_DEPTH, which at least gives it more context. Please also add a comment.

@@ +1989,4 @@
>      }
>  
>      // Maybe it's initialized in the body.
> +    evaluateExpression(body, variablesMap, resolverMap, 0);

Maybe add a /* depth = */ comment before the 0.

	Andi [:andi] Assignee
	Comment 73 • 8 years ago

As we've talked in London at All Hands Meeting, in the clase the class overloads operator new than we will skip the check for uninitialised variables.

	Andi [:andi] Assignee
	Comment 74 • 8 years ago

Comment on attachment 8769627 [details] [diff] [review]
clang-plugin update 3.diff

One a more closer thought for this we should fire another bugzilla issue.

	Andi [:andi] Assignee
	Comment 75 • 8 years ago

Another feature that we might add to this checker, as dbaron suggested, is to have the ability to mark different function members that are part of the initialisation process like:
>>class Test {
>>	int a;
>>	int b;
>>	int c;
>>
>>public:
>>	Test()
>>	: a(0) {
>>
>>	}
>>	
>>	CLASS_INIT 
>>	void Init() {
>>		b = 0;
>>		c = 0;
>>	}	
>>};

Where by using macro CLASS_INIT we tell the checker to also check the indicated function for variable initialisation/ assignment.

	Andi [:andi] Assignee
	Comment 76 • 8 years ago

MozReview-Commit-ID: 5yTnTUVLpSC

	Andi [:andi] Assignee
	Comment 77 • 8 years ago

MozReview-Commit-ID: 5yTnTUVLpSC

	Nathan Froyd [:froydnj]
	Comment 78 • 8 years ago

Comment on attachment 8785262 [details] [diff] [review]
add attribute to mark functions that initialize member variables for their parent class, in order to be scanned by clang-plugin static analysis

Review of attachment 8785262 [details] [diff] [review]:
-----------------------------------------------------------------

The comment needs adjusting, see below.  Is it not possible to traverse calls recursively from within the constructor, and avoid the annotation entirely?  I thought that's what the plugin was already doing; why is this annotation needed?

::: mfbt/Attributes.h
@@ +502,4 @@
>   *   Using this attribute on a member disables the check that this member must be
>   *   initialized in constructors via list-initialization, in the constructor body,
>   *   or via functions called from the constructor body.
> + * MOZ_IS_CLASS_INIT: Applies to function class member declaration. Occasionally

I think this should be "Applies to class method declarations."

@@ +503,5 @@
>   *   initialized in constructors via list-initialization, in the constructor body,
>   *   or via functions called from the constructor body.
> + * MOZ_IS_CLASS_INIT: Applies to function class member declaration. Occasionally
> + *   the constructor doesn't initialize all of the member variables and another
> + *   function is used to initialize the rest. This marker is used to let the static

"...to make the static analysis tool..."

@@ +504,5 @@
>   *   or via functions called from the constructor body.
> + * MOZ_IS_CLASS_INIT: Applies to function class member declaration. Occasionally
> + *   the constructor doesn't initialize all of the member variables and another
> + *   function is used to initialize the rest. This marker is used to let the static
> + *   analysis tool aware that the marked function is part of the initialize process

"..part of the initialization process..."

@@ +505,5 @@
> + * MOZ_IS_CLASS_INIT: Applies to function class member declaration. Occasionally
> + *   the constructor doesn't initialize all of the member variables and another
> + *   function is used to initialize the rest. This marker is used to let the static
> + *   analysis tool aware that the marked function is part of the initialize process
> + *   and to include it in the scan mechanism that determines witch variables still

"...to include the marked function...that determines which member variables..."

@@ +506,5 @@
> + *   the constructor doesn't initialize all of the member variables and another
> + *   function is used to initialize the rest. This marker is used to let the static
> + *   analysis tool aware that the marked function is part of the initialize process
> + *   and to include it in the scan mechanism that determines witch variables still
> + *   remain uninitialized or unassigned values.

We can drop the "or unassigned values" bit here.

	Andi [:andi] Assignee
	Comment 79 • 8 years ago

(In reply to Nathan Froyd [:froydnj] from comment #78)
> Comment on attachment 8785262 [details] [diff] [review]
> add attribute to mark functions that initialize member variables for their
> parent class, in order to be scanned by clang-plugin static analysis
> 
> Review of attachment 8785262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The comment needs adjusting, see below.  Is it not possible to traverse
> calls recursively from within the constructor, and avoid the annotation
> entirely?  I thought that's what the plugin was already doing; why is this
> annotation needed?
This is what clang-plugin does in it's actual state but there could be initialisation function that 
are not called from within the constructor and we want to be able to scan those functions as well, that's
why we could use an annotation. Check this basic example
>>class Test {
>>	int a;
>>	int b;
>>	int c;
>>
>>public:
>>	Test()
>>	: a(0) {
>>
>>	}
>>	
>>	MOZ_IS_CLASS_INIT 
>>	void Init() {
>>		b = 0;
>>		c = 0;
>>	}	
>>};

where somewhere in the code where we instantiate class Test we can have:
>> Test myTest;
>> myTest.Init();

In the current form of the plugin we scan recursively only the body of the CTOR, now after we add the annotation we will also scan Init.

> 
> ::: mfbt/Attributes.h
> @@ +502,4 @@
> >   *   Using this attribute on a member disables the check that this member must be
> >   *   initialized in constructors via list-initialization, in the constructor body,
> >   *   or via functions called from the constructor body.
> > + * MOZ_IS_CLASS_INIT: Applies to function class member declaration. Occasionally
> 
> I think this should be "Applies to class method declarations."
> 
> @@ +503,5 @@
> >   *   initialized in constructors via list-initialization, in the constructor body,
> >   *   or via functions called from the constructor body.
> > + * MOZ_IS_CLASS_INIT: Applies to function class member declaration. Occasionally
> > + *   the constructor doesn't initialize all of the member variables and another
> > + *   function is used to initialize the rest. This marker is used to let the static
> 
> "...to make the static analysis tool..."
> 
> @@ +504,5 @@
> >   *   or via functions called from the constructor body.
> > + * MOZ_IS_CLASS_INIT: Applies to function class member declaration. Occasionally
> > + *   the constructor doesn't initialize all of the member variables and another
> > + *   function is used to initialize the rest. This marker is used to let the static
> > + *   analysis tool aware that the marked function is part of the initialize process
> 
> "..part of the initialization process..."
> 
> @@ +505,5 @@
> > + * MOZ_IS_CLASS_INIT: Applies to function class member declaration. Occasionally
> > + *   the constructor doesn't initialize all of the member variables and another
> > + *   function is used to initialize the rest. This marker is used to let the static
> > + *   analysis tool aware that the marked function is part of the initialize process
> > + *   and to include it in the scan mechanism that determines witch variables still
> 
> "...to include the marked function...that determines which member
> variables..."
> 
> @@ +506,5 @@
> > + *   the constructor doesn't initialize all of the member variables and another
> > + *   function is used to initialize the rest. This marker is used to let the static
> > + *   analysis tool aware that the marked function is part of the initialize process
> > + *   and to include it in the scan mechanism that determines witch variables still
> > + *   remain uninitialized or unassigned values.
> 
> We can drop the "or unassigned values" bit here.

Thanks for the help with the comment i will update it and post it for review.

	Nathan Froyd [:froydnj]
	Comment 80 • 8 years ago

(In reply to Andi-Bogdan Postelnicu from comment #79)
> (In reply to Nathan Froyd [:froydnj] from comment #78)
> > Comment on attachment 8785262 [details] [diff] [review]
> > add attribute to mark functions that initialize member variables for their
> > parent class, in order to be scanned by clang-plugin static analysis
> > 
> > Review of attachment 8785262 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The comment needs adjusting, see below.  Is it not possible to traverse
> > calls recursively from within the constructor, and avoid the annotation
> > entirely?  I thought that's what the plugin was already doing; why is this
> > annotation needed?
> This is what clang-plugin does in it's actual state but there could be
> initialisation function that 
> are not called from within the constructor and we want to be able to scan
> those functions as well, that's
> why we could use an annotation. Check this basic example
> >>class Test {
> >>	int a;
> >>	int b;
> >>	int c;
> >>
> >>public:
> >>	Test()
> >>	: a(0) {
> >>
> >>	}
> >>	
> >>	MOZ_IS_CLASS_INIT 
> >>	void Init() {
> >>		b = 0;
> >>		c = 0;
> >>	}	
> >>};
> 
> where somewhere in the code where we instantiate class Test we can have:
> >> Test myTest;
> >> myTest.Init();
> 
> In the current form of the plugin we scan recursively only the body of the
> CTOR, now after we add the annotation we will also scan Init.

Ah, very good, thank you for the example.  The plugin then checks that there are no uses between construction and MOZ_IS_CLASS_INIT methods being called as well?

	Andi [:andi] Assignee
	Comment 81 • 8 years ago

(In reply to Nathan Froyd [:froydnj] from comment #80)
> Ah, very good, thank you for the example.  The plugin then checks that there
> are no uses between construction and MOZ_IS_CLASS_INIT methods being called
> as well?

Yes, the current stage of the plugin checks only the ctor now having this annotation we will be able to also check the marked functions.

	Andi [:andi] Assignee
	Comment 82 • 8 years ago

MozReview-Commit-ID: 5yTnTUVLpSC

	Andi [:andi] Assignee
	Comment 83 • 8 years ago

MozReview-Commit-ID: FhEAzS5CCDf

	Andi [:andi] Assignee
	Comment 84 • 8 years ago

Comment on attachment 8785924 [details] [diff] [review]
clang-plugin.diff

just a rebase.

	Nika Layzell [:nika] (ni? for response)
	Comment 85 • 8 years ago

Comment on attachment 8785924 [details] [diff] [review]
clang-plugin.diff

Review of attachment 8785924 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
I didn't look over it much because it is just a rebase. If part of it changed significantly and you want me to look at it again, just ping me and point me at it.

	Andi [:andi] Assignee
	Comment 86 • 8 years ago

Comment on attachment 8785825 [details] [diff] [review]
add attribute to mark functions that initialize member variables for their parent class, in order to be scanned by clang-plugin static analysis

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b245d867368

	Andi [:andi] Assignee
	Comment 87 • 8 years ago

MozReview-Commit-ID: AYerMratJHe

	Andi [:andi] Assignee
	Comment 88 • 8 years ago

Comment on attachment 8786261 [details] [diff] [review]
clang-plugin update 3.diff

In order to have things simple we first determine if we have functions that are annotated as being part of the initialisation process and scan them. The variables that get marked as being initialised are stored in the map with a special flag - InitByFunc. This flag is used in order to differentiate variables initialised in init functions from the one initialised by the ctors since the map is refreshed for each ctor pass.

	Ryan VanderMeulen [:RyanVM]
	Comment 89 • 8 years ago

(In reply to Andi-Bogdan Postelnicu from comment #86)
> Comment on attachment 8785825 [details] [diff] [review]

https://hg.mozilla.org/mozilla-central/rev/8b245d867368

	Nika Layzell [:nika] (ni? for response)
	Comment 90 • 8 years ago

Comment on attachment 8786261 [details] [diff] [review]
clang-plugin update 3.diff

Review of attachment 8786261 [details] [diff] [review]:
-----------------------------------------------------------------

I feel mildly uncomfortable about initializers working on partially initialized structs, but I understand that it probably happens a lot in our code base, and it would be too much work to get rid of them. 

nit: In bug 1287458, the clang-plugin is getting a consistent style. As that patch is probably going to land before this one, it would be awesome if you could update the style in this patch to match the LLVM style which we're using for the clang-plugin now :).

::: build/clang-plugin/clang-plugin.cpp
@@ +2085,5 @@
> +
> +  // First look for functions that are marked as being part of the initialization
> +  // process and scan though them before looking at constructors since in this way
> +  // we can avoid popping false-positive errors for variables that are not
> +  // initialized in the constructor

I understand why you run this logic first, but it seems a bit odd to me as it's the opposite order to how initialization actually works in the code.

It probably makes the most sense to leave it this way anyways though.

::: build/clang-plugin/tests/TestMemberInitialization.cpp
@@ +29,5 @@
>  
>  // this should trigger an unintialized error message
>  struct TestStruct2 {
> +  int b;
> +  TestStruct2() { // expected-error {{b is not correctly initialialized in the constructor of 'TestStruct2'}}

As you no longer report the error on the property which is not initialized, it seems like it would be good to say something like `member 'b' is not ....`, or perhaps even:

    Error: Member 'TestStruct2::b` is not initialized in this constructor
    Note: All members must be initialized in the constructor, or in a MOZ_IS_CLASS_INIT method

The above error message isn't as clear as it could be.

@@ +178,5 @@
> +public:
> +  TestInitFunc() : a(0) {
> +
> +  }
> +  MOZ_IS_CLASS_INIT void Init();

Can you file a follow-up for making sure that the init function is called immediately after construction of all objects which have one?

	(no longer active)
	Comment 91 • 8 years ago

What's the status of this project?

	Andi [:andi] Assignee
	Comment 92 • 8 years ago

I will update it shortly, we still need to fix all of the uninitialized variables that it detects. I'll also add support for templates.

	(no longer active)
	Comment 93 • 8 years ago

Sounds great.  Thank you.  :-)

	Andi [:andi] Assignee
	Comment 94 • 8 years ago

So i've returned on this bug since it needs to be compliant with the new clang tidy infrastructure and after talking with Julian Seward, he would really use this patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1232696

	(no longer active)
	Comment 95 • 8 years ago

Nice.  Sorry I bitrotted your patches.  I hope the new code structure makes it much easier to hack on new static analyses in the future.  I have been meaning to write some documentation but haven't gotten around to doing that yet.  But I just uploaded a rebased version of bug 1114683 and you can use that as a sample on how to rebase.  Let me know if you have any questions or hit any issues.  :-)

	Andi [:andi] Assignee
	Comment 96 • 7 years ago

This is WIP in order to work with the new build mechanism based on clang-tidy. The template support will be added.

	Emilio Cobos Álvarez (:emilio)
	Comment 97 • 7 years ago

I was trying to run the analysis on layout code today, and I needed this patch in order to avoid a lot of false positives.

Hope it helps :)

	Andi [:andi] Assignee
	Comment 98 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #97)
> Created attachment 8864147 [details] [diff] [review]
> Skip delegating constructors in the non-initialized member checker.
> 
> I was trying to run the analysis on layout code today, and I needed this
> patch in order to avoid a lot of false positives.
> 
> Hope it helps :)

Thanks for this, if you need help running it or you encounter corner cases that are not yet covered let me know and I'll update it. Also the present implementation doesn't cover the use of template classes.

	Emilio Cobos Álvarez (:emilio)
	Comment 99 • 7 years ago

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #98)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #97)
> > Created attachment 8864147 [details] [diff] [review]
> > Skip delegating constructors in the non-initialized member checker.
> > 
> > I was trying to run the analysis on layout code today, and I needed this
> > patch in order to avoid a lot of false positives.
> > 
> > Hope it helps :)
> 
> Thanks for this, if you need help running it or you encounter corner cases
> that are not yet covered let me know and I'll update it. Also the present
> implementation doesn't cover the use of template classes.

It works quite well, to be honest.

There's one annoying thing when grepping for the error message in the build logs, which is that it contains a typo (initialialized instead of just initialized), but that's not something to be worried about I think :)

	Andi [:andi] Assignee
	Comment 100 • 7 years ago

This new patch has the following new addons:
1. Courtesy of Emilio - Skip delegating constructors in the non-initialized member checker.
2. Fixed type of uninitialised.
4. Added the framework for detecting for the class templates.

	Andi [:andi] Assignee
	Comment 101 • 7 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #99)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #98)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #97)
> > > Created attachment 8864147 [details] [diff] [review]
> > > Skip delegating constructors in the non-initialized member checker.
> > > 
> > > I was trying to run the analysis on layout code today, and I needed this
> > > patch in order to avoid a lot of false positives.
> > > 
> > > Hope it helps :)
> > 
> > Thanks for this, if you need help running it or you encounter corner cases
> > that are not yet covered let me know and I'll update it. Also the present
> > implementation doesn't cover the use of template classes.
> 
> It works quite well, to be honest.
> 
> There's one annoying thing when grepping for the error message in the build
> logs, which is that it contains a typo (initialialized instead of just
> initialized), but that's not something to be worried about I think :)

Did you encounter any case where this analysis failed?

	Emilio Cobos Álvarez (:emilio)
	Comment 102 • 7 years ago

I seem (In reply to Andi-Bogdan Postelnicu [:andi] from comment #101)
> Did you encounter any case where this analysis failed?

I'm pretty sure it reported wrong the WritingMode constructor[2], but that presumably just needs an init annotation? Ideally it wouldn't require it though.

From memory, I seem to recall it reported incorrectly gfxTextPerfMetrics::reflowCount[1], but I'm less sure about this one.

Note that I was filtering by path and excluded some files that reported a lot of errors, like nsDisplayList.h, which has constructors with uninitialized members that expects subclasses to initialize, and nsStyleStruct.h, which intentionally leaves uninitialized values too. But those errors seemed legit.

I don't recall any other off-hand.

[1]: http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/gfx/thebes/gfxFont.h#421
[2]: http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/layout/generic/WritingModes.h#483

	Andi [:andi] Assignee
	Comment 103 • 7 years ago

Thanks for telling me this, I'll take a look and see what was going on there.

	Andi [:andi] Assignee
	Comment 104 • 7 years ago

This is a good catch:

[1]
>>    gfxTextPerfMetrics() {
>>        memset(this, 0, sizeof(gfxTextPerfMetrics));
>>    }

I wonder if we would need to support this kind of construction. 

[2]

Why would WritingMode need an init annotation, isn't the initialisation done in InitFromStyleVisibility that's called from the constructor?

The annotation MOZ_IS_CLASS_INIT must be used on member functions that are not called from CTOR but are used as initialisers, like:

>>SomeClass A;
>>// Do some funky logic 
>>A->Init();

	Tristan Bourvon
	Comment 105 • 7 years ago

This adds full tree traversal of statements and expressions and also handles switches and ternary conditional operators to make sure all branches initialize a member for it to be considered as initialized.

	Tristan Bourvon
	Comment 106 • 7 years ago

Refactor duplicated traversal of children into a common method

	Tristan Bourvon
	Comment 107 • 7 years ago

Add support for detecting inits through intermediate references

	Tristan Bourvon
	Comment 108 • 7 years ago

Also added handling of pointers and references to pointers

	Tristan Bourvon
	Comment 109 • 7 years ago

Add detection for POD fields full initialization (recursively)

	Tristan Bourvon
	Comment 110 • 7 years ago

Add fixit notes to add MOZ_UNCHECKED_INIT in order to whitelist existing defects.
Also refactors the code into smaller functions and cleaner types.

	Tristan Bourvon
	Comment 111 • 7 years ago

Folds all the previous patches

	Nika Layzell [:nika] (ni? for response)
	Comment 112 • 7 years ago

Comment on attachment 8879505 [details] [diff] [review]
525063-fold.patch

Review of attachment 8879505 [details] [diff] [review]:
-----------------------------------------------------------------

Please don't land a commit with this commit message. Either keep it split into separate patches, or use a unified commit name.

It looks really good. My brain started hurting a bit when reading over the expression evaluator, so I might come back to that later, but I think my comment about its exact semantics being wrong being OK is probably correct. I mostly have nits, but this is a pretty big patch, so there are a bunch :)

::: build/clang-plugin/CustomMatchers.h
@@ +161,5 @@
> +      // Skip constructors in system headers
> +      !ASTIsInSystemHeader(Declaration->getASTContext(), *Declaration) &&
> +      // Skip ignored namespaces and paths
> +      !isInIgnoredNamespaceForImplicitCtor(Declaration) &&
> +      !isIgnoredPathForImplicitCtor(Declaration);

Consider using inThirdPartyPath (http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/build/clang-plugin/Utils.h#387-426) to avoid the proliferation of more different "ignored path" and "ignored namespace" methods :).

::: build/clang-plugin/NonInitializedMemberChecker.cpp
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "NonInitializedMemberChecker.h"
> +#include "CustomMatchers.h"
> +#include <iostream>

Do you need iostream in this file? Usually clang uses its own form of iostream, rather than the one from the standard library.

@@ +192,5 @@
> +        return;
> +      }
> +
> +      DeclaratorDecl *decl =
> +          dyn_cast_or_null<DeclaratorDecl>(declExpr->getDecl());

NIT: you don't need the or_null here

@@ +226,5 @@
> +    ResolverMap &resolverMap, InitFlag flag) {
> +
> +  static struct externFuncDefinition {
> +    std::string funcName;
> +    uint8_t indexInArgList;

Why is this only 8-bits wide? This isn't saving any space due to padding, so might as well make it 32-bits.

@@ +227,5 @@
> +
> +  static struct externFuncDefinition {
> +    std::string funcName;
> +    uint8_t indexInArgList;
> +  } funcSchemaArray[] = {// for memset

NIT: Please name this struct with UpperCamelCase, and declare it outside of the variable declaration.

@@ +242,5 @@
> +
> +  for (int i = 0; i < numberOfFuncs; i++) {
> +    externFuncDefinition &funcSchema = funcSchemaArray[i];
> +    if (funcSchema.funcName == method->getName().data()) {
> +      Expr *exprArg = funcCall->getArg(funcSchema.indexInArgList);

Check that funcSchema.indexInArgList is in range before calling getArg. getArg doesn't do bounds checking.

@@ +262,5 @@
> +    }
> +  }
> +}
> +
> +void NonInitializedMemberChecker::evaluateExpression(

I didn't look too closely at the exact behavior of the symbolic evaluator here, as I could easily poke holes of places where it has odd behavior, but I don't think we're trying to build a perfect symbolic evaluator for this bug.

Perhaps in a future bug we can look into creating a generic symbolic evaluator (or using one which someone else has written for us) which can make writing future static analyses easier, but if this one does a good enough job, then I'm probably mostly fine with it.

Please add a comment to this function explaining that it isn't an accurate evaluator, and often makes heuristic judgements to try to catch as many situations as possible situations where a member is correctly initialized. If you have a good idea of places where the evaluator makes incorrect judgements, it might be nice to document some of these as well.

@@ +270,5 @@
> +  if (!stmtExpr) {
> +    return;
> +  }
> +
> +  stmtExpr = stmtExpr->IgnoreImplicit();

We usually use our own IgnoreTrivials method instead of Stmt->IgnoreImplicit(), as it skips more things which we usually don't care about (like ParenExprs).

http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/build/clang-plugin/Utils.h#307-330

@@ +273,5 @@
> +
> +  stmtExpr = stmtExpr->IgnoreImplicit();
> +
> +  // Check depth and if it's equal equal with MAX_DEPTH return
> +  if (depth == MAX_DEPTH) {

NIT: Can you be a little safer here, and use >= :)

@@ +277,5 @@
> +  if (depth == MAX_DEPTH) {
> +    return;
> +  }
> +
> +  if (BinaryOperator *binOp = dyn_cast_or_null<BinaryOperator>(stmtExpr)) {

NIT: Use auto* here, it adds less line noise, and the target type is already written on the right. 

In addition, null-check stmtExpr before entering these ifs, and don't bother with `dyn_cast_or_null`, just use `dyn_cast`

@@ +328,5 @@
> +          }
> +        }
> +      }
> +    }
> +  } else if (IfStmt *ifStmt = dyn_cast_or_null<IfStmt>(stmtExpr)) {

You might be able to get away without all of this explicit handling of control flow by instead using the CFG (see the MustReturnFromCallerChecker for an example of using the CFG in tree http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/build/clang-plugin/MustReturnFromCallerChecker.cpp#34-40)

@@ +505,5 @@
> +      uncheckedFieldNames.insert(Attr->getAnnotation());
> +      isLastAttrMozUncheckedInit = false;
> +    } else if (Attr->getAnnotation() == "moz_unchecked_initialization") {
> +      isLastAttrMozUncheckedInit = true;
> +    }

I make a comment below about how I'm mildly uncomfortable with this design for the annotations, and would prefer it to be simplfiied to a `moz_unchecked_initialzation=mApples,mPears` style annotation.

@@ +510,5 @@
> +  }
> +
> +  UncheckedFieldsSet uncheckedFields;
> +  for (auto field : decl->fields()) {
> +    if (uncheckedFieldNames.count(field->getName())) {

Please use getNameChecked in places where you're calling getName(), as this can fail if the field has a non-identifier name (http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/build/clang-plugin/Utils.h#46-49

@@ +519,5 @@
> +  return uncheckedFields;
> +}
> +
> +void NonInitializedMemberChecker::buildVariablesMap(
> +    VariablesMap &variablesMap, CXXRecordDecl const * decl,

NIT: llvm style would write this as `const CXXRecordDecl *Decl`

@@ +520,5 @@
> +}
> +
> +void NonInitializedMemberChecker::buildVariablesMap(
> +    VariablesMap &variablesMap, CXXRecordDecl const * decl,
> +    std::string prefix = "") {

NIT: Please declare defaults in the declaration in the header, rather than in the definition.

@@ +522,5 @@
> +void NonInitializedMemberChecker::buildVariablesMap(
> +    VariablesMap &variablesMap, CXXRecordDecl const * decl,
> +    std::string prefix = "") {
> +  // For each field, if they are builtinType, if they are pointer  or if they
> +  // are PDO, add them to the table, possibly recursively if PDO

Please document all of these helper methods, and place the comments immediately before the declaration in the header file.

@@ +524,5 @@
> +    std::string prefix = "") {
> +  // For each field, if they are builtinType, if they are pointer  or if they
> +  // are PDO, add them to the table, possibly recursively if PDO
> +
> +  auto uncheckedFields = getUncheckedFields(decl);

You seem to use this list of unchecked fields to then filter the list to only a list of checked field. Why not make `getUncheckedFields` instead be `getCheckedFields`, which returns a list of the fields which you do want to look at instead?

@@ +541,5 @@
> +    QualType type = field->getType();
> +
> +    const CXXRecordDecl *fieldDecl = type->getAsCXXRecordDecl();
> +    // This should be OK because the reference types are mandated to be
> +    // initialized by the language.

I'm not sure I understand what your comment is trying to say.

I think what this check is trying to do is ignore non-POD classes as they will initialize their own members implicitly? I think writing that in a comment will be more clear.

@@ +543,5 @@
> +    const CXXRecordDecl *fieldDecl = type->getAsCXXRecordDecl();
> +    // This should be OK because the reference types are mandated to be
> +    // initialized by the language.
> +    if (!type->isBuiltinType() && !type->isPointerType()
> +        && (!fieldDecl || !fieldDecl->isPOD())) {

This check seems overly complicated to me, Can't you just write:

if (!fieldDecl || !fieldDecl->isPOD())

because having the type being a CXXRecordDecl implies !type->isBuiltinType and !type->isPointerType()?

@@ +550,5 @@
> +
> +    bool isPointerType = type->isPointerType();
> +    if (isPointerType) {
> +      fieldDecl = type->getPointeeCXXRecordDecl();
> +    }

Why are we looking through raw pointers? I would think that we would want to just expect these raw pointers to be initialized to some value.

@@ +555,5 @@
> +
> +    // Add to variable map
> +    variablesMap[field] = FieldData {
> +      InitFlag::NotInit,
> +      prefix + field->getName().str()

getNameChecked

@@ +571,5 @@
> +void NonInitializedMemberChecker::checkInitMethods(
> +    const CXXRecordDecl *decl, VariablesMap &variablesMap,
> +    ResolverMap &resolverMap) {
> +  for (auto method : decl->methods()) {
> +    Stmt *funcBody = method->getBody();

NIT: Call this method after checking for the moz_is_class_init method.

@@ +606,5 @@
> +    return;
> +  }
> +
> +  SourceLocation fixitLoc = decl->getInnerLocStart().getLocWithOffset(offset);
> +  std::string fixitString = "MOZ_UNCHECKED_INIT(";

I don't want to recommend MOZ_UNCHECKED_INIT() as the fix for not initializing these members. I feel like recommending initializing them to 0 would be a better solution. I would rather not encourage using MOZ_UNCHECKED_INIT more than we need to.

If you want to use a fixit hint to do some automated rewriting of the tree for the initial landing then you should keep it around for that, but I would rather not land it.

@@ +630,5 @@
> +
> +const CXXRecordDecl* NonInitializedMemberChecker::getFieldDeclPODRecord(
> +    FieldDecl *fieldDecl) {
> +  const CXXRecordDecl *declPOD = fieldDecl->getType()->getAsCXXRecordDecl();
> +  if (fieldDecl->getType()->isPointerType()) {

I don't understand the motivation behind ensuring that the fields of objects behind pointers are initialized. We should just check that the pointer itself is initialized I would think.

@@ +692,5 @@
> +
> +    // This is when definition is done outside of declaration
> +    // loop through the redecls and get the one that is different
> +    // than the current ctor since it will be the definition.
> +    if (!ctor->isThisDeclarationADefinition()) {

There is a method on FunctionDecl called `getDefinition` and `isDefined` which you can use to do this work for you :)

@@ +771,5 @@
> +          continue;
> +        }
> +
> +        // We emit the error diagnostic for the (var, ctor) pair
> +        Diag.Report(ctor->getLocation(), nonInitializedMemberDiagID)

Use diag() here rather than Diag.Report, as the Checker has that functionality built in now.

@@ +795,5 @@
> +          "'%0' is not correctly initialized in the constructor of %1");
> +  unsigned fixitNoteDiagID = Diag.getDiagnosticIDs()->getCustomDiagID(
> +      DiagnosticIDs::Note,
> +      "consider marking %0's fields as unchecked in MOZ_UNCHECKED_INIT here "
> +      "(after the class/struct keyword)");

With the new clang-tidy based setup, we don't need to do this work to pre-generate these IDs. Instead, you should emit diagnostics through the `diag` method defined on the Checker class.

See for example: 
http://searchfox.org/mozilla-central/rev/2bcd258281da848311769281daf735601685de2d/build/clang-plugin/MustUseChecker.cpp#58-59

For example, wanting to emit a diagnostic would look like:

diag(ctor->getLocation(), "'%0' is not correctly initialized in the constructor of %1", DiagnosticIDs::Error) << "A" << "B";

If you want to use the same diagnostic message in multiple places, please just define a: `static const char* NON_INITIALIZED_MEMBER = "'%0' is not correctly initialized in the constructor of %1";` at the top of this file :).

@@ +808,5 @@
> +  }
> +
> +  // We clear the uncheckedFields cache just in case this class instance is
> +  // reused multiple times
> +  m_uncheckedFieldsCache.clear();

Why are you worried about keeping the same cache between multiple different struct checks? This class will be re-used, but that should be a good thing, potentially saving you unnecessary re-checks (for example if the same POD struct is referenced from 2 different other structs as a member).

::: build/clang-plugin/NonInitializedMemberChecker.h
@@ +24,5 @@
> +  };
> +
> +  struct FieldData {
> +    InitFlag flag;
> +    std::string fullName;

NIT: Please change these member names to match LLVM style (Flag and FullName).

@@ +35,5 @@
> +  typedef std::unordered_map<DeclaratorDecl*, DeclaratorDecl*> ResolverMap;
> +  typedef std::unordered_set<FieldDecl*> FixitFields;
> +
> +  const uint8_t MAX_DEPTH = 100;
> +  UncheckedFieldsCacheMap m_uncheckedFieldsCache;

NIT: We do not use the m_ prefix for member variables in the clang plugin. We use LLVM's coding style which styles member variables in UpperCamelCase, so this would be:

UncheckedFieldsCacheMap UncheckedFieldsCache;

@@ +37,5 @@
> +
> +  const uint8_t MAX_DEPTH = 100;
> +  UncheckedFieldsCacheMap m_uncheckedFieldsCache;
> +
> +  void runThroughDefaultFunctions(

NIT: Please use a consistent style for these method declarations. We use llvm-style in the clang plugin, so this function should be declared like:

    void runThroughDefaultFunctions(CallExpr *callExpr,
                                    VariablesMap &varMap,
                                    ResolverMap &resolverMap,
                                    InitFlag flag);

In addition, LLVM generally uses UpperCamelCase for variable and parameter names, so ideally these parameters would have their names changed to match that style.

::: build/clang-plugin/tests/TestInitializedMemberChecker.cpp
@@ +40,5 @@
> +  }
> +};
> +
> +
> +class MOZ_UNCHECKED_INIT(y) Test { // expected-note {{consider marking Test's fields as unchecked in MOZ_UNCHECKED_INIT here (after the class/struct keyword)}}

Please add a test for MOZ_UNCHECKED_INIT(y, z, q) or something like that (marking multiple fields as un-intialized with one annotation).

::: mfbt/Attributes.h
@@ +517,5 @@
> + *   the time of its introduction. This allows us to focus on the new defects
> + *   introduced later on. Using this attribute on class members disables the
> + *   check that these member must be initialized in constructors via
> + *   list-initialization, in the constructor body, or via functions called from
> + *   the constructor body.

I don't find this explanation of the annotation very clear. How about:

MOZ_UNCHECKED_INIT(members...): Applies to class declarations. Marks the listed members as potentially uninitialized, disabling errors or warnings generated due to them not being initialized in class constructors. All non-annotated members must be initialized in constructors via list initialization, in the constructor body, or via functions called directly from the constructor.

I also don't really understand why we decided to go with MOZ_UNCHECKED_INIT(members...) on the class declaration, rather than just MOZ_UNCHECKED_INIT on the member declaration itself, which seems more intuitive to me.

@@ +575,5 @@
>  #  define MOZ_NON_AUTOABLE __attribute__((annotate("moz_non_autoable")))
>  #  define MOZ_INIT_OUTSIDE_CTOR \
>      __attribute__((annotate("moz_ignore_ctor_initialization")))
> +#  define MOZ_UNCHECKED_INIT(...) \ // Attributes are read backwards
> +    __attribute__((annotate(#__VA_ARGS__))) \

Doing this seems a little sketchy to me. If we have 2 attributes which both want to handle lists, we would not be able to easily tell the annotations apart. I would prefer it if we instead had one annotation:

__attribute__((annotate("moz_unchecked_initialization=" #__VA_ARGS__)))

and in the code when looking for the annotation, we simply checked if an annotation starts with "moz_unchecked_initialization".

@@ +576,5 @@
>  #  define MOZ_INIT_OUTSIDE_CTOR \
>      __attribute__((annotate("moz_ignore_ctor_initialization")))
> +#  define MOZ_UNCHECKED_INIT(...) \ // Attributes are read backwards
> +    __attribute__((annotate(#__VA_ARGS__))) \
> +    __attribute__((annotate("moz_unchecked_initialization")))

Please add the equivalent define in the #else branch, (like #  define MOZ_UNCHECKED_INIT(...) /* nothing */)

	Tristan Bourvon
	Comment 113 • 7 years ago

Thanks very much for the in-depth review. If I don't answer on one of your points, it means that it is fixed/taken care of.
The patch following this reply will come very soon.

(In reply to Michael Layzell [:mystor] from comment #112)
> Comment on attachment 8879505 [details] [diff] [review]
> 525063-fold.patch
> 
> Review of attachment 8879505 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +262,5 @@
> > +    }
> > +  }
> > +}
> > +
> > +void NonInitializedMemberChecker::evaluateExpression(
> 
> I didn't look too closely at the exact behavior of the symbolic evaluator
> here, as I could easily poke holes of places where it has odd behavior, but
> I don't think we're trying to build a perfect symbolic evaluator for this
> bug.
> 
> Perhaps in a future bug we can look into creating a generic symbolic
> evaluator (or using one which someone else has written for us) which can
> make writing future static analyses easier, but if this one does a good
> enough job, then I'm probably mostly fine with it.
> 
> Please add a comment to this function explaining that it isn't an accurate
> evaluator, and often makes heuristic judgements to try to catch as many
> situations as possible situations where a member is correctly initialized.
> If you have a good idea of places where the evaluator makes incorrect
> judgements, it might be nice to document some of these as well.

This method probably has very, very few false positives, and mostly has false negatives because it doesn't cover all cases. This is intentional, as I feel we have reached a point where most common constructs are covered, and it would take too much time to try to cover what's left. I will comment on it anyway.

> @@ +328,5 @@
> > +          }
> > +        }
> > +      }
> > +    }
> > +  } else if (IfStmt *ifStmt = dyn_cast_or_null<IfStmt>(stmtExpr)) {
> 
> You might be able to get away without all of this explicit handling of
> control flow by instead using the CFG (see the MustReturnFromCallerChecker
> for an example of using the CFG in tree
> http://searchfox.org/mozilla-central/rev/
> 2bcd258281da848311769281daf735601685de2d/build/clang-plugin/
> MustReturnFromCallerChecker.cpp#34-40)

This could probably have been better if we had used it since the beginning, but now I highly doubt it's worth the time to refactor the existing code, which covers most if not all cases in terms of control flow (especially since it has special cases for if and switch statements).

> @@ +524,5 @@
> > +    std::string prefix = "") {
> > +  // For each field, if they are builtinType, if they are pointer  or if they
> > +  // are PDO, add them to the table, possibly recursively if PDO
> > +
> > +  auto uncheckedFields = getUncheckedFields(decl);
> 
> You seem to use this list of unchecked fields to then filter the list to
> only a list of checked field. Why not make `getUncheckedFields` instead be
> `getCheckedFields`, which returns a list of the fields which you do want to
> look at instead?

Doing this wouldn't change anything in terms of performance or behaviour, but the pattern we have adopted throughout the code is to do a trivial loop and then filter things out progressively with early-returns/continues. It seems logical to me to keep this construct. I don't think it's worth spending time on this.

> 
> @@ +541,5 @@
> > +    QualType type = field->getType();
> > +
> > +    const CXXRecordDecl *fieldDecl = type->getAsCXXRecordDecl();
> > +    // This should be OK because the reference types are mandated to be
> > +    // initialized by the language.
> 
> I'm not sure I understand what your comment is trying to say.
> 
> I think what this check is trying to do is ignore non-POD classes as they
> will initialize their own members implicitly? I think writing that in a
> comment will be more clear.

That's the point of the code, but I have to agree that the comment is not very clear. What it meant is that we don't have to check member references as they have to be initialized. Hence the absence of a !type->isReferenceType() in this early continue.
I will rephrase it.

> @@ +543,5 @@
> > +    const CXXRecordDecl *fieldDecl = type->getAsCXXRecordDecl();
> > +    // This should be OK because the reference types are mandated to be
> > +    // initialized by the language.
> > +    if (!type->isBuiltinType() && !type->isPointerType()
> > +        && (!fieldDecl || !fieldDecl->isPOD())) {
> 
> This check seems overly complicated to me, Can't you just write:
> 
> if (!fieldDecl || !fieldDecl->isPOD())
> 
> because having the type being a CXXRecordDecl implies !type->isBuiltinType
> and !type->isPointerType()?

These two boolean expressions are not equivalent.
The current one goes into the early-continue if the type is NOT a builtin AND NOT a pointer AND not a POD.
These are exactly the cases that we want to cover, so it makes sense to discard any other case (this includes non-POD objects and references).
Your proposal would early-continue if the type is NOT a POD, but would early-continue for a builtin or a pointer as well, which is not what we want.

I think that the implication that you wanted to write was:
fieldDecl => !type->isBuiltinType && !type->isPointerType()
But in the expression you are suggesting, you are using !fieldDecl

We could filter based on (fieldDecl && !field->isPOD()), but that would not filter references (or other cases we haven't thought of). It's better to whitelist than to blacklist cases.

> @@ +550,5 @@
> > +
> > +    bool isPointerType = type->isPointerType();
> > +    if (isPointerType) {
> > +      fieldDecl = type->getPointeeCXXRecordDecl();
> > +    }
> 
> Why are we looking through raw pointers? I would think that we would want to
> just expect these raw pointers to be initialized to some value.
>
> @@ +630,5 @@
> > +
> > +const CXXRecordDecl* NonInitializedMemberChecker::getFieldDeclPODRecord(
> > +    FieldDecl *fieldDecl) {
> > +  const CXXRecordDecl *declPOD = fieldDecl->getType()->getAsCXXRecordDecl();
> > +  if (fieldDecl->getType()->isPointerType()) {
> 
> I don't understand the motivation behind ensuring that the fields of objects
> behind pointers are initialized. We should just check that the pointer
> itself is initialized I would think.

This is actually a good point. My initial thought was that pointers to PODs don't have their member field initialized either, but I just realized that we could be assigning an existing pointer (I was only thinking about the case where we initialize with |new|). Maybe I can try and detect when a pointer is initialized with |new| and when it is initialized from a pre-existing pointer. I will discuss that with Andi.

> @@ +606,5 @@
> > +    return;
> > +  }
> > +
> > +  SourceLocation fixitLoc = decl->getInnerLocStart().getLocWithOffset(offset);
> > +  std::string fixitString = "MOZ_UNCHECKED_INIT(";
> 
> I don't want to recommend MOZ_UNCHECKED_INIT() as the fix for not
> initializing these members. I feel like recommending initializing them to 0
> would be a better solution. I would rather not encourage using
> MOZ_UNCHECKED_INIT more than we need to.
> 
> If you want to use a fixit hint to do some automated rewriting of the tree
> for the initial landing then you should keep it around for that, but I would
> rather not land it.

My intent is to do what you describe in your second paragraph. I'll remove the fixit part from the patch.

> @@ +808,5 @@
> > +  }
> > +
> > +  // We clear the uncheckedFields cache just in case this class instance is
> > +  // reused multiple times
> > +  m_uncheckedFieldsCache.clear();
> 
> Why are you worried about keeping the same cache between multiple different
> struct checks? This class will be re-used, but that should be a good thing,
> potentially saving you unnecessary re-checks (for example if the same POD
> struct is referenced from 2 different other structs as a member).

Good catch. This is a leftover from when we were using strings to identify members.

> ::: mfbt/Attributes.h
> @@ +517,5 @@
> > + *   the time of its introduction. This allows us to focus on the new defects
> > + *   introduced later on. Using this attribute on class members disables the
> > + *   check that these member must be initialized in constructors via
> > + *   list-initialization, in the constructor body, or via functions called from
> > + *   the constructor body.
> 
> I don't find this explanation of the annotation very clear. How about:
> 
> MOZ_UNCHECKED_INIT(members...): Applies to class declarations. Marks the
> listed members as potentially uninitialized, disabling errors or warnings
> generated due to them not being initialized in class constructors. All
> non-annotated members must be initialized in constructors via list
> initialization, in the constructor body, or via functions called directly
> from the constructor.
> 
> I also don't really understand why we decided to go with
> MOZ_UNCHECKED_INIT(members...) on the class declaration, rather than just
> MOZ_UNCHECKED_INIT on the member declaration itself, which seems more
> intuitive to me.

We want to use MOZ_UNCHECKED_INIT to whitelist all existing bugs. Adding the annotation to every single uninitialized field in all classes would add a lot of noise, hence our choice to reduce it to the class declaration.
Semantically, MOZ_UNCHECKED_INIT means we're just disabling the warning because it is about existing code when adding the checker. We don't know if these fields should be initialized or not.
If someone wants to explicitly, and on purpose, disable warnings for a field because it is initialized outside or because our checker missed the initialization, MOZ_INIT_OUTSIDE_CTOR can be used in front of the field itself.

> @@ +575,5 @@
> >  #  define MOZ_NON_AUTOABLE __attribute__((annotate("moz_non_autoable")))
> >  #  define MOZ_INIT_OUTSIDE_CTOR \
> >      __attribute__((annotate("moz_ignore_ctor_initialization")))
> > +#  define MOZ_UNCHECKED_INIT(...) \ // Attributes are read backwards
> > +    __attribute__((annotate(#__VA_ARGS__))) \
> 
> Doing this seems a little sketchy to me. If we have 2 attributes which both
> want to handle lists, we would not be able to easily tell the annotations
> apart. I would prefer it if we instead had one annotation:
> 
> __attribute__((annotate("moz_unchecked_initialization=" #__VA_ARGS__)))
> 
> and in the code when looking for the annotation, we simply checked if an
> annotation starts with "moz_unchecked_initialization".

After re-reading the docs, I realized I had a complete misunderstanding of how var args work with defines. Your solution is indeed much better than the current one.

	Nika Layzell [:nika] (ni? for response)
	Comment 115 • 7 years ago

Comment on attachment 8880382 [details] [diff] [review]
525063.patch

Review of attachment 8880382 [details] [diff] [review]:
-----------------------------------------------------------------

A few more comments :). Thanks for the changes it's looking nicer.

::: build/clang-plugin/CustomMatchers.h
@@ +161,5 @@
> +      // Skip constructors in system headers
> +      !ASTIsInSystemHeader(Declaration->getASTContext(), *Declaration) &&
> +      // Skip ignored namespaces and paths
> +      !isInIgnoredNamespaceForImplicitCtor(Declaration) &&
> +      !isIgnoredPathForImplicitCtor(Declaration);

Does this not work with http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/build/clang-plugin/Utils.h#387

I would prefer we use the generic isInThirdPartyPath

::: build/clang-plugin/NonInitializedMemberChecker.cpp
@@ +15,5 @@
> +                                           const DeclaratorDecl *Decl) {
> +  auto VarFromMap = ResolverMap.find(Decl);
> +  if (VarFromMap != ResolverMap.end()) {
> +    // variable found in resolve map that means we must use it's corespondent
> +    // -> second in variablesMap

nit: We like making our comments full sentences, which includes Capitalizing the first letter, and adding a period at the end (I'm pretty bad at this :-S) - it would be awesome if you could do that too!

@@ +226,5 @@
> +    const CallExpr *FuncCall, VariablesMap &VariablesMap,
> +    ResolverMap &ResolverMap, InitFlag Flag) {
> +
> +  struct ExternFuncDefinition {
> +    std::string FuncName;

Why not use StringRef here?

@@ +230,5 @@
> +    std::string FuncName;
> +    unsigned IndexInArgList;
> +  };
> +
> +  static ExternFuncDefinition FuncSchemaArray[] = {{"memset", 0}}; // for memset

nit: remove this comment.

@@ +498,5 @@
> +      Decl->specific_attrs<AnnotateAttr>();
> +
> +  for (AnnotateAttr *Attr : Attrs) {
> +    if (Attr->getAnnotation().startswith("moz_unchecked_initialization=")) {
> +      SmallVector<StringRef, 0> FieldStrings;

You might as well take advantage of the small vector optimization, I would probably pass in 2 for the inline allocation size.

@@ +597,5 @@
> +  if (!DeclPOD) {
> +    return;
> +  }
> +
> +  if (FieldData.Flag) {

Please explicitly compare this against InitFlag. I don't like depending on the first variant being falsey.

@@ +604,5 @@
> +      if (!VariablesMap.count(Field)) {
> +        continue;
> +      }
> +
> +      VariablesMap[Field].Flag = FieldData.Flag;

If you have nested POD types, are those handled here?

@@ +615,5 @@
> +      if (!VariablesMap.count(Field)) {
> +        continue;
> +      }
> +
> +      FlagPOD = VariablesMap[Field].Flag;

This seems wrong, consider this:

struct X {
  int a;
  int b;
};

struct Y {
  X x;

  Y(int) {
    x.a = 10;
  }
  Y(float) {}

  void MOZ_INIT_OUTSIDE_CTOR Init() {
    x.b = 20;
  }
};

We would first walk through Init, and determine that x.b is initialized outside the constructor, and thus set it to InitFlag::InitByFunc.

In our first constructor we walk Y(int), which sets x.a with InitFlag::InitByCtor, then we walk the POD types, and declare x as initialized however, as we use the most recent initialized type, we mark it as InitByFunc, which is wrong.

When we then read the second constructor, we infer that x.a is initialized by a function, even though it isn't, through the first branch in this function.

@@ +616,5 @@
> +        continue;
> +      }
> +
> +      FlagPOD = VariablesMap[Field].Flag;
> +      if (!FlagPOD) {

Again, please explicitly compare to the enum variant.

@@ +710,5 @@
> +            << Var.second.FullName << Ctor
> +            << Ctor->getNameInfo().getSourceRange();
> +      } else {
> +        // Reset it only if it was init by ctor
> +        if (Var.second.Flag == InitFlag::InitByCtor) {

nit: This can be an else if rather than an if in the else :)

::: build/clang-plugin/NonInitializedMemberChecker.h
@@ +16,5 @@
> +  void registerMatchers(MatchFinder *AstMatcher) override;
> +  void check(const MatchFinder::MatchResult &Result) override;
> +
> +private:
> +  enum InitFlag : uint8_t { NotInit, InitByCtor, InitByFunc };

Can you split these variants over multiple lines, and consider marking this type as an `enum class`?

@@ +26,5 @@
> +
> +  typedef std::unordered_set<const FieldDecl *> UncheckedFieldsSet;
> +  typedef std::unordered_map<const CXXRecordDecl *, UncheckedFieldsSet>
> +      UncheckedFieldsCacheMap;
> +  typedef std::unordered_map<const FieldDecl *, FieldData> VariablesMap;

Can we call this something else, because it doesn't really have much to do with variables (It's a map of fields to whether or not they are initialized). Perhaps FieldInitMap?

@@ +41,5 @@
> +  void runThroughDefaultFunctions(const CallExpr *CallExpr,
> +                                  VariablesMap &VariablesMap,
> +                                  ResolverMap &ResolverMap, InitFlag Flag);
> +
> +  /// This method checks if a field or associated variable is the assignee in

Is "associated variable" a variable in the resolver map?

@@ +49,5 @@
> +                      ResolverMap &ResolverMap, InitFlag Flag);
> +
> +  /// Resolves the given declaration through the resolver map, allowing us to
> +  /// know if a local variable of a parameter may be indirectly refering to a
> +  /// field.

Can we reword this as "looks up the given DeclaratorDecl in the resolver map, determining the field which it aliases, if any."? That seems more clear to me.

@@ +53,5 @@
> +  /// field.
> +  const DeclaratorDecl *resolveMapVar(ResolverMap &ResolverMap,
> +                                      const DeclaratorDecl *Decl);
> +
> +  /// This updates the variable map only if the varible is not initialized

"Marks the passed-in field as initialized. Does not override the initialization flag if the field has been previously initialized."

@@ +58,5 @@
> +  void updateVarMap(VariablesMap &VariablesMap, const FieldDecl *Var,
> +                    InitFlag Flag);
> +
> +  /// When going through a function call, builds a new resolver map from an
> +  /// existing one by matching arguments and parameters together.

I don't find this comment super clear. This method is creating a new resolver map which has all of the entries from the original resolver map, but with additional links if parameters are passed in which alias local variables, right?

@@ +95,5 @@
> +
> +  /// Builds the initial variables map from the field list of the declaration,
> +  /// potentially recursing into POD fields to make sure their fields are in
> +  /// the map as well.
> +  void buildVariablesMap(VariablesMap &VariablesMap, const CXXRecordDecl *Decl,

I don't like the name of this function, but I think it will be more clear if we change the name of VariablesMap.

@@ +107,5 @@
> +  /// MOZ_IS_CLASS_INIT.
> +  void checkInitMethods(const CXXRecordDecl *Decl, VariablesMap &VariablesMap,
> +                        ResolverMap &ResolverMap);
> +
> +  /// Returns the CXXRecordDecl of the field if it's POD, else nullptr.

"Returns the fields type as a CXXRecordDecl if it is POD, else nullptr." perhaps?

@@ +110,5 @@
> +
> +  /// Returns the CXXRecordDecl of the field if it's POD, else nullptr.
> +  const CXXRecordDecl *getFieldDeclPODRecord(const FieldDecl *FieldDecl);
> +
> +  /// If all fields of a POD are initialized, this marks the POD as initialized.

and vice-versa :)

@@ +115,5 @@
> +  void updateFlagsIfPOD(const FieldDecl *Variable, FieldData &FieldData,
> +                        VariablesMap &VariablesMap);
> +
> +  /// Goes through the constructors looking for initializations and emits errors
> +  /// for each (uninitialized_field, constructor) couple.

Consider mentioning that init methods should be checked before running this method.

::: build/clang-plugin/tests/TestInitializedMemberChecker.cpp
@@ +1,2 @@
> +#include <stdint.h>
> +#include <string.h>

Can you add a test for initializing nested POD types?

e.g.

struct A { int x; }
struct B { A a; int y; }
class C {
public:
  C() {
    // This shouldn't error:
    b.y = 10;
    b.a.x = 20;
    // Nor should this:
    b.y = 10;
    b.a = SomeA;
    // Or this:
    b = SomeB;
    // But forgetting to set an inner type should:
    b.y = 10;
  }
  B b;
}

	Tristan Bourvon
	Comment 117 • 7 years ago

In addition to your comments, I realized that identifying fields only by their FieldDecl is incorrect because we can have multiple POD fields of the same type:

struct POD {
  int a;
};

class {
  POD foo;
  POD bar;
};

In the old version, foo.a and bar.a would have the same entry in the FieldInitMap. Now, the full FieldDecl path is stored as the key and allows for a unique identification of fields.

(In reply to Michael Layzell [:mystor] from comment #115)
> Comment on attachment 8880382 [details] [diff] [review]
> 525063.patch
> 
> Review of attachment 8880382 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/clang-plugin/CustomMatchers.h
> @@ +161,5 @@
> > +      // Skip constructors in system headers
> > +      !ASTIsInSystemHeader(Declaration->getASTContext(), *Declaration) &&
> > +      // Skip ignored namespaces and paths
> > +      !isInIgnoredNamespaceForImplicitCtor(Declaration) &&
> > +      !isIgnoredPathForImplicitCtor(Declaration);
> 
> Does this not work with
> http://searchfox.org/mozilla-central/rev/
> ae94cfb36d804727dfb38f2750bfcaab4621df6e/build/clang-plugin/Utils.h#387
> 
> I would prefer we use the generic isInThirdPartyPath

Woops missed that one review part, it's fixed now :) 

> @@ +604,5 @@
> > +      if (!VariablesMap.count(Field)) {
> > +        continue;
> > +      }
> > +
> > +      VariablesMap[Field].Flag = FieldData.Flag;
> 
> If you have nested POD types, are those handled here?
>
> ::: build/clang-plugin/tests/TestInitializedMemberChecker.cpp
> @@ +1,2 @@
> > +#include <stdint.h>
> > +#include <string.h>
> 
> Can you add a test for initializing nested POD types?
> 
> e.g.
> 
> struct A { int x; }
> struct B { A a; int y; }
> class C {
> public:
>   C() {
>     // This shouldn't error:
>     b.y = 10;
>     b.a.x = 20;
>     // Nor should this:
>     b.y = 10;
>     b.a = SomeA;
>     // Or this:
>     b = SomeB;
>     // But forgetting to set an inner type should:
>     b.y = 10;
>   }
>   B b;
> }


Thanks for catching this bit, I added a new way to recursively initialize PODs depending on their parents and children, and it seems to work now!

> 
> @@ +615,5 @@
> > +      if (!VariablesMap.count(Field)) {
> > +        continue;
> > +      }
> > +
> > +      FlagPOD = VariablesMap[Field].Flag;
> 
> This seems wrong, consider this:
> 
> struct X {
>   int a;
>   int b;
> };
> 
> struct Y {
>   X x;
> 
>   Y(int) {
>     x.a = 10;
>   }
>   Y(float) {}
> 
>   void MOZ_INIT_OUTSIDE_CTOR Init() {
>     x.b = 20;
>   }
> };
> 
> We would first walk through Init, and determine that x.b is initialized
> outside the constructor, and thus set it to InitFlag::InitByFunc.
> 
> In our first constructor we walk Y(int), which sets x.a with
> InitFlag::InitByCtor, then we walk the POD types, and declare x as
> initialized however, as we use the most recent initialized type, we mark it
> as InitByFunc, which is wrong.
> 
> When we then read the second constructor, we infer that x.a is initialized
> by a function, even though it isn't, through the first branch in this
> function.

I added a new MixedInit flag to account for the cases where a POD is partially initialized in a constructor and in a method.

	Tristan Bourvon
	Comment 118 • 7 years ago

Small fixes for the patch uploaded 15 minutes ago.

	Nika Layzell [:nika] (ni? for response)
	Comment 119 • 7 years ago

Comment on attachment 8882233 [details] [diff] [review]
525063.patch

Review of attachment 8882233 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the long delay on this review! Was too busy on Friday, and just got back to work today after Canada Day :).

::: build/clang-plugin/NonInitializedMemberChecker.cpp
@@ +793,5 @@
> +    for (auto &Var : FieldInitMap) {
> +      // Reset only if initialized by tthis constructor (completely or
> +      // partially).
> +      if (Var.second.Flag == InitFlag::InitByCtor ||
> +          Var.second.Flag == InitFlag::InitMixed) {

Not sure this is necessary (I think you could just use InitByCtor everywhere InitMixed is used) but it makes it clearer, so I like it :).

@@ +812,5 @@
> +    return;
> +  }
> +
> +  // Build the fields map (recursively on PODs) to know which fields should
> +  // be initialized

nit: periods at the end of comments, here and everywhere.

	Tristan Bourvon
	Comment 120 • 7 years ago

Don't worry for the delay, you've been very quick to respond all the other times ;)

This fixes grammar, punctuation and basic explanation mistakes in the comments.

	Nika Layzell [:nika] (ni? for response)
	Comment 121 • 7 years ago

Comment on attachment 8883563 [details] [diff] [review]
525063.patch

Review of attachment 8883563 [details] [diff] [review]:
-----------------------------------------------------------------

carrying over r=me assuming that all you changed was comments (didn't re-read the code)

	Tristan Bourvon
	Comment 122 • 7 years ago

You shouldn't have to re-read all the code as most changes are just minor tweaks to fix some corner cases I encountered while running the checker on m-c. I have done extensive testing and I'm fairly confident in the logical aspect of the algorithm.

The main thing to review here is the introduction of a database.json file that the checker reads from to know which fields to ignore. We discussed this with Andi and Sylvestre and we believe it's a much nicer way to handle issues whitelisting in comparison with the MOZ_UNCHECKED_INIT macro we had before, which was pretty invasive.

	Tristan Bourvon
	Comment 123 • 7 years ago

This is the patch which adds whitelisting to the database. By default, thanks to macros that can be controlled from moz.build, it doesn't do anything different from the main patch. However, when whitelisting to database is activated, it writes to the database file all the fields which should be ignored in the future, by using a lock in order to prevent races (as clang-tidy is often run in parallel).

Let me know if you need any more info on this as I'm aware it might not be easy to get into with all the platform specific stuff. I'm available on IRC anytime I'm not asleep ;)

	Nika Layzell [:nika] (ni? for response)
	Comment 124 • 7 years ago

Comment on attachment 8893826 [details] [diff] [review]
525063.patch

Review of attachment 8893826 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like that we imported a new JSON parser into the tree, especially because clang already has one ^.^.

Clang has an internal yaml parser which it uses to parse JSON (JSON is a subset of YAML), so perhaps we could use that? (example usage: https://github.com/llvm-mirror/clang/blob/1b373f4999b64c4b56bf8ef4ce28e01da976ba19/lib/Tooling/JSONCompilationDatabase.cpp#L266-L372).

I'd also like ehsan's feedback as to whether or not he thinks that the JSON static analysis opt-out list is a good solution.

	Nika Layzell [:nika] (ni? for response)
	Comment 125 • 7 years ago

Comment on attachment 8893828 [details] [diff] [review]
525063-database.patch

Review of attachment 8893828 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review for now

	(no longer active)
	Comment 126 • 7 years ago

I don't think landing this analysis without fixing the existing places where it flags out errors is a good idea.  We have had security bugs in the past due to these types of issues, and this will give black hat hackers a recipe for finding those places in our code to poke through.  It seems that fixing the issues this analysis finds should be fairly simple.  Tristan, can you please explain why you chose this approach?  Why not fix those issues first?

	Tristan Bourvon
	Comment 127 • 7 years ago

(In reply to Michael Layzell [:mystor] from comment #124)
> Comment on attachment 8893826 [details] [diff] [review]
> 525063.patch
> 
> Review of attachment 8893826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't like that we imported a new JSON parser into the tree, especially
> because clang already has one ^.^.
> 
> Clang has an internal yaml parser which it uses to parse JSON (JSON is a
> subset of YAML), so perhaps we could use that? (example usage:
> https://github.com/llvm-mirror/clang/blob/
> 1b373f4999b64c4b56bf8ef4ce28e01da976ba19/lib/Tooling/JSONCompilationDatabase.
> cpp#L266-L372).
> 
> I'd also like ehsan's feedback as to whether or not he thinks that the JSON
> static analysis opt-out list is a good solution.

Oh I'm sorry, I forgot to explain the JSON bit. The reason for importing the JSON parser into the tree is that using anything else from m-c will make the process of exporting the checker to clang-tidy very complicated. Hence the need to have the parser in the clang-tidy folder.

However, I was not aware that Clang had a JSON parser, which seems to solve the whole issue here. Thanks for pointing that out!
I'm going to adapt the code to make use of clang's parser.



(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away Aug 7-8) from comment #126)
> I don't think landing this analysis without fixing the existing places where
> it flags out errors is a good idea.  We have had security bugs in the past
> due to these types of issues, and this will give black hat hackers a recipe
> for finding those places in our code to poke through.  It seems that fixing
> the issues this analysis finds should be fairly simple.  Tristan, can you
> please explain why you chose this approach?  Why not fix those issues first?

We had a pretty long discussion about this with Andi and Sylvestre: knowing that the checker finds several thousand issues, it seems unreasonable to try to fix all of them by hand. We thought about using an autofix but it doesn't seem doable:
 - needlessly initializing variables has a performance cost: some variables should be marked with MOZ_INIT_OUTSIDE_CTOR instead of being initialized.
 - zero may not be a reasonable default for some variables, and some behaviour in the code right now might rely (wrongfully, of course) on that.

Now obviously the choice is not easy, and depending on your guesses and estimates it might be worth it to autofix everything with default construction, but the best approach for now seemed to be to whitelist all existing issues and only complain about the new ones. What's your opinion on this?

	Sylvestre Ledru [:Sylvestre]
	Comment 128 • 7 years ago

(In reply to Michael Layzell [:mystor] from comment #124)
> I don't like that we imported a new JSON parser into the tree, 

Especially when we already have a copy toolkit/components/jsoncpp/