This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][MallocChecker] Improve warning messages on double-delete errors
ClosedPublic

Authored by Szelethus on Nov 22 2018, 7:34 AM.

Diff Detail

Event Timeline

Szelethus created this revision.Nov 22 2018, 7:34 AM
NoQ accepted this revision.Nov 30 2018, 5:16 PM

Thanks, nice catch!

It seems that the ReportDoubleDelete() thing was never used for reporting double-delete, but it was used for some strange check when a destructor is called. Is that old code even correct?

This revision is now accepted and ready to land.Nov 30 2018, 5:16 PM
In D54834#1315475, @NoQ wrote:

Thanks, nice catch!

It seems that the ReportDoubleDelete() thing was never used for reporting double-delete, but it was used for some strange check when a destructor is called.

It was used for reporting, there actually is a test case for it in test/Analysis/NewDelete-checker-test.cpp on line 380.

Szelethus marked 2 inline comments as done.Dec 15 2018, 10:41 AM
Szelethus added inline comments.
test/Analysis/NewDelete-checker-test.cpp
380 ↗(On Diff #175054)

Right here.

392 ↗(On Diff #175054)

And here.

This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Dec 16 2018, 4:22 PM

Mm, yeah, but it's still weird to see it in checkPreCall for CXXDestructorCall.

I suspect that your new warning covers these tests as well.

NoQ added a comment.Dec 16 2018, 4:41 PM

Ugh. MSVC buildbot is acting weird again. Also it is not acting deterministically.

I was also yelled at by it in D55388.

Which makes me suspect that it has something to do with checker registration, as i change checker option related behavior in my patch, and the crashes in MallocChecker deal with loading checker name.

NoQ added a comment.Dec 16 2018, 5:32 PM

Wait, no, i didn't do anything with checker options yet. Something else then, i guess.

NoQ added a comment.Dec 16 2018, 10:48 PM

Ok, D55388 seems to have landed (3 builds without failures in move checker), so i guess these are separate problems after all.

The first failing build seems to be http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/2662 - it includes both this commit and rC349281.

Interesting, I've been watching the bots closely, but got no mail after a while. I'm not sure what the cause is, so I'll revert one-by-one.

Szelethus reopened this revision.Dec 17 2018, 4:12 AM

Reverted in rC349340. With the wrong revision name... oops...

This revision is now accepted and ready to land.Dec 17 2018, 4:12 AM
NoQ added a comment.Dec 17 2018, 9:13 AM

Interesting, I've been watching the bots closely, but got no mail after a while. I'm not sure what the cause is, so I'll revert one-by-one.

One of the common reasons for that is that the buildbot was already failing when you committed your stuff. Which is why for many buildbots there are special people who look at them and tell people to fix their stuff manually.

This time it wasn't the case though: build 2661 was green. So dunno. I definitely did receive a mail from this buildbot when it failed on my patch.

But generally it's fine if you don't notice that you're breaking a buildbot or two. It's not something to worry about. Sooner or later, they will come after you :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 3:14 AM
Herald added a subscriber: Charusso. · View Herald Transcript