Working on my first patch for the static analyzer, few questions.

Hi everyone,

I’ve been writing one of the potential checkers from the clang-analyzer website. It detects calls to member functions before all base classes are initialized in a constructor initializer list, which the standard says is undefined behavior (C++11 12.6.2.p13.)

My checker finds these cases fine, it runs perfectly on the examples given from the standard, and I ran it on the LLVM codebase and it even found three instances of this (calls to allocHungoffUses in lib/IR/Instructions.cpp if you’re curious, lines 89, 196, and 3579.)

Questions,

1. Does this seem like it might be better as a compiler warning? Right now the checker only uses checkASTDecl, and so is not path sensitive. It could be made path sensitive in the future to detect code indirectly using ’this’, however.

2. Does the ReportBug method look sensible? Given the check is not path sensitive, I couldn’t quite figure out what the PathDiagnosticLocation class is supposed to represent, or whether or not I’m using it correctly because most of the constructors take an ExplodedNode. The error messages my checker gives out looks reasonable to me, so it works, but I’m not sure if it’s idiomatic usage of the reporting API.

Example error:

/Users/emilybellows/Workspace/llvm/lib/IR/Instructions.cpp:89:17: warning:
Method called before all bases were initialized
                allocHungoffUses(PN.getNumOperands()), PN.getNumOperands()),
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

3. Who is the code owner of this part of clang for when I end up submitting the patch. Is the owner Chris Lattner by default? And does this patch look ready to submit to cfe-commits?

I welcome any other comments too, this is my first time working with either LLVM or Clang, and my first time contributing code to an open source project.

Thanks,
Emily

newchecker.patch (5.48 KB)

Hi,

This probably belongs to cfe-commits instead.

Regarding FindMethodCallAnywhere(): would this find method calls that are not direct children of the initializer Expr? For example:

struct Moo {

explicit Moo(int n) {}

};

int global_func(int x) { return x; }

struct Foo : Moo {

Foo() : Moo(global_func(member_func())) {}

int member_func() { return 42; }

};

If not, consider using a RecursiveASTVisitor instead. Regardless of whether it currently works, you should test for this in your test file.

Otherwise, it’s looking good to me, good job!

I also think that this would definitely qualify to become a compiler warning.

You’re right, it doesn't. I hadn’t run into RecursiveASTVisitor yet, I will rewrite it to use that and to test for that in the test file.

Thanks for your help!

Hi everyone,

I’ve been writing one of the potential checkers from the clang-analyzer
website. It detects calls to member functions before all base classes are
initialized in a constructor initializer list, which the standard says is
undefined behavior (C++11 12.6.2.p13.)

My checker finds these cases fine, it runs perfectly on the examples given
from the standard, and I ran it on the LLVM codebase and it even found
three instances of this (calls to allocHungoffUses in
lib/IR/Instructions.cpp if you’re curious, lines 89, 196, and 3579.)

Questions,

1. Does this seem like it might be better as a compiler warning?

This warning does seem well suited for including within the compiler.
Clang already detects other uninitialized uses in field initializers. The
code for the Clang warnings is in UninitializedFieldVisitor in
lib/Sema/SemaDeclCXX.cpp

Thanks for the info. I was just looking into how I would implement it as a compiler warning and looking for where the code would go.