This is an archive of the discontinued LLVM Phabricator instance.

[Sema] raise nullptr check to cover additional uses
Needs RevisionPublic

Authored by nickdesaulniers on May 19 2019, 7:11 PM.

Details

Reviewers
rsmith
Summary

This was flagged in https://www.viva64.com/en/b/0629/ under "Snippet No.
14" (see under #13). It looks like PVS studio flags nullptr checks where
the ptr is used inbetween creation and checking against nullptr.

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2019, 7:11 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Note that this changes when the destructor for CXXThisScopeRAII runs. It's not clear to me why ThisScope is constructed at all.

A similar case exists and was flagged in https://www.viva64.com/en/b/0629/ under "Snippet No. 16" (see under #13), clang/lib/Sema/SemaTemplateInstantiate.cpp Sema::InstantiateClass(). I'll wait for feedback on this patch, then either roll up the additional fix into this or create it as a separate patch (or change both, abandon, etc).

rsmith requested changes to this revision.May 19 2019, 11:23 PM
rsmith added inline comments.
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
557–558

The purpose of creating this is to change the type of this in the scope of the instantiation below, so this change is incorrect. The right fix would be to change the initializer of ThisContext to handle ND being null. (I suspect that actually can't happen at the moment because we happen to only attach late-parsed attributes to NamedDecls, but I don't think there's any fundamental reason why that should be the case so it makes some sense to retain the check here.)

This revision now requires changes to proceed.May 19 2019, 11:23 PM
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
557–558

The purpose of creating this is to change the type of this in the scope of the instantiation below, so this change is incorrect.

In this case I agree. The } added on L559 should be extended to L564.

so it makes some sense to retain the check here

I assume PVS studio is complaining about dereferencing ND on L556, then doing what looks like a nullptr check on L558 (reminiscent of -fno-delete-null-pointer-checks). I assume dyn_cast on L554 can fail at runtime and return nullptr? Should that happen, how should we proceed?