Page MenuHomePhabricator

[analyzer] Don't track function calls as control dependencies

Authored by Szelethus on Jan 4 2022, 5:53 AM.



I recently evaluated ~150 of bug reports on open source projects relating to my GSoC'19 project, which was about tracking control dependencies that were relevant to a bug report.

Here is what I found: when the condition is a function call, the extra notes were almost always unimportant, and often times intrusive:

void f(int *x) {
  x = nullptr;
  if (alwaysTrue()) // We don't need a whole lot of explanation
                    // here, the function name is good enough.
    *x = 5;

It almost always boiled down to a few "Returning null pointer, which participates in a condition later", or similar notes. I struggled to find a single case where the notes revealed anything interesting or some previously hidden correlation, which is kind of the point of condition tracking.

This patch checks whether the condition is a function call, and if so, bails out.

The argument against the patch is the popular feedback we hear from some of our users, namely that they can never have too much information. I was specifically fishing for examples that display best that my contribution did more good than harm, so admittedly I set the bar high, and one can argue that there can be non-trivial trickery inside functions, and function names may not be that descriptive.

My argument for the patch is all those reports that got longer without any notable improvement in the report intelligibility. I think the few exceptional cases where this patch would remove notable information are an acceptable sacrifice in favor of more reports being leaner.

Diff Detail

Event Timeline

Szelethus created this revision.Jan 4 2022, 5:53 AM
Szelethus requested review of this revision.Jan 4 2022, 5:53 AM
NoQ added a comment.Jan 5 2022, 10:14 PM

Interesting. Might it be that in this scenario in order to be of interest to the user the condition value has to be trackable back to the current stack frame?

the popular feedback we hear from some of our users, namely that they can never have too much information

They should try prune-paths=false in C++. Hundreds of inlined copy-constructors will definitely give them the desired experience ;)


Let's make it clear that this decision is purely stochastic: we can totally build an artificial example where this results in bad behavior but we've never seen one in practice.

Szelethus updated this revision to Diff 402549.Jan 24 2022, 8:44 AM

Fix tests, mention that this is purely a heuristic.

Szelethus marked an inline comment as done.Jan 24 2022, 8:44 AM

Looks great!
Thanks for your hard work on this topic @Szelethus.

5908–5909 ↗(On Diff #402549)

How are these dump() changes related?


extra blank line


Please also have a c++17 init if statement.


What if this expression is enclosed by a logical operator such as &&?

Szelethus updated this revision to Diff 419132.Mar 30 2022, 7:03 AM
Szelethus marked an inline comment as done.

Fixes according to reviewer comments.

Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 7:03 AM
Szelethus marked 2 inline comments as done.Mar 30 2022, 7:03 AM
Szelethus added inline comments.
5908–5909 ↗(On Diff #402549)

I'll commit them in a separate patch.


That is intentional -- I think it makes the code more readable. Separates the function signature from the implementation.


For each of those operators, a different CFGBlock would be created:

if (A && B) 

    /   \
 /         \
A---------> D

This means that operands of || and && is retrievable through CFGBlock::getLastCondition(), so I shouldn't need to tear the AST apart to that extent. Though, I admit, you likely don't need to go very far to fool my implementation for realizing whether the condition boils down to a function call.

steakhal accepted this revision.Mar 30 2022, 8:12 AM

I'm convinced!




Some of these hunks are unrelated to the parts you actually changed.
Strictly peaking I don't mind them, just pointing out. would format only the changed lines.


For the record, I never understood why we have two notes about the same thing: we are taking that branch.
Maybe we should keep simplifying these notes in the future as well.


I see, thanks.

This revision is now accepted and ready to land.Mar 30 2022, 8:12 AM
This revision was automatically updated to reflect the committed changes.
Szelethus marked an inline comment as done.