This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer][StreamChecker] Report every leak, clean up state.
ClosedPublic

Authored by balazske on Jun 30 2020, 12:24 AM.

Details

Summary

Report resource leaks with non-fatal error.
Report every resource leak.
Stream state is cleaned up at checkDeadSymbols.

Diff Detail

Event Timeline

balazske created this revision.Jun 30 2020, 12:24 AM

Why does this not work?
I get only warning for the first resource leak (in the test f_leak_2). How to fix this?

Why does this not work?
I get only warning for the first resource leak (in the test f_leak_2). How to fix this?

First off, are two errors detected by the checker? You could check this with debug.ViewExplodedGraph + clang/tools/analyzer/exploded-graph-rewriter.py.

I do suspect though that this is a uniqueing issue. Just out of curiosity, what would happen is you dump the leaked stream symbol into the warning message? (or whatever, just so that two distinct warning messages are emitted)

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
969

It is a realistic worry that either the predecessor exploded node or the CFGBlock is null? Could we assert this instead?

971

Ah, I see, we made it fatal accidentally.

I checked it with simple debug print, the LeakedSyms will contain 2 items with different StreamOpenNode. So I think these should be independent problem reports for the bug reporter.

balazske marked an inline comment as done.Jun 30 2020, 9:02 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
969

It may be simply more safe to check for it. I do not know if exactly if these can be null at this place.

I checked it with simple debug print, the LeakedSyms will contain 2 items with different StreamOpenNode. So I think these should be independent problem reports for the bug reporter.

It would still be beneficial to check the exploded graph to see whether it was constructed correctly, find the error nodes, etc. Have you tried modifying the warning message? That is considered during uniquing.

I checked it with simple debug print, the LeakedSyms will contain 2 items with different StreamOpenNode. So I think these should be independent problem reports for the bug reporter.

I wonder whether you can report two bugs on the same error node.

I checked it with simple debug print, the LeakedSyms will contain 2 items with different StreamOpenNode. So I think these should be independent problem reports for the bug reporter.

I wonder whether you can report two bugs on the same error node.

I recall that i did something similar in UninitializedObjectChecker when the option NotesAsWarnings is true.

The problem is that PathDiagnostic::Profile does not use the uniqueing locations. Two PathDiagnostic objects with different uniqueing location but otherwise same data are counted as "same" and only one is kept (BugReporter::FlushReport). How to fix this?

  • Add uniqueing locations to the profile. May have unexpected effects to other checkers.
  • Change something in the leak bug reports to make these different. Probably the location can be set to the allocation site (but the full path should be reported). Or include name of the leaked symbol in the report, but it may not exists always.
NoQ added inline comments.Jul 1 2020, 2:41 PM
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
969

Please use SuppressOnSink instead.

balazske marked an inline comment as done.Jul 2 2020, 12:10 AM
balazske added inline comments.
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
969

That was my plan for the next patch. So I want to leave this as is for now (add a FIXME), it should be removed anyway.

balazske updated this revision to Diff 275311.Jul 3 2020, 12:38 AM

Rebase, added a FIXME

Szelethus accepted this revision.Jul 10 2020, 4:17 AM

I'd prefer if you moved f_leak_2 to stream-notes.c. Otherwise, LGTM.

clang/test/Analysis/stream.c
147–150

I'd prefer to see notes from D81407 here.

This revision is now accepted and ready to land.Jul 10 2020, 4:17 AM
balazske updated this revision to Diff 278755.Jul 17 2020, 6:34 AM

Checking notes in test f_leak_2.

balazske updated this revision to Diff 279131.Jul 19 2020, 11:50 PM

Fixed formatting.

balazske updated this revision to Diff 279133.Jul 20 2020, 12:03 AM

Rebase to master.

This revision was automatically updated to reflect the committed changes.