This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Avoid nullptr dereference
AbandonedPublic

Authored by FreddyYe on Nov 3 2021, 7:38 PM.

Details

Summary

These warning are reported by static code analysis tool: Klocwork

Diff Detail

Event Timeline

FreddyYe created this revision.Nov 3 2021, 7:38 PM
FreddyYe requested review of this revision.Nov 3 2021, 7:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2021, 7:38 PM
skan added inline comments.Nov 3 2021, 10:23 PM
llvm/lib/CodeGen/TypePromotion.cpp
259–260

You mean

Return->getReturnValue() may return a NULL pointer but LessOrEqualTypeSize assumes its argument is never be a NULL?

If the answer is yes, you need to provide a test case tiggering the compfail.

pengfei added inline comments.Nov 3 2021, 10:47 PM
llvm/lib/CodeGen/TypePromotion.cpp
259–260

Agreed with @skan . You changed the functionality here.
Besides, I didn't find difference between StoreInst and ReturnInst, why only warn here?

Besides, I didn't find difference between StoreInst and ReturnInst, why only warn here?

The difference is Store->getValueOperand() doesn't return a nullptr but Return->getValueOperand() does.

Hi both, thx for review. This is reported by static code analysis tool, so I don't have a reproduce. I can also add assert() to these callees to satisfy the tool, but there will be more than only one of them. Does that sound OK to you?

FreddyYe updated this revision to Diff 384668.Nov 3 2021, 11:00 PM

changed into assert()

skan added a comment.EditedNov 3 2021, 11:09 PM

Hi both, thx for review. This is reported by static code analysis tool, so I don't have a reproduce. I can also add assert() to these callees to satisfy the tool, but there will be more than only one of them. Does that sound OK to you?

Why not add the assert in this if clause if you make sure Return->getReturnValue() can never be NULL here.

skan added inline comments.Nov 3 2021, 11:19 PM
llvm/lib/CodeGen/TypePromotion.cpp
207

I don't know if this assert is necessary...
https://llvm.org/docs/CodingStandards.html#assert-liberally
The code standard suggests using the “assert” macro to its fullest. However I think the NULL ptr check can be omitted b/c we can get error message like "null pointer deference" at run time even w/o the assert.

FreddyYe abandoned this revision.Nov 14 2021, 6:27 PM

After reading the whole logic, I'll judge this as not a defect. Since TypePromotion::isSupportedValue() has checked before codes here. ReturnInst's operand won't be null. So getReturnValue() here won't return Null. I'll close this PR, sorry for the noise.