This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][NFC] Refactoring BugReporter.cpp P5.: Compact mile long function invocations into objects
ClosedPublic

Authored by Szelethus on Jul 30 2019, 3:37 PM.

Details

Summary

This patch is scary big, I'm aware, but I genuinely didn't find a satisfying splitting point. To my defense, its also brain dead refactoring for the most part!


In D65379, I briefly described the construction of bug paths from an ExplodedGraph. This patch is about refactoring the code processing the bug path into a bug report.

A part of finding a valid bug report was running all visitors on the bug path, so we already have a (possibly empty) set of diagnostics for each ExplodedNode in it.
Then, for each diagnostic consumer, we construct non-visitor diagnostic pieces.

  • We first construct the final diagnostic piece (the warning), then
  • We start ascending the bug path from the error node's predecessor (since the error node itself was used to construct the warning event). For each node
    • We check the location (whether its a CallEnter, CallExit) etc. We simultaneously keep track of where we are with the execution by pushing CallStack when we see a CallExit (keep in mind that everything is happening in reverse!), popping it when we find a CallEnter, compacting them into a single PathDiagnosticCallEvent.
void f() {
  bar();
}

void g() {
  f();
  error(); // warning
}

=== The bug path ===

(root) -> f's CallEnter -> bar() -> f's CallExit -> (error node)


=== Constructed report ===

  f's CallEnter -> bar() -> f's CallExit
           ^               /
            \             V
(root) --->  f's CallEvent --> (error node)
    • We also keep track of different PathPieces different location contexts (CallEvent::path in the above example has f's LocationContext, while the CallEvent itself is in g's context) in a LocationContextMap object.
    • Construct whatever piece, if any, is needed for the note.
    • If we need to generate edges (or arrows) do so. Make sure to also connect these pieces with the ones that visitors emitted.
  • Clean up the constructed PathDiagnostic by making arrows nicer, pruning function calls, etc.

So I complained about mile long function invocations with seemingly the same parameters being passed around. This problem, as I see it, a natural candidate for creating classes and tying them all together.

I tried very hard to make the implementation feel natural, like, rolling off the tongue. I introduced 2 new classes: PathDiagnosticBuilder (I mean, I kept the name but changed almost everything in it) contains every contextual information (owns the bug path, the diagnostics constructed but the visitors, the BugReport itself, etc) needed for constructing a PathDiagnostic object, and is pretty much completely immutable. BugReportContruct is the object containing every non-contextual information (the PathDiagnostic object we're constructing, the current location in the bug path, the location context map and the call stack I meantioned earlier), and is passed around all over the place as a single entity instead of who knows how many parameters.

I tried to used constness, asserts, limiting visibility of fields to my advantage to clean up the code big time and dramatically improve safety. Also, whenever I found the code difficult to understand, I added comments and/or examples.

Here's a complete list of changes and my design philosophy behind it:

  • Instead of construcing a ReportInfo object (added by D65379) after finding a valid bug report, simply return an optional PathDiagnosticBuilder object straight away. Move findValidReport into the class as a static method. I find GRBugReporter::generatePathDiagnostics a joy to look at now.
  • Rename generatePathDiagnosticForConsumer to generate (maybe not needed, but felt that way in the moment) and moved it to PathDiagnosticBuilder. If we don't need to generate diagnostics, bail out straight away, like we always should have. After that, construct a BugReportConstruct object, leaving the rest of the logic untouched.
  • Move all static methods that would use contextual information into PathDiagnosticBuilder, reduce their parameter count drastically by simply passing around a BugReportConstruct object.
  • Glance at the code I removed: Could you tell what the original PathDiagnosticBuilder::LC object was for? It took a gooood long while for me to realize that nothing really. It is always equal with the LocationContext associated with our current position in the bug path. Remove it completely.
  • The original code contains the following expression quite a bit: LCM[&PD.getActivePath()], so what does it mean? I said that we collect the contexts associated with different PathPieces, but why would we ever modify that, shouldn't it be set? Well, theoretically yes, but in the implementation, the address of PathDiagnostic::getActivePath doesn't change if we move to an outer, previously unexplored function. Add both descriptive method names and explanations to BugReportConstruct to help on this.
  • Add plenty of asserts, both for safety and as a poor man's documentation.

Please, if you find this explanation, the size of the patch or anything too vague/too large, let me know and I'll make adjustments. Overall, I feel this is a giant leap towards the right direction.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Jul 30 2019, 3:37 PM
NoQ accepted this revision.Aug 2 2019, 7:12 PM

*bows down to those who bestow documentation upon this code*

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
111 ↗(On Diff #212445)

You mean Construct like a noun? Not sure what that means.

Maybe PathDiagnosticContext?

127 ↗(On Diff #212445)

Dunno, "Diag" near "Vector" kinda reads to me more like "Diagonal".

140 ↗(On Diff #212445)

Curr

Purr? =^.^=

This revision is now accepted and ready to land.Aug 2 2019, 7:12 PM
Szelethus marked an inline comment as done.Aug 4 2019, 3:49 PM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
111 ↗(On Diff #212445)

Yea, I originally wanted to have the class name BaconStrip so that it screams just a bit louder that I couldnt come up with anything good, but maybe PathDiagnosticConstruction or something like that would be better.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 12:02 PM
Szelethus reopened this revision.Aug 13 2019, 12:10 PM

Oh, crap, forgot to address inlines. I'll see how buildbots react to these changes and will follow up in a smaller patch. Reopening as a reminder.

This revision is now accepted and ready to land.Aug 13 2019, 12:10 PM
Szelethus closed this revision.Aug 13 2019, 1:42 PM
Szelethus marked 5 inline comments as done.

The deed is done.

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
111 ↗(On Diff #212445)

Mmmmm this entire class is about non-contextual stuff, I'd rather stick with PathDiagnosticConstruct and just explain it well in the comments.

140 ↗(On Diff #212445)

I've always been a cat person. They take care of themselves, just like a good C++ class should :^)