Page MenuHomePhabricator

[WIP] [analyzer] Dump counterexample traces as C programs
AbandonedPublic

Authored by george.karpenkov on Dec 4 2017, 3:07 PM.

Details

Summary

This patch is work in progress, but has already shown itself to be useful.

For large traces understanding the analyzer output can be painful, especially when arrows indicating the flow start to loop around.
This patch provides an alternative output method: simply iterate over all program points in a bug report, and dump them to the associated C statements (with program-line-based granularity).
In the ideal case (which we currently don't aim to get) the resulting program is a refinement of an input which goes through the indicated error path.
In the current case, it is a C-like visualization (as some statements are unexpectedly dropped, and some context is missing) of what the error path is.

Sample output is:

gram.c:18444|int yychar;
gram.c:18447|YYSTYPE yylval;
gram.c:18450|int yynerrs;
gram.c:18452|YYLTYPE yylloc;
gram.c:18454|  register int yystate;
gram.c:18455|  register int yyn;
gram.c:18456|  int yyresult;
gram.c:18458|  int yyerrstatus;
gram.c:18460|  int yytoken = 0;
gram.c:18471|  short	yyssa[YYINITDEPTH];
gram.c:18472|  short *yyss = yyssa;
gram.c:18473|  register short *yyssp;
gram.c:18476|  YYSTYPE yyvsa[YYINITDEPTH];
gram.c:18477|  YYSTYPE *yyvs = yyvsa;
gram.c:18478|  register YYSTYPE *yyvsp;
gram.c:18481|  YYLTYPE yylsa[YYINITDEPTH];
gram.c:18482|  YYLTYPE *yyls = yylsa;
gram.c:18483|  YYLTYPE *yylsp;
gram.c:18484|  YYLTYPE *yylerrsp;
gram.c:18488|  YYSIZE_T yystacksize = YYINITDEPTH;
gram.c:18492|  YYSTYPE yyval;
gram.c:18493|  YYLTYPE yyloc;
gram.c:18497|  int yylen;
gram.c:18501|  yystate = 0;
gram.c:18502|  yyerrstatus = 0;
gram.c:18503|  yynerrs = 0;
gram.c:18504|  yychar = YYEMPTY;		/* Cause a token to be read.  */
gram.c:18511|  yyssp = yyss;
gram.c:18512|  yyvsp = yyvs;
gram.c:18513|  yylsp = yyls;
gram.c:18646|  if (yyn == YYFINAL)
gram.c:18653|  if (yychar != YYEOF)
gram.c:18654|    yychar = YYEMPTY;
gram.c:18656|  *++yyvsp = yylval;
gram.c:18657|  *++yylsp = yylloc;
gram.c:18661|  if (yyerrstatus)
gram.c:18664|  yystate = yyn;
gram.c:18523|  yyssp++;
gram.c:18526|  *yyssp = yystate;
gram.c:18528|  if (yyss + yystacksize - 1 <= yyssp)
gram.c:18608|  yyn = yypact[yystate];
gram.c:18609|  if (yyn == YYPACT_NINF)
gram.c:18615|  if (yychar == YYEMPTY)
gram.c:18618|      yychar = YYLEX;
gram.c:18621|  if (yychar <= YYEOF)
gram.c:18628|      yytoken = YYTRANSLATE (yychar);
gram.c:18634|  yyn += yytoken;
gram.c:18635|  if (yyn < 0 || YYLAST < yyn || yycheck[yyn] != yytoken)
gram.c:18637|  yyn = yytable[yyn];
gram.c:18638|  if (yyn <= 0)
gram.c:18640|      if (yyn == 0 || yyn == YYTABLE_NINF)
gram.c:18642|      yyn = -yyn;
gram.c:18683|  yylen = yyr2[yyn];
gram.c:18693|  yyval = yyvsp[1-yylen];
gram.c:18696|  YYLLOC_DEFAULT (yyloc, (yylsp - yylen), yylen);
gram.c:18698|  switch (yyn)
gram.y:11696|					TypeName *t = makeTypeNameFromNameList(yyvsp[-4].list);
gram.y:11697|					ListCell *lc;
gram.y:11704|					foreach(lc, yyvsp[-2].list)
gram.y:11714|					t->typmods = yyvsp[-2].list;
gram.y:11715|					t->location = yylsp[-4];

Diff Detail

Event Timeline

george.karpenkov edited the summary of this revision. (Show Details)
NoQ edited edge metadata.Dec 5 2017, 1:07 PM

This looks great for understanding what exactly is going on in a large real-world report.

If you want to turn this into a form of user-facing analyzer output variant, you may want to implement it as a PathDiagnosticConsumer rather than a visitor. If we only want a neat debugging mechanism, then you might want to stick it directly into BugReporter::emitReport() or maybe BugReporter::FlushReport() (not sure how both of these interact with report deduplication).

NoQ added a comment.Dec 5 2017, 1:11 PM
In D40809#945551, @NoQ wrote:

If you want to turn this into a form of user-facing analyzer output variant, you may want to implement it as a PathDiagnosticConsumer rather than a visitor.

Or maybe no, actually it's totally wrong, never mind. PathDiagnosticConsumer only operates on diagnostics, and it no longer knows anything about nodes, so the info is lost by that time. We could probably still go this way by making a visitor emit special per-line path diagnostics that are all ignored by all consumers except yours. This may actually be the best way to go, but such patch would be very intrusive because there are many switches by diagnostic kinds all over the place and they all would need to be updated.

NoQ added a comment.Dec 5 2017, 5:49 PM

So we have an ExplodedGraph, in that there's an ExplodedNode against which the report is being thrown, we have a bunch of BugReporterVisitors that walk from that node to the root of the graph and mark places they find interesting with PathDiagnosticPieces of different kinds, and PathDiagnosticConsumers (such as Plist or Html) that look at the vector of pieces and display it to the user.

It sounds solid (even if a bit over-engineered) to implement your counterexample generator as a set of:

  1. new class of PathDiagnosticPieces that holds information that you need about a single line of code. Like, a while ago i was adding blue bubbles to scan-build (D24278), you might use this as an example on how to add a new class of diagnostic pieces.
  2. a new BugReporterVisitor attached to all reports that puts such piece on every line of code. You already have it, but now it'd return the piece instead of printing stuff to console. Now that i think of it, the best place to add a visitor to all reports would be GRBugReporter::generatePathDiagnostic() - this is where other non-checker-specific visitors are added.
  3. a new PathDiagnosticConsumer that would transform your new pieces into report files or however you want to present the results. I guess you could follow the examples of the existing consumers here; i didn't hack much on those, but there doesn't seem to be much boilerplate here. Most of the complicated processing, if any, goes here, the visitor can at best provide a set of pieces in chronological order, he cannot rearrange them anyhow but the consumer can.
george.karpenkov edited the summary of this revision. (Show Details)

This is seems like a very useful visualization, *especially* for loops. Can we this patch get it into a state where it can be committed and used for debugging purposes?

One possibility is to turn this into a debug checker similar to debug.ViewExplodedGraph. That checker registers for a checkEndAnalysis() callback and traverses the node graph (see DebugCheckers.cpp). Can you do the same here? It doesn't look like you really need this to be a BugReporterVisitor -- and making it a debug checker would avoid outputting multiple copies for each diagnostic consumer.

You could also control file output with an analyzer-config option that takes an optional path.

alexshap added inline comments.Dec 13 2017, 9:21 PM
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1976

spaces around '=' (maybe clang-format this diff ?)

NoQ added a comment.EditedDec 13 2017, 11:01 PM

One possibility is to turn this into a debug checker similar to debug.ViewExplodedGraph. That checker registers for a checkEndAnalysis() callback and traverses the node graph (see DebugCheckers.cpp). Can you do the same here? It doesn't look like you really need this to be a BugReporterVisitor -- and making it a debug checker would avoid outputting multiple copies for each diagnostic consumer.

These prints are only for actual bugs, not for the whole graph. Even if we identify error nodes in the final graph, we're unlikely to identify which of them are suppressed by visitors.

In D40809#954890, @NoQ wrote:

One possibility is to turn this into a debug checker similar to debug.ViewExplodedGraph. That checker registers for a checkEndAnalysis() callback and traverses the node graph (see DebugCheckers.cpp). Can you do the same here? It doesn't look like you really need this to be a BugReporterVisitor -- and making it a debug checker would avoid outputting multiple copies for each diagnostic consumer.

These prints are only for actual bugs, not for the whole graph. Even if we identify error nodes in the final graph, we're unlikely to identify which of them are suppressed by visitors.

As it stands, this is a debugging tool not a way to communicate error paths to end users. (I think that, too, would be very helpful but I'd like to see this as a debugging aid first.) The workflow for debugging would be more like viewing the exploded graph than (say) emitting html diagnostics.

My point is this: this is valuable as a debugging tool, we should get it committed as such. We can work on making it user facing separately.

NoQ added a comment.Dec 14 2017, 2:46 PM

As it stands, this is a debugging tool not a way to communicate error paths to end users. (I think that, too, would be very helpful but I'd like to see this as a debugging aid first.) The workflow for debugging would be more like viewing the exploded graph than (say) emitting html diagnostics.

My point is this: this is valuable as a debugging tool, we should get it committed as such. We can work on making it user facing separately.

Per-report dumps are still much more useful for a debugging tool than whole-graph dumps. I guess such tool would be used on live code, not on reduced examples, and in this case there would be a *lot* of noise we'd want to avoid.

@dcoughlin my current iteration creates a PathDiagnosticConsumer which outputs HTML with this report. I think that makes much more sense (as essentially this is a way of visualizing the error path).

george.karpenkov edited the summary of this revision. (Show Details)

Fixed formatting, moved output to diagnostic consumer.

@dcoughlin @NoQ I think this version is reasonable enough to get committed. Another easy iteration would be to change visitor to simply add the diagnostic to path, and move the actual printing to CounterexampleDiagnostics.

Are there any plans to add tests for this ?

@alexshap That's a good question, and honestly I am not sure.
It is probably a good idea to have the tests which run the counterexample dumper and check that it does not crash.
As for the contents, I'm not sure: I would like to switch to generating HTML, and testing HTML output is IMO close to useless, because it has all the presentation stuff in it.
(it could have been possible to e.g. make the output mode which generates JSON, test that, and then use javascript templating to convert it to HTML, but that would not allow reusing existing code for outputting HTML formatted code with macros expanded properly)