Page MenuHomePhabricator

[analyzer] Highlight container object destruction in MallocChecker

Authored by rnkovacs on Jun 23 2018, 11:56 AM.

Diff Detail


Event Timeline

rnkovacs created this revision.Jun 23 2018, 11:56 AM
xazax.hun added inline comments.Jun 23 2018, 12:05 PM
483 ↗(On Diff #152600)

To be honest, I am not sure what was the purpose of the AST based checks in the original version. IMHO, looking at the states only should be sufficient without examining the AST.

2914 ↗(On Diff #152600)

I think these messages should be full sentences starting with a capital letter and ending with a period.

2988 ↗(On Diff #152600)

This new code should only be used for AF_InternalBuffer allocation family? If that is the case, maybe adding an assert here and running it on some large codebases to se if this is triggered would be great.

2989 ↗(On Diff #152600)

LLVM's optional can implicitly convert to bool. I think it is perfectly fine to rely on the implicit conversion here.

NoQ accepted this revision.Jun 23 2018, 4:18 PM

Only minor nits, as usual :)

483 ↗(On Diff #152600)

Yeah, same. I guess we could replace the check with an assertion (i.e. if there's statement, it's a call or delete-expression and it's AF_InternalBuffer), and see if anything crashes.

2861 ↗(On Diff #152600)

I don't think our check will be limited to STL containers only. We might cover other containers as well, such as LLVM or WebKit custom strings and string views.

2914 ↗(On Diff #152600)

Or exclamantion mark! :)

2991 ↗(On Diff #152600)


This revision is now accepted and ready to land.Jun 23 2018, 4:18 PM
rnkovacs updated this revision to Diff 152615.Jun 24 2018, 9:53 AM
rnkovacs marked 7 inline comments as done.
rnkovacs retitled this revision from [analyzer] Highlight STL object destruction in MallocChecker to [analyzer] Highlight container object destruction in MallocChecker.

Thanks for the comments!
I'll run this on some projects and see if any assertions fail.

xazax.hun accepted this revision.Jun 24 2018, 9:56 AM

LGTM, given that the assert does not fire for the projects you tested on.

No crashes on Harfbuzz, ICU, Bitcoin, and LLVM. I'll commit.

This revision was automatically updated to reflect the committed changes.