This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][UninitializedObjectChecker] Uninit regions are only reported once
ClosedPublic

Authored by Szelethus on Aug 31 2018, 2:13 AM.

Details

Diff Detail

Repository
rC Clang

Event Timeline

Szelethus created this revision.Aug 31 2018, 2:13 AM
Szelethus edited the summary of this revision. (Show Details)Aug 31 2018, 2:27 AM
Szelethus updated this revision to Diff 163542.Aug 31 2018, 8:54 AM
Szelethus edited the summary of this revision. (Show Details)Aug 31 2018, 12:40 PM

Polite ping :)

NoQ added a comment.Sep 13 2018, 6:59 PM

Hmm, so we're reporting the same region twice in the same bug report through different field chains, right? Why is a state trait necessary here? Maybe just de-duplicate them locally before throwing the report?

NoQ accepted this revision.Sep 13 2018, 7:02 PM

Hmm, i guess it's necessary to de-duplicate across multiple reports as well.

This revision is now accepted and ready to land.Sep 13 2018, 7:02 PM
NoQ requested changes to this revision.Sep 13 2018, 7:03 PM

Or, wait, hmm, no, we can still de-duplicate the reports with a global set stored in the checker object. Because we would probably like to de-duplicate across different paths as well.

This revision now requires changes to proceed.Sep 13 2018, 7:03 PM
In D51531#1234280, @NoQ wrote:

we can still de-duplicate the reports with a global set stored in the checker object.

I tried it, but it crashes randomly. I added a mutable std::set<const MemRegion *> to the checker class, used the same logic, but seems to me that you can't just erase elements like this. Is there any other checker doing the same thing?

Another possibility could be to gather CXXConstructorDecl, and emit one warning per ctor, but it would be waaay to drastic. Wouldn't a global set be too?

Ping, @NoQ, can you go a little bit more in depth about what you'd prefer to see here?

NoQ added a comment.Oct 21 2018, 3:18 PM

Ugh, i was sure i scanned through all of them, sorry! Pls let me recall what's going on><

As I understood it, your objection was that we could collect already reported regions in a global set stored in the checker objects, but I suspect it's impossible to implement (very difficult to see when a pointer is dangling etc).

NoQ added a comment.EditedOct 21 2018, 3:37 PM

MemRegions have lifetime of ExprEngine, i.e. a single analysis of top-level function. You'll need to clean them up in checkEndAnalysis().

NoQ added a comment.Oct 22 2018, 11:56 AM

I think RetainCountChecker is the only checker that maintains such maps and does such cleanup: https://github.com/llvm-mirror/clang/blob/efe41bf98/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h#L288

I didn't know it does that until today.

Szelethus added a comment.EditedNov 2 2018, 3:21 PM

Sorry for the late reply.

I'm not actually super confident about this idea, I don't think it would add much value, compared to how complicated it would make things. This patch reduces report by a significant amount when pointer chasing is enabled, but a global set might not make things that much cleaner -- if the same report would be made on different execution paths, it would be uniqued with the help of the uniqueing location. If it it wouldn't be, I guess it shouldn't be.

Of course, I'm always open to a discussion!

Oh, and the reason why I think it would add a lot of complication: since only ExprEngine is avaible in the checkEndAnalysis callback, which, from what I can see, doesn't have a handly isDead method, so I'm not even sure how I'd implement it.

Ping, @NoQ, do you insist on a global set?

NoQ accepted this revision.Nov 12 2018, 3:24 PM

Mm, ok, i admit that i don't know what specific de-duplication do we want to have and what are the usability implications of it.

If we want de-duplication for the same region reported in different spots of the same path, we need a state trait.

If we want de-duplication for the same region globally across the whole analysis, we need a checker-wide set of regions, like what you're doing.

If we want de-duplication for the same region globally across the whole translation unit, we need a checker-wide set of AST nodes (eg., field chains) that describe regions in terms that outlive ExprEngine.

If we want de-duplication for different regions within the same constructor call site, we need a uniqueing location at the call site, like what you're doing.

If we want de-duplication for different regions within the same constructor itself, we don't need a uniqueing location.

I guess you should just do whatever you want here?

Oh, and the reason why I think it would add a lot of complication: since only ExprEngine is avaible in the checkEndAnalysis callback, which, from what I can see, doesn't have a handly isDead method, so I'm not even sure how I'd implement it.

Mm, what do you need isDead for? Everything is dead at the end of the analysis, you need to clean up everything, otherwise you get use-after-free(?)

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
169

Hmm, does LocUsedForUniqueing remain uninitialized sometimes? I guess it's on the top frame. What does it do in this case? Why do we need a uniqueing location in other cases? I guess it only *increases* the amount of reports, right? I.e., constructors called from different call sites produce different reports(?)

This revision is now accepted and ready to land.Nov 12 2018, 3:24 PM
In D51531#1296256, @NoQ wrote:

Oh, and the reason why I think it would add a lot of complication: since only ExprEngine is avaible in the checkEndAnalysis callback, which, from what I can see, doesn't have a handly isDead method, so I'm not even sure how I'd implement it.

Mm, what do you need isDead for? Everything is dead at the end of the analysis, you need to clean up everything, otherwise you get use-after-free(?)

Oh, right. Silly me :)

This revision was automatically updated to reflect the committed changes.