Page MenuHomePhabricator

[Analyzer] Only add container note tags to the operations of the affected container
ClosedPublic

Authored by baloghadamsoftware on Mar 3 2020, 4:27 AM.

Details

Summary

If an error happens which is related to a container the Container Modeling checker adds note tags to all the container operations along the bug path. This may be disturbing if there are other containers beside the one which is affected by the bug. This patch restricts the note tags to only the affected container and adjust the debug checkers to be able to test this change.

Diff Detail

Event Timeline

Charusso added a comment.EditedMar 3 2020, 4:43 AM

I believe our path and context sensitive engine is more extensible and precise than checking the source file. Are you sure it scales? I would prefer to tie this information for MemRegions, rather than arbitrary places in the source code. My knowledge is very weak in this checker but I have changed from the Tidy world to the Analyzer to enjoy its benefits. Please enjoy these benefits in your work as well.

Alternative approach for debugging (instead of checking the source range): clang_analyzer_express() from ExprInspection marks its argument as interesting in the bug report. DebugContainerModeling propagates the interestingness from the symbol (begin or end symbol of the container) to the container itself.

Charusso accepted this revision.EditedMar 3 2020, 8:46 AM

Cool, that one a lucky one! I think the SymbolRef based world also working, just at some point it could not scale because other systems are region based... For now, it is a much better solution, and this pattern to overload the callback with all the interestingness seems like the standard way of using NoteTags. Thanks!

This revision is now accepted and ready to land.Mar 3 2020, 8:46 AM

I don't have any technical comments on this patch since I haven't used NoteTags yet, only a couple of readability ones.

clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
97–103

Probably a flattened version would be more readable.

auto *PSBR = dyn_cast<PathSensitiveBugReport>(&BR);
if (PSBR && PSBR->isInteresting(Field))
  PSBR->markInteresting(Cont);
return "";
clang/test/Analysis/container-modeling.cpp
218

I'm not sure about the convention about using dashes, but I've seen multiple time comments like this:
no-warning etc. Probably a simple no-note or something would be more conventional?

232–234

This line looks quite crowded, can't we use expected-note@-1 here just like it was used previously?
The only once comment could be in a separate line as well, just to fit nicely.

Updated according to the style comments.

baloghadamsoftware marked 3 inline comments as done.Mar 3 2020, 9:24 AM

Alternative approach for debugging (instead of checking the source range): clang_analyzer_express() from ExprInspection marks its argument as interesting in the bug report. DebugContainerModeling propagates the interestingness from the symbol (begin or end symbol of the container) to the container itself.

But why is this related to UndefinedVal?

baloghadamsoftware retitled this revision from [Analyzer] Only add container note tags to the operations of te affected container to [Analyzer] Only add container note tags to the operations of the affected container.Mar 5 2020, 5:03 AM

But why is this related to UndefinedVal?

Because UndefinedVal seems to be the "null" value of SVal thus it is suitable for default value of the parameter.

But why is this related to UndefinedVal?

Because UndefinedVal seems to be the "null" value of SVal thus it is suitable for default value of the parameter.

Shouldn't we use Optional then? None means something completely different then UndefinedVal.

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
55–61

I would also prefer to comment what happens when ExprVal has a special value, because its non-obvious from the declaration.

NoQ added a comment.EditedMar 5 2020, 6:45 AM

But why is this related to UndefinedVal?

Because UndefinedVal seems to be the "null" value of SVal

It is not. It is a very specific and exotic SVal that almost never appears in practice but plays a very important role in the analysis.
Let's remove the default constructor for SVal entirely because it seems to be a common source of confusion.

NoQ added inline comments.Mar 5 2020, 7:12 AM
clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
724–725

Mmm, this is definitely impossible. We should change the NoteTag API to pass in a PathSensitiveBugReport &. My bad.

Updated according to the comments.

Updated according to the comments.

baloghadamsoftware marked an inline comment as done.

Typo fixed.

baloghadamsoftware marked 2 inline comments as done.Mar 5 2020, 9:01 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
724–725

Good idea. For now I changed it to simple cast<>() without check.

baloghadamsoftware marked an inline comment as done.

Rebased.

The code from in ExprInspectionChecker.cpp is duplicated from D75677, isn't it?

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
55–57

Please terminate this sentence.

clang/test/Analysis/container-modeling.cpp
99–100

Why did we unpack this? And the rest?

Updated according to the comments.

baloghadamsoftware marked 3 inline comments as done.Mar 12 2020, 4:50 AM

The code from in ExprInspectionChecker.cpp is duplicated from D75677, isn't it?

It is. The one accepted earlier will be committed earlier, then I will rebase the other one so this part disappears from that one.

clang/test/Analysis/container-modeling.cpp
99–100

We did not unpack this but forgot to pack it.

baloghadamsoftware marked an inline comment as done.

Updated according to the comments of D75677.

NoQ accepted this revision.Mar 15 2020, 7:31 PM

Wonderful, thank you!

clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
726

I suspect you can remove the explicit conversion to std::string given that you've specified the return type explicitly.

clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
32

llvm::function_ref?

Szelethus accepted this revision.Mar 16 2020, 3:23 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
32

Is that a thing? Nice.

baloghadamsoftware marked an inline comment as done.Mar 16 2020, 12:56 PM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp
32

You mean a callback instead of a template?

baloghadamsoftware marked an inline comment as done.Mar 17 2020, 2:27 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp
726

Theoretically yes, because llvm::StringRef has a conversion operator for std::string. However, when compiling I get a compilation error:

error: could not convert `Out.llvm::raw_svector_ostream::str()` from `llvm::StringRef` to `std::__cxx11::string {aka std::__cxx11::basic_string<char>}`
This revision was automatically updated to reflect the committed changes.