Page MenuHomePhabricator

[analyzer][Review request] Improved checker lists.
Needs ReviewPublic

Authored by ayartsev on Apr 22 2014, 3:02 PM.

Details

Reviewers
jordan_rose
Summary

Here are the renewed checker lists we started to switch to a long time ago. The last suggestion was to split examples to produce one warning per example. Attached patch addresses this. It also contain lots of new improvements. Since the last review:

  • made the list of actual checkers to look like the list of potential checkers, added examples;
  • tested, reviewed and updated examples, removed odd and outdated ones, added new;
  • split examples to produce a single warning per example;
  • improved html structure and markup;
  • added the improved expand/collapse feature;
  • reviewed and improved descriptions, added missing <code> tegs, added missing periods;
  • pointed to the sources related to the problem the checker evolves;

Diff Detail

Event Timeline

ayartsev updated this revision to Diff 8743.Apr 22 2014, 3:02 PM
ayartsev retitled this revision from to [analyzer][Review request] Improved checker lists..
ayartsev updated this object.
ayartsev edited the test plan for this revision. (Show Details)
ayartsev added a reviewer: jordan_rose.
ayartsev added a subscriber: Unknown Object (MLST).
jordan_rose edited edge metadata.Apr 28 2014, 6:03 PM

I committed most of this for you in r207478, just to split it up. (Sorry if that was presumptuous.) Among the modifications I made were replacing the <div class="separator"></div> with slightly cleverer CSS, tweaking the ZeroAllocDereference checker to have an example that wasn't just a bounds check, making the categories slightly clearer, and commenting out undefbehavior.DeadReferenced.

Why the last one? Because a lot of the examples didn't show uses of the previous object, but rather uses of the new object that might look like uses of the previous object. I didn't want to spend all of our time cleaning it up before getting the rest of the improvements in, though, so I just hid it inside a comment for now.

I also dropped the link from checker_dev_manual.html because implicit_checks.html doesn't seem to exist. What was that change intended to do?

ayartsev updated this revision to Diff 8906.Apr 29 2014, 3:45 AM
ayartsev edited edge metadata.

Thanks for committing this! My fault, did not included alpha_checks.html, implicit_checks.html and expandcollapse.js to the previous patch.
Additionally fixed table headers in available_checks.html, fixed paddings/margins of all table headers and updated undefbehavior.DeadReferenced.

I'm still not sold on undefbehavior.DeadReferenced. Except for the delete, none of these are actually trying to use the old object...they're using the new object. (In some cases incorrectly, but still.) Using the old object would involve just calling the destructor, or keeping a pointer to a field within the object, or similar.

Not sure that I've got the idea. Each example is designed for the particular case of a dangerous pointer usage. In order:
The first example illustrates the usage of a pointer as an operand of a delete-expression - OK.
The second example is a modified example from the C++ spec - a call to a virtual function of an object whose lifetime has ended. Did you mean to call a destructor or keep a pointer to a field within the object instead? Isn't a call to a virtual function a usage of an old object?
The last three examples illustrate the following cases:

  • the pointer is implicitly converted to a pointer to a base class type
  • the pointer is used as the operand of a static_cast (except when the conversion is to void*, or to void* and subsequently to char*, or unsigned char*)
  • the pointer is used as the operand of a dynamic_cast

Do you think the corresponding tests should be modified somehow?

Okay, let's look at this example:

class A {
public:
  virtual void f();
};

class B : public A {
public:
  void f();
};

void test() {
  A *a = new A;
  new(a) B;
  a->f(); // warn
}

Both A and B have f() methods, so it's okay to invoke f() on an instance of type B (whether through an A* or a B*). This code is perfectly acceptable:

void test() {
  A *a = new A;
  a->~A();
  static_assert(sizeof(A) == sizeof(B) && alignof(A) == alignof(B),
                "different allocation requirements");
  new(a) B;
  a->f(); // okay, calls B::f.
}

In fact, this would be okay even if A::f weren't virtual.

I think the interesting thing to check is whether you are reinitializing an object whose destructor hasn't been called, and if there are any references to sub-objects used after the destructor has been called. Simply accessed members of the new object through the existing pointer is not an error.

ayartsev updated this revision to Diff 9083.May 5 2014, 11:34 AM

Substituted the second example with the one that uses a destructor call.
References to sub-objects used after the destructor has been called are the matter of undefbehavior.MemberRefAfterDtor. Reinitialization of an object whose destructor hasn't been called is a good idea for an another checker!
This checker is designed for the particular cases described in C++03 3.8p5, p7 and C++11 3.8p5, p7.

C++11 3.8p5: Before the lifetime of an object has started but after the storage which the object will occupy has been
allocated or, after the lifetime of an object has ended and before the storage which the object occupied is
reused or released, any pointer that refers to the storage location where the object will be or was located
may be used but only in limited ways. For an object under construction or destruction, see 12.7. Otherwise,
such a pointer refers to allocated storage (3.7.4.2), and using the pointer as if the pointer were of type void*,
is well-defined. Such a pointer may be dereferenced but the resulting lvalue may only be used in limited
ways, as described below. The program has undefined behavior if:
— the object will be or was of a class type with a non-trivial destructor and the pointer is used as the
operand of a delete-expression,
— the pointer is used to access a non-static data member or call a non-static member function of the
object, or
— the pointer is implicitly converted (4.10) to a pointer to a base class type, or
— the pointer is used as the operand of a static_cast (5.2.9) (except when the conversion is to void*,
or to void* and subsequently to char*, or unsigned char*), or
— the pointer is used as the operand of a dynamic_cast (5.2.7). [ Example:
#include <cstdlib>
struct B {
virtual void f();
void mutate();
virtual ~B();
};
struct D1 : B { void f(); };
struct D2 : B { void f(); };
void B::mutate() {
new (this) D2; reuses storage — ends the lifetime of *this
f();
undefined behavior
... = this; OK, this points to valid memory
}
void g() {
void* p = std::malloc(sizeof(D1) + sizeof(D2));
B* pb = new (p) D1;
pb->mutate();
&pb;
OK: pb points to valid memory
void* q = pb; OK: pb points to valid memory
pb->f();
undefined behavior, lifetime of *pb has ended
}

ayartsev updated this revision to Diff 9089.May 5 2014, 2:55 PM

Update to the patch after r207995. Enabling of the expandcollapse feature added.

Did you write expandcollapse.js yourself? Or does it have a license we need to mention? Don't forget the images when you commit.

Content looks good! (with a few inline comments, of course). If there are no issues with the license thing, please commit.

www/analyzer/alpha_checks.html
21

We should probably include something like: "These are checkers with known issues or limitations that keep them from being on by default. They are likely to have false positives. Bug reports are welcome but will likely not be investigated for some time. Patches welcome!"

104

It supports more than just operators now. "Warn about suspicious uses of identical expressions"?

The expandcollapse.js is written by me from scratch as well as button images. Committed as r209131.