This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] pr37578: Fix lvalue/rvalue problem in field-of-temporary adjustments.
ClosedPublic

Authored by NoQ on Aug 16 2018, 11:19 AM.

Details

Summary

Despite the effort to eliminate the need for skipRValueSubobjectAdjustments(), we still encounter ASTs that require it from time to time, for example in https://bugs.llvm.org/show_bug.cgi?id=37578

One way of properly modeling such expressions would be to include the member-expression "adjustment" into the construction context of the temporary, but that's not something i'm planning to do, because such ASTs are rare and seem to be only becoming more rare over time, so for now i'm just fixing the old code.

The root cause of the problem in this example is that while evaluating the MemberExpr in

`-MemberExpr 0x7fd5ef0035b8 <col:9, col:13> 'a::(anonymous struct at test.cpp:2:3)' . 0x7fd5ee06a068
  `-CXXTemporaryObjectExpr 0x7fd5ef001f70 <col:9, col:11> 'a' 'void () noexcept' zeroing

there's no way for createTemporaryRegionIfNeeded() to communicate the newly created temporary region through the Environment (as it usually does), because all expressions so far have been prvalues.

The current code works around that problem by binding the region to the CXXTemporaryObjectExpr, which is of course a bad thing to do because we should not bind Locs to prvalue expressions, and it leads to a crash when eventually this bad binding propagates to the Store and the Store is unable to load it.

The solution is to bind the correct [lazy compound] value to the CXXTemporaryObjectExpr and then communicate the region to the caller directly via an out-parameter.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Aug 16 2018, 11:19 AM
NoQ updated this revision to Diff 161073.Aug 16 2018, 11:21 AM

Add comments for optional arguments.

The reason why i chose an out-parameter rather than returning a std::pair is because most callers presumably don't need this feature.

NoQ added a comment.Aug 16 2018, 11:41 AM

which is of course a bad thing to do because we should not bind Locs to prvalue expressions

... of non-pointer type. On the other hand, binding NonLocs to glvalue expressions is always a bad thing to do; but, for the reference, adding this as an assertion currently crashes 92 tests.

NoQ added a comment.Aug 16 2018, 12:25 PM

...and adding the aforementioned assertion for prvalues increases the number of crashes on tests to 196. It seems that the analyzer assigns values of improper loc-less very often, but then immediately overwrites them :/ I wonder how much performance could be gained by fixing these bugs.

george.karpenkov requested changes to this revision.Aug 21 2018, 10:55 AM

looks good apart from minor nits

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
744 ↗(On Diff #161073)

In general I would say that pair is still preferable, but I agree that with pre-c++17 codebase it gets too verbose.

lib/StaticAnalyzer/Core/ExprEngine.cpp
423 ↗(On Diff #161073)

could we have braces here? Once we have "else" I would say we should have those.

2543 ↗(On Diff #161073)

Let's initialize that to nullptr

This revision now requires changes to proceed.Aug 21 2018, 10:55 AM
NoQ updated this revision to Diff 162755.Aug 27 2018, 2:55 PM

Address comments.

george.karpenkov accepted this revision.Aug 27 2018, 4:20 PM
This revision is now accepted and ready to land.Aug 27 2018, 4:20 PM
This revision was automatically updated to reflect the committed changes.