This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Add control flow arrows to the analyzer's HTML reports
ClosedPublic

Authored by vsavchenko on Dec 4 2020, 12:36 AM.

Details

Summary

This commit adds a very first version of this feature.
It is off by default and has to be turned on by checking the
corresponding box. For this reason, HTML reports still keep
control notes (aka grey bubbles).

Further on, we plan on attaching arrows to events and having all arrows
not related to a currently selected event barely visible. This will
help with reports where control flow goes back and forth (eg in loops).
Right now, it can get pretty crammed with all the arrows.

Diff Detail

Event Timeline

vsavchenko created this revision.Dec 4 2020, 12:36 AM
vsavchenko requested review of this revision.Dec 4 2020, 12:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2020, 12:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Here is how it looks for that test file:

It is really a good idea!

The operations that would not leave an event in the report are now clearly printed.

But there are three arrows that confuse me in the example report: the assignment x = 0 (x -> 0 -> x), the function call dereference(x) (x -> dereference), and the return statement return *x (int -> *x). I know the arrow is based on the evaluation order of the engine. But from the view of a user, I think these arrows are confusing to some extent.

For the first two, I think it would be better to point just the statement (maybe a CFGElement) without inner arrows (x -> 0 -> x and x -> dereference), or point to the location of the operator itself rather than the BeginLoc (e.g. x -> 0 -> =). For the third one, an arrow from the function name to the first CFGElement looks good to me. And an arrow from the returned expr to the return type or to a special mark (e.g. ⬅️) can also be added, together with function calls (an arrow from the callstmt to a special mark, e.g. ➡️).

By the way, what do you think about adding arrows for data flows of specific symbolic values in the future?

It is really a good idea!

Thanks 😊

The operations that would not leave an event in the report are now clearly printed.

But there are three arrows that confuse me in the example report: the assignment x = 0 (x -> 0 -> x), the function call dereference(x) (x -> dereference), and the return statement return *x (int -> *x). I know the arrow is based on the evaluation order of the engine. But from the view of a user, I think these arrows are confusing to some extent.

For the first two, I think it would be better to point just the statement (maybe a CFGElement) without inner arrows (x -> 0 -> x and x -> dereference), or point to the location of the operator itself rather than the BeginLoc (e.g. x -> 0 -> =). For the third one, an arrow from the function name to the first CFGElement looks good to me. And an arrow from the returned expr to the return type or to a special mark (e.g. ⬅️) can also be added, together with function calls (an arrow from the callstmt to a special mark, e.g. ➡️).

Sorry, for the confusion. I did not come up with the idea of arrows and neither did I chose which tokens are connected by arrows. It is an existing feature, and it existed for quite a long time. The analyzer can produce plist files that contain all these locations, plist files are further used by Xcode to draw arrows. You can see a very old example on our website: https://clang-analyzer.llvm.org
Essentially I took this existing information and used it in the HTML report generation as well. The biggest chunk of this commit is the algorithm for drawing SVG curves.

By the way, what do you think about adding arrows for data flows of specific symbolic values in the future?

I'm open for many ideas in the ways how we can improve our HTML reports! I thought of a popup when you hover over an arrow showing values/constraints for symbols actively involved in the report.

vsavchenko updated this revision to Diff 309497.Dec 4 2020, 2:46 AM

Fix incorrect comment

vsavchenko updated this revision to Diff 310481.Dec 9 2020, 3:16 AM

Replace let with const

@vsavchenko

I checked it in IE. It doesn't draw arrows. Investigate this, please.

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
143

Are you sure that non-arrow piece always has non-empty string representation? Can this give us a false positive result?

474–475

Seems like this shortcut works only with the capital S aka shift+s
Should we support any s state?

@vsavchenko

I checked it in IE. It doesn't draw arrows. Investigate this, please.

Oh, I checked it (using a bunch of weird websites to host html and then to see it in IE). It doesn't show it and I have no idea why. I don't have easy access to IE and I couldn't fix it unfortunately :-(
If you know any good way I can run it on Mac, I'd be happy to test it.

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
143

I haven't seen empty grey bubbles. If there are pieces like this, it'd be a bug in event generation.

474–475

As far as I understand, that matches the original intention because the help message mentions exactly "Shift+S"

ASDenysPetrov added a comment.EditedApr 26 2021, 8:56 AM

@vsavchenko
I made some tests and fixed. Please, consider.

clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
474–475

I've checked again.
When CapsLock is ON, shift+s has no effect, but single s has. The same vice versa.
The real issue is that we check for capital s instead of shift+s.
This change will handle it correctly. I've checked it on IE and Chrome. It works for me.

1292–1295

This is the root cause of why I wasn't able to see arrows in IE11.
I found and checked a fix. It works on IE and Chrome. Thanks to https://github.com/imgix/drift/issues/33#issue-165784939

@vsavchenko
I make some tests and fixes. Please, consider.

OMG, that's so awesome! Thank you so much!

@vsavchenko
I make some tests and fixes. Please, consider.

OMG, that's so awesome! Thank you so much!

Never mind :-) Apply the changes and I will approve the patch.

Fix IE issue and rebase

@vsavchenko How about this?

Yep, thanks for reminding!

ASDenysPetrov accepted this revision.May 28 2021, 3:49 AM

OK, then. Let's land it!

This revision is now accepted and ready to land.May 28 2021, 3:49 AM

OK, then. Let's land it!

Can you please take a look at D92928 as well?

This revision was landed with ongoing or failed builds.Aug 2 2021, 9:31 AM
This revision was automatically updated to reflect the committed changes.