Page MenuHomePhabricator

[Analyzer] Fix for `ExprEngine::computeObjectUnderConstruction()` for base and delegating consturctor initializers
ClosedPublic

Authored by baloghadamsoftware on Aug 5 2020, 12:45 PM.

Details

Summary

For /C++/ constructor initializers ExprEngine:computeUnderConstruction() asserts that they are all member initializers. This is not neccessarily true when this function is used to get the return value for the construction context thus attempts to fetch return values of base and delegating constructor initializers result in assertions. This small patch fixes this issue.

Diff Detail

Event Timeline

@NoQ Could you please take a look on this one? It is a fix of my earlier work.

Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.

Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.

Why should we? This is just a fix for cases not covered, but it is the same functionality (retrieving the return value under construction). I added the missed cases to the test of this exact functionality.

Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.

Why should we? This is just a fix for cases not covered, but it is the same functionality (retrieving the return value under construction). I added the missed cases to the test of this exact functionality.

I think its a bad experience if you break something while developing. Instead of getting a test failure for "delegating constructor initializers", you'll have to deal with a test that handles a variety of things at once, and are forced to tease it apart to find what just broke. When the introduced assert fires, this wouldn't be an issue, but in any non-crashing case it might be.

I think its a bad experience if you break something while developing. Instead of getting a test failure for "delegating constructor initializers", you'll have to deal with a test that handles a variety of things at once, and are forced to tease it apart to find what just broke. When the introduced assert fires, this wouldn't be an issue, but in any non-crashing case it might be.

This is a crashing case.

@NoQ could you please take a look on this short fix?

Tests separated.

The tests look great, thanks! I still lack the confidence to accept, unfortunately.

NoQ added inline comments.Sep 8 2020, 12:43 PM
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
138–139

For base initializer you probably want the base class region. It may have a non-trivial offset and it also has the correct type and extent.

Fix for base constructors, test extended.

baloghadamsoftware marked an inline comment as done.Sep 10 2020, 2:53 AM
baloghadamsoftware added inline comments.
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
138–139

Thank you for noticing this! You are completely right! Now I extended the tests with comparing the type of the returned region with the type of the expression. It failed when I just returned ThisVal but it passes with the fixed code.

NoQ accepted this revision.Sep 21 2020, 10:50 PM

Aha, yup, thanks, this looks good!

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
146

Whitespace?

378

Let's add some reasoning, eg. "Base and delegating initializers handled above"?

This revision is now accepted and ready to land.Sep 21 2020, 10:50 PM