This is an archive of the discontinued LLVM Phabricator instance.

[llvm][unittests] Fix protential nullptr dereferences due to unchecked return value or EXPECT_* macro
Needs RevisionPublic

Authored by OikawaKirie on Nov 23 2020, 5:04 AM.

Details

Summary

These problems are reported by my static analyzer on smart pointers.

In GTest, the ASSERT_* macro will stop the execution of the current test case or function, while EXPECT_* will not. Besides, If the ASSERT_* macro is used in a sub-function of a test case, it will only stop the execution of the current function and return to the caller test function. Although a test failure will be reported, the test program will still crash, which will skip some unit tests in the current test program. (See https://github.com/google/googletest/blob/master/googletest/docs/primer.md#basic-assertions)
In addition, in some test cases, the return value of a parser function is checked in a wrapper function, but both branches are allowed to reach the dereference site.

In this patch, I fixed some of the nullptr dereference with the problems above. The fix will not dismiss any check failures but can make the test program skip the current test case and continue with the next, rather than crash immediately.

Diff Detail

Event Timeline

OikawaKirie created this revision.Nov 23 2020, 5:04 AM
OikawaKirie requested review of this revision.Nov 23 2020, 5:04 AM

Some of my fixes still need to be improved (see the comments in the code). If any reviewers have better ideas, I will update the patch.

Thanks.

llvm/unittests/Analysis/VectorFunctionABITest.cpp
76

Please note that. In function invokeParser, it will still crash after the call here. Since I do not know how to arrange a proper return value, I made no changes here. If any reviewers have any ideas about the problem, I will fix the problem here.

llvm/unittests/Transforms/IPO/AttributorTestBase.h
44

The same problem also appears here. It would be better if this function returns a pointer rather than a reference. But a lot of places related to it need to be changed. Therefore, I just leave it as it is.

vsk added a comment.Nov 30 2020, 11:44 AM

Hi @OikawaKirie, thanks for the patch! Would you mind splitting up this patch? This should make each individual patch easier to review, and make it possible for changes to land incrementally (& faster). Generally we ask for patches to be split up into self-contained units: it's not an exact science, but splitting things up by directory might be a reasonable heuristic to start with.

aeubanks resigned from this revision.Dec 2 2020, 6:34 PM
fhahn requested changes to this revision.Jan 20 2021, 7:40 AM

Marking as Request changes as there's an open suggestion by @vsk

This revision now requires changes to proceed.Jan 20 2021, 7:40 AM
lebedev.ri resigned from this revision.Jan 12 2023, 4:51 PM

This review seems to be stuck/dead, consider abandoning if no longer relevant.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 4:51 PM