This is an archive of the discontinued LLVM Phabricator instance.

Fix false positive warning about memory leak for QApplication::postEvent
ClosedPublic

Authored by Dushistov on Oct 28 2015, 6:28 PM.

Details

Summary

Recent version of clang (3.7) start complain about such code:
void send(QObject *obj)
{

QKeyEvent *event = new QKeyEvent(QEvent::KeyRelease, Qt::Key_Tab, Qt::NoModifier);
qApp->postEvent(obj, event);

}

warning: Potential leak of memory pointed to by 'event'

This is false positive, because of according to Qt documentation Qt take care about memory allocated for QEvent:
http://doc.qt.io/qt-4.8/qcoreapplication.html#postEvent

Because of Qt popular enought I suggest to handle postEvent case in MallocChecker

Diff Detail

Event Timeline

Dushistov updated this revision to Diff 38705.Oct 28 2015, 6:28 PM
Dushistov retitled this revision from to Fix false positive warning about memory leak for QApplication::postEvent.
Dushistov updated this object.
Dushistov added a subscriber: cfe-commits.
zaks.anna edited edge metadata.Nov 2 2015, 1:13 PM

This needs a test case. You can either extend ./test/Analysis/malloc.cpp or add another QT specific test file.

Thanks!
Anna.

Dushistov updated this revision to Diff 39009.Nov 2 2015, 5:12 PM
Dushistov edited edge metadata.

I added test case and move it to separate file, because of
1)it require additional option -analyzer-checker=cplusplus to trigger warning
2)Simple emulation of qt type system not work (not trigger any warning), so I add preprocessored file.

Test cases need to be small and self contained.
You might be having a problem with reproducing with a simple test case, where you define QCoreApplication::postEvent in the test file because it is not considered to be in the system header. You can use special pragmas for that. See ./test/Analysis/Inputs, where we have a bunch of .h files declaring system APIs. Please add another file for QT there.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2516
Dushistov updated this revision to Diff 39264.Nov 4 2015, 2:51 PM

I reduce testcase to almost minimal variant.

Dushistov updated this revision to Diff 39265.Nov 4 2015, 2:55 PM

Fix line length issue

Dushistov marked an inline comment as done.Nov 11 2015, 1:52 PM
Dushistov updated this revision to Diff 39985.Nov 11 2015, 3:42 PM

mistype, actually I want to use '&&' here, not '||' to not create std::string,
if match failed.

xazax.hun added inline comments.Nov 17 2015, 1:05 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2514

Shouldn't you use == instead of endswith here?

Dushistov updated this revision to Diff 40485.Nov 18 2015, 1:54 AM

Replace 'endswith' with 'operator==', also update test to check that all four ways to call postEvent not produce warning

Dushistov marked an inline comment as done.Nov 18 2015, 1:54 AM

apply all suggestions

zaks.anna added inline comments.Nov 18 2015, 6:47 PM
test/Analysis/qt_malloc.cpp
12

Should the leak be reported when the object is passed to QApplication::postEvent?

In this test, 'event' escapes during one of these calls and the rest of the calls are not tested/make no difference. If you want to test that 'event' escapes when each of them is called, you'd need to test each of them with a different object.

Dushistov updated this revision to Diff 40596.Nov 18 2015, 7:18 PM

Make test for usage of different variants of postEvent more robust.

Dushistov marked an inline comment as done.Nov 18 2015, 7:20 PM

Should the leak be reported when the object is passed to QApplication::postEvent?

No, QApplication::postEvent == QCoreApplication::postEvent, it is just the same function,
because of QApplication inherit from QCoreApplication, and not introduce it's own postEvent.

In this test, 'event' escapes during one of these calls and the rest of the calls are not tested/make no >difference.

fixed

zaks.anna accepted this revision.Nov 18 2015, 8:59 PM
zaks.anna edited edge metadata.

Thanks!
LGTM. I'll commit.

This revision is now accepted and ready to land.Nov 18 2015, 8:59 PM
This revision was automatically updated to reflect the committed changes.