This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Add more information to the Exploded Graph
ClosedPublic

Authored by isuckatcs on Aug 4 2022, 11:08 AM.

Details

Summary

During non-POD array construction and destruction we store additional data in the program state.

Right now only the index of the actual element being constructed is dumped into the egraph in case
of simple construction invoked by CXXConstructExpr.

This patch dumps information about pending ArrayInitLoopExprs and destructors.

Depends on: D130737

The resulting nodes:

Diff Detail

Event Timeline

isuckatcs created this revision.Aug 4 2022, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 11:08 AM
isuckatcs requested review of this revision.Aug 4 2022, 11:08 AM
isuckatcs updated this revision to Diff 450073.

Updated

NoQ added a comment.Aug 4 2022, 11:23 AM

Beautiful!!

I think we should add an exploded-graph-rewriter test in which these new traits aren't null.

At this point, with all these Environment-like traits, maybe we can combine them into a single table? (Would be a separate patch).

----
Environment:

  #0 Call S::S (test.cpp:16:19)
    Expressions:
      S1002  .x  &Element{arr,0 S64b,struct S}.x
      S1008  .x  1 S32b

   #1 Call main
      Expressions:
        S967   arr     &arr
        S9088  arr[*]  &Element{arr,0S64b,struct S}
      Objects Under Construction:
        S1058  (construct into local variable)  auto = {arr[*]}  &Element{NonParamVarRegion{D952},0 S64b,struct S}
      Indices Of Elements Under Construction:
        S1018  (Cur: 0)  CXXConstructExpr <test.cpp:16:19, col:10>  'S'  Next: 1
      Pending Array Init Loop Expressions:
        S1018            CXXConstructExpr <test.cpp:16:19, col:10>  'S'  Size: 2
----
clang/test/Analysis/exploded-graph-rewriter/checker_messages_diff.dot
72–76

It may also make sense to drop the : null traits entirely and have the python script treat it as the default value, this may win like 10% performance at this point. Not necessarily worth it given that the primary bottleneck is still the viewer.

xazax.hun accepted this revision.Aug 8 2022, 9:46 AM

Very minor nit: In the construction we have "Size" in the destruction we have "Size of the array". Is it the case that the first size is the flattened size the second in is the currently destroyed array's size only? (I.e., not modified by nesting.) If that is the case, I think the name of the field could be clearer for the ArrayInitLoopExprs, like "Flattened size".

This revision is now accepted and ready to land.Aug 8 2022, 9:46 AM
isuckatcs updated this revision to Diff 455611.Aug 25 2022, 9:12 AM
isuckatcs edited the summary of this revision. (Show Details)
  • Rebased
  • Updated how the information is displayed.
  • Updated the screenshots of the nodes in the description to reflect the changes.
isuckatcs updated this revision to Diff 456117.Aug 27 2022, 7:27 AM
isuckatcs marked an inline comment as done.

Empty traits are no longer dumped. This also solves the reverse compatibility issue caused by D127973.

xazax.hun added inline comments.Aug 29 2022, 9:02 AM
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
776

Nit: is there any reason to have a string literal here instead of a character literal?

781

Same as above.

808

Same as above, and for a few other locations below.

isuckatcs added inline comments.Aug 29 2022, 10:05 AM
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
776

I though the code will look more uniform like this. Are there cons of using a string literal instead of a character literal here?

xazax.hun added inline comments.Aug 29 2022, 10:54 AM
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
776

Piping a character is more efficient. A string literal decays to a pointer that needs to be dereferenced and we also need to store the closing null character. Passing a literal will save us storing the closing null and the dereference. Although we are on a path where we do IO, so micro-optimizations like this make little difference (if anything at all). So, I'm also fine with the uniformity argument.

isuckatcs updated this revision to Diff 456476.Aug 29 2022, 3:31 PM
isuckatcs marked 4 inline comments as done.

Replaced one length string literals with char literals

isuckatcs updated this revision to Diff 456948.Aug 31 2022, 6:51 AM

Added a new test case where the program states are not empty in the dot files

NoQ accepted this revision.Sep 1 2022, 9:04 PM

Thanks a lot!!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 3:21 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript