Analyzer: Fix a crasher in UbigraphViz

[ismailp]

Summary

Name Out refers to the parameter. It is moved into the member Out
in ctor-init. Dereferencing null pointer will crash clang, if user
passes '-analyzer-viz-egraph-ubigraph' argument.

[ismailp]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

749 ↗

(On Diff #32459)

Is Ubigraph generator actively maintained? If I run tests with '-analyzer-viz-egraph-ubigraph', this assertion gets triggered. According to static analyzer documentation, cycles are allowed in ExplodedGraph. I am unsure whether self-cycles are allowed, however.

[krememek]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

784 ↗

(On Diff #32459)

This seems really brittle.

Can we just rename the parameter 'Out' to something else, and then just use 'Out'? Seems more reasonable to rename the parameter than to change every single functional line of code that uses 'Out'.

[krememek]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

749 ↗

(On Diff #32459)

There was a time where a self-cycle indicated a bug in the analyzer. Much has changed since destructor support was added, but the Ubigraph support hasn't bitrotted. It could be the case that this assertion needs to be removed because an invariant in the analyzer has changed, but I would be willing to also say it is possible that the tests are failing because an invariant is getting violated.

[ismailp]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

784 ↗

(On Diff #32459)

Sure, I can rename the parameter!

Advertorial; I have done some upgrades to my patch D10425. Currently, I am trying to make it an analysis based warning. It can diagnose this problem, and asks; "did you mean 'this->Out'?". Perhaps, that patch should suggest renaming the parameter instead of 'this->' fixit :)

[ismailp]

• Renamed Out parameter to Stm.
• Removed assertion that checks whether an ExplodedNode has an edge to itself.
• Added '-analyzer-viz-egraph-ubigraph' to an analyzer invocation in a test.

[krememek]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

778 ↗

(On Diff #32745)

While succinct, I think 'Stm' is an actively harmful name as it conveys no meaning. There's no need to hyper optimize here.

How about 'outStream', or something that clearly indicates what it is.

You're only going to use it twice. No harm in spelling it out. 'Stm' could mean a bunch of things.

[krememek]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

749 ↗

(On Diff #32745)

Did you look at the test case that causes this assertion to fail? I think it would be good to know if this assertion is actually safe to remove. I'm a bit skeptical that it is safe to remove, and that (per my last review) that this may be detecting that an invariant is getting violated. If you are not certain how to investigate that part, please report which test is triggering the problem and myself or someone else familiar with the engine core can take a look. Thanks.

[ismailp]

• Change parameter name to OutStream.

[ismailp]

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

749 ↗

(On Diff #32745)

There wasn't a test that checks Ubigraph generator. After making the patch below, I picked a few existing tests, and added '-analyzer-viz-egraph-ubigraph' to their RUN lines. Then, I ran lit.py, and 'tests/Analysis/cfg.cpp' has crashed. I have minimized the test case to understand the problem.

Self-loop happens during implicit destructor of Aggregate in the following minimized test case:

struct LifetimeExtend { LifetimeExtend(int); ~LifetimeExtend(); };
struct Aggregate { const LifetimeExtend a; const LifetimeExtend b; };
void test_lifetime_extended_temporaries() {
  {
    Aggregate aggregate{LifetimeExtend(4), LifetimeExtend(4)};
    4;
  }
}

The destructor of a has a self-loop. My hypothesis is that a and b have the same ProgramStates. b's destructor is visited first, since it's destroyed first. When destructor of a gets visited, analyzer calls ProgramState::Profile and finds the same state as b's destructor. Therefore, both a's destructor and b's destructor have the same ExplodedNode. Do you think this is correct?

I have added -analyzer-viz-egraph-ubigraph to a few more C++ tests. I didn't observe any crash. However, I didn't verify that it generates correct ubigraph output.

[ismailp]

Ping!

[zaks.anna]

I do not understand why there should be a cycle here in the Exploded Graph. Most likely this assertion uncovers a bug in cfg-temporary-doors, which is a disabled by default feature (it has not been completed yet). You can think of "ProgramState" as the state of the program, so it would have information about the values of both, 'a' and 'b' at the same time. You are right, maybe while we are evaluating the destructor, the distinction between 'a' and 'b' is lost, but that's a bug in how the temporary destructors are handled.

I do not think the assertion should be removed.

[ismailp]

• Rolled back to the first version of patch, where only parameter Out is renamed to OutStream
• Use a different test that doesn't trigger the self-loop assertion.

[ismailp]

Thank you for the feedback! I have added the test in a C file, so we can get the first part of the patch in -- between lines 778-780.

I think self-loop is a different problem, and that requires its own patch. I'll see what I can do about self-loop, if someone else isn't working on it.

[zaks.anna]

test/Analysis/free.c

1 ↗

(On Diff #34082)

I am not sure how to best test ubigraph, but adding it to an existing test is not great. Some builedbot machines might not have ubigraph installed.

I suggest to commit the fix without any changes to the tests and investigate how it can be tested later on.

[ismailp]

I don't have ubigraph installed either. The purpose of the test isn't to check as to whether we can generate a conforming/sensible ubigraph output, but to ensure that this tiny patch works and clang doesn't crash. So, I'd keep it. But if you are worried about other/unknown problems in ubigraph generator that the test might surface, then we can proceed without the test, and a ubigraph expert can write an output validation test.

[zaks.anna]

I see, Maybe we should add a new test file to test this instead of adding it to an existing test.

[zaks.anna]

Otherwise, LGTM.

[ismailp]

• Added a new test file instead of adding '-analyzer-viz-egraph-ubigraph' to an existing test.

[ismailp]

Thank you for reviewing!

[zaks.anna]

Thanks for the fix! We'll commit shortly.

cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

else {
     *Out << "('change_vertex_style', " << DstID << ", 1)\n";
   }
 
   // Add the edge.
   *Out << "('edge', " << SrcID << ", " << DstID
        << ", ('arrow','true'), ('oriented', 'true'))\n";
 }
 
-UbigraphViz::UbigraphViz(std::unique_ptr<raw_ostream> Out, StringRef Filename)
-    : Out(std::move(Out)), Filename(Filename), Cntr(0) {
+UbigraphViz::UbigraphViz(std::unique_ptr<raw_ostream> OutStream,
+                         StringRef Filename)
+    : Out(std::move(OutStream)), Filename(Filename), Cntr(0) {
 
   *Out << "('vertex_style_attribute', 0, ('shape', 'icosahedron'))\n";
   *Out << "('vertex_style', 1, 0, ('shape', 'sphere'), ('color', '#ffcc66'),"
           " ('size', '1.5'))\n";
 }
 
 UbigraphViz::~UbigraphViz() {
   Out.reset();

cfe/trunk/test/Analysis/ubigraph-viz.cpp

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.API -analyzer-viz-egraph-ubigraph -verify %s
+// expected-no-diagnostics
+
+int f(int x) {
+  return x < 0 ? 0 : 42;
+}
+