Page MenuHomePhabricator

[analyzer] Allow visitors to run callbacks on completion
AbandonedPublic

Authored by RedDocMD on May 31 2021, 10:57 PM.

Details

Summary

This patch introduces the concept of *Stoppable* visitors, ie, a
visitor that must perform some task when it runs out of nodes on which
it has work to do.
Initially, the ReturnVisitor is being made *stoppable*, thus allowing
it to run callbacks on completion. These callbacks are meant to be
supplied from trackExpressionValue.
Thus, this patch will also allow trackExpressionValue to recieve a
callback to run when it is done tracking.

Diff Detail

Unit TestsFailed

TimeTest
430 msx64 debian > Clang.Analysis::placement-new.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -std=c++11 /var/lib/buildkite-agent/builds/llvm-project/clang/test/Analysis/placement-new.cpp -analyzer-checker=core -analyzer-checker=cplusplus.NewDelete -analyzer-checker=cplusplus.PlacementNew -analyzer-output=text -verify -triple x86_64-unknown-linux-gnu
420 msx64 debian > Clang.Analysis::track-control-dependency-conditions.m
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -w -analyzer-checker=core,nullability -verify /var/lib/buildkite-agent/builds/llvm-project/clang/test/Analysis/track-control-dependency-conditions.m
420 msx64 debian > Clang.Analysis::uninit-vals.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -analyzer-checker=core -fblocks -verify -analyzer-output=text /var/lib/buildkite-agent/builds/llvm-project/clang/test/Analysis/uninit-vals.c
440 msx64 debian > Clang.Analysis/inlining::false-positive-suppression.m
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -analyzer-checker=core -analyzer-config suppress-null-return-paths=false -verify /var/lib/buildkite-agent/builds/llvm-project/clang/test/Analysis/inlining/false-positive-suppression.m
190 msx64 windows > Clang.Analysis::placement-new.cpp
Script: -- : 'RUN: at line 1'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w16-1\llvm-project\premerge-checks\build\lib\clang\13.0.0\include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -std=c++11 C:\ws\w16-1\llvm-project\premerge-checks\clang\test\Analysis\placement-new.cpp -analyzer-checker=core -analyzer-checker=cplusplus.NewDelete -analyzer-checker=cplusplus.PlacementNew -analyzer-output=text -verify -triple x86_64-unknown-linux-gnu
View Full Test Results (8 Failed)

Event Timeline

RedDocMD created this revision.May 31 2021, 10:57 PM
RedDocMD requested review of this revision.May 31 2021, 10:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2021, 10:57 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Note: This patch is only partially done.

The following things are still left:

  • trackExpressionValue must receive a callback
  • The callback must be passed to a visitor (ReturnVisitor for now)
  • Tests to verify that ReturnVisitor actually does what we intend it to do (call the callback at the right place).
vsavchenko added inline comments.Jun 1 2021, 12:44 AM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
895

function_ref is a reference, it doesn't own the function. It means that it shouldn't outlive the actual function object (very similar to StringRef).

xazax.hun added inline comments.Jun 1 2021, 9:02 PM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
95

Are we anticipating visitors that do something else other than calling a callback when they are stopped? If that is not the case, I wonder if storing/invoking the visitor in this class would make more sense.

97

Should we allow derived classes or even others query if a visitor was stopped? Or is this only for catching bugs?

101

I prefer really small functions, like this dtor to be in header files. Feel free to ignore.

104

Do you anticipate cases where the visitor itself does not know that it needs to stop (e.g. in the Visit functions) so someone needs to call this from the outside?

RedDocMD added inline comments.Jun 1 2021, 9:58 PM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
95

I think it was decided in the last meet that we should not make this new abstraction overtly specific. So a stoppable visitor is free to have a callback (like the ReturnVisitor now does) and is also free to have some more behavior in the OnStop method. Also making the OnStop method pure-virtual means that the sub-classes are required to override it (so it can't be ignored).

97

Right now it can't be queried. The purpose of the Stopped field is primarily for catching bugs. (this ensures visitors are stopped exactly once).

104

No, I don't. Are you suggesting I should make it a protected method?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
895

So I guess this means that the callback must be a method/lambda that persists (ie, it can't be a lambda in a VisitNode method, but must be stored a field in the class).

1154

I am not entirely certain this is the right place to call the Stop method. It seems reasonable but I am not sure.

RedDocMD updated this revision to Diff 349295.Jun 2 2021, 9:02 AM

trackExpressionValue recieving callback, passing it to ReturnVisitor

The following things are now done:

  • trackExpressionValue must receive a callback
  • The callback must be passed to a visitor (ReturnVisitor for now)

I still don't know a good way to test the following:

  • Tests to verify that ReturnVisitor actually does what we intend it to do (call the callback at the right place).

@vsavchenko, @NoQ, @xazax.hun, @teemperor can you give some suggestions?

NoQ added a comment.Jun 2 2021, 10:25 PM

Tests to verify that ReturnVisitor actually does what we intend it to do (call the callback at the right place).

You mean write a test that demonstrates that? I guess unless we're willing to wait for the checker to catch up, a good approach to this would be to write a unit test. See unittests/StaticAnalyzer - there aren't many of them because they're relatively hard to write but these days they're getting more and more popular as we're trying to eliminate mutually exclusive bugs invisible on LIT tests. You can mock some code, emit a warning against it from a mocked checker, and test directly that the visitor is stopped at, say, a given node or a given line number.

Or you mean like, add an assert that ensures that you're doing right thing? I think this is also possible to some extent. For example, you can check when the visitor is stopped that no other visitors were added during the current VisitNode() invocation (otherwise the process has to continue). Or you could assert the opposite: if the visitor has run to completion and no other visitors were added then the callback has to be invoked. I think this can get messy really quickly (esp. knowing that you can't simply count all visitors to see if more were added, given how newly added visitors may be duplicates of existing visitors). But I suspect that it could be easy to check this in at least one direction.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
109

I think we have to use std::function here.

/// An efficient, type-erasing, non-owning reference to a callable. This is
/// intended for use as the type of a function parameter that is not used
/// after the function in question returns.
///
/// This class does not own the callable, so it is not in general safe to store
/// a function_ref.
template<typename Fn> class function_ref;

An llvm::function_ref becomes a dangling reference as soon as trackExpressionValue() returns. This is too early.

balazske added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
95

According to the conventions, should use stop and onStop instead of Stop, OnStop?

I was thinking a lot about this problem after our last call, and even though StoppableVisitor helps to solve the problem that we have, it is extremely unclear when this callback is called and should be called.
I decided to restore the balance (and rid of all younglings, maybe) and put together this interface: D103605. I still need to migrate the existing code into the new architecture, but it can be done incrementally.

I was thinking a lot about this problem after our last call, and even though StoppableVisitor helps to solve the problem that we have, it is extremely unclear when this callback is called and should be called.
I decided to restore the balance (and rid of all younglings, maybe) and put together this interface: D103605. I still need to migrate the existing code into the new architecture, but it can be done incrementally.

Right. I then perhaps should focus now on D98726, and then return to GetNoteEmitter.

RedDocMD abandoned this revision.Jun 18 2021, 11:51 AM

I am closing this since it has been addressed much better by the patches from @vsavchenko.