Static analyzer doesn't model copy elision

[Quuxplusone]

Bugzilla Link	PR30936
Status	NEW
Importance	P normal
Reported by	Alexander Kornienko (alexfh@google.com)
Reported on	2016-11-07 15:45:06 -0800
Last modified on	2017-10-04 06:40:10 -0700
Version	trunk
Hardware	PC All
CC	llvm-bugs@lists.llvm.org, sbenza@google.com, xazax.hun@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

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_MESSAGE_AT_(file, line, message) \
  AssertHelper(file, line, message) = Message()

#define GTEST_MESSAGE_(message) GTEST_MESSAGE_AT_(__FILE__, __LINE__, message)

#define GTEST_FATAL_FAILURE_(message) return GTEST_MESSAGE_(message)
#define GTEST_NONFATAL_FAILURE_(message) GTEST_MESSAGE_(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_ELSE_BLOCKER_ switch (0) case 0: default:
#define GTEST_TEST_BOOLEAN_(expression, text, actual, expected, fail) \
  GTEST_AMBIGUOUS_ELSE_BLOCKER_                                       \
  if (const AssertionResult gtest_ar_ = AssertionResult(expression))  \
    ;                                                                 \
  else                                                                \
    fail("")

#define ASSERT_TRUE(condition) \
  GTEST_TEST_BOOLEAN_(condition, #condition, false, true, GTEST_FATAL_FAILURE_)

#define EXPECT_TRUE(condition)                            \
  GTEST_TEST_BOOLEAN_(condition, #condition, false, true, \
                      GTEST_NONFATAL_FAILURE_)

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.

[Quuxplusone]

Thanks for the reproducer!

[Quuxplusone]

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.

[Quuxplusone]

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

[Quuxplusone]

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

[Quuxplusone]

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