Page MenuHomePhabricator

Analyzer: Fix a crasher in UbigraphViz

Authored by ismailp on Aug 18 2015, 3:01 PM.



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.

Diff Detail


Event Timeline

ismailp updated this revision to Diff 32459.Aug 18 2015, 3:01 PM
ismailp retitled this revision from to Analyzer: Fix a crasher in UbigraphViz.
ismailp updated this object.
ismailp added reviewers: zaks.anna, krememek.
ismailp added a subscriber: cfe-commits.
ismailp added inline comments.Aug 18 2015, 3:06 PM
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 added inline comments.Aug 18 2015, 3:18 PM
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 added inline comments.Aug 18 2015, 3:20 PM
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 added inline comments.Aug 18 2015, 3:52 PM
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 updated this revision to Diff 32745.Aug 20 2015, 2:08 PM
  • 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 added inline comments.Aug 21 2015, 10:09 PM
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 added inline comments.Aug 21 2015, 10:11 PM
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 updated this revision to Diff 32903.Aug 22 2015, 4:39 AM
  • Change parameter name to OutStream.
ismailp added inline comments.Aug 22 2015, 4:46 AM
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, 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)};

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.

zaks.anna edited edge metadata.Sep 3 2015, 2:49 PM

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 updated this revision to Diff 34082.Sep 4 2015, 2:57 PM
ismailp edited edge metadata.
  • 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.

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 added inline comments.Sep 14 2015, 12:57 PM
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.

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.

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

zaks.anna accepted this revision.Sep 17 2015, 6:35 PM
zaks.anna edited edge metadata.

Otherwise, LGTM.

This revision is now accepted and ready to land.Sep 17 2015, 6:35 PM
ismailp updated this revision to Diff 35141.Sep 18 2015, 2:50 PM
ismailp edited edge metadata.
  • Added a new test file instead of adding '-analyzer-viz-egraph-ubigraph' to an existing test.
This revision was automatically updated to reflect the committed changes.

Thank you for reviewing!

Thanks for the fix! We'll commit shortly.