Static analyzer doesn't model copy elision

[llvmbot]

Bugzilla Link	30936
Version	trunk
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@Xazax-hun

Extended Description

Static Analyzer looses track of a field of an object when it doesn't see its copy constructor, even if the copy should be elided according to the standard ([class.copy]p32). In the case below, this part of the paragraph applies: " When certain criteria are met, an implementation is allowed to omit the copy/move construction of a class object, even if the constructor selected for the copy/move operation and/or the destructor for the object have side effects. In such cases, the implementation treats the source and target of the omitted copy/move operation as simply two different ways of referring to the same object, and the destruction of that object occurs at the later of the times when the two objects would have been destroyed without the optimization. This elision of copy/move operations, called copy elision, is permitted in the following circumstances (which may be combined to eliminate multiple copies): ... — when a temporary class object that has not been bound to a reference (12.2) would be copied/moved to a class object with the same cv-unqualified type, the copy/move operation can be omitted by constructing the temporary object directly into the target of the omitted copy/move"

Here's a repro:

$ clang-tidy -checks=-,clang-analyzer,-clang-analyzer-alpha q.cc -- -std=c++11 1 warning generated. q.cc:51:20: warning: Dereference of null pointer (loaded from variable 'p') [clang-analyzer-core.NullDereference] EXPECT_TRUE(1 == p); ^ ...

If you remove the semicolon and the comment after AssertionResult(const AssertionResult& other), the false positive goes away.

Here's the code: =========== q.cc ============= class Message {}; class AssertHelper { public: AssertHelper(const char file, int line, const char message); ~AssertHelper();

void operator=(const Message& message) const; };

define GTEST_MESSAGEAT(file, line, message) \

AssertHelper(file, line, message) = Message()

define GTESTMESSAGE(message) GTEST_MESSAGEAT(FILE, LINE, message)

define GTEST_FATALFAILURE(message) return GTESTMESSAGE(message)

define GTEST_NONFATALFAILURE(message) GTESTMESSAGE(message)

class AssertionResult { public: AssertionResult(const AssertionResult& other); // : success(other.success) {} explicit AssertionResult(bool success) : success(success) {} operator bool() const { return success; }

private: bool success_; };

define GTEST_AMBIGUOUS_ELSEBLOCKER switch (0) case 0: default:

define GTEST_TESTBOOLEAN(expression, text, actual, expected, fail) \

GTEST_AMBIGUOUS_ELSEBLOCKER \ if (const AssertionResult gtestar = AssertionResult(expression)) \ ; \ else \ fail("")

define ASSERT_TRUE(condition) \

GTEST_TESTBOOLEAN(condition, #condition, false, true, GTEST_FATALFAILURE)

define EXPECT_TRUE(condition) \

GTEST_TESTBOOLEAN(condition, #condition, false, true, \ GTEST_NONFATALFAILURE)

extern int *q();

void f() { int p = q(); ASSERT_TRUE(p != nullptr); EXPECT_TRUE(1 == p); }

This issue accounts for a significant number of false positives in our codebase.

[Xazax-hun]

Never mind, the patch is for the false positive for gtest. The underlying issue is still not solved.

[Xazax-hun]

The linked phabricator review is closed. Can this issue be closed as well?

[devincoughlin]

A proposed patch addressing the false positive is up for review at https://reviews.llvm.org/D27773

[devincoughlin]

I looked into this a bit using Alexander's reproducer and the gtest sources on GitHub.

There are two separate confounders to reasoning about AssertionResult.

(1) As Alexander notes, the implementation of the copy constructor is not exposed in the header so the analyzer doesn't model it. Since the class is not trivial the analyzer doesn't treat it as C-struct-like copy.

(2) The boolean constructor is into a temporary and since there are cleanups the analyzer doesn't do its trick to try to construct it directly into the variable storage instead. Since the the constructor is on a temporary and has a destructor, the analyzer skips inlining the constructor -- this is to avoid inconsistency because the analyzer's support for temporary destructors is not yet turned on by default.

One possibility for addressing this is to explicitly model the non-inlined parts of the API in an API-specific checker. I have a hacked up prototype of this that seems to work quite well.

[devincoughlin]

Thanks for the reproducer!

[llvmbot]

assigned to @devincoughlin