New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 505 - Assert if temporary objects that are still referred to get destructed (was: Misbehaving Product on C++11)
Summary: Assert if temporary objects that are still referred to get destructed (was: M...
Status: RESOLVED WONTFIX
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - matrix products (show other bugs)
Version: 3.4 (development)
Hardware: All All
: High Internal Design
Assignee: Nobody
URL:
Whiteboard:
Keywords:
: 646 834 (view as bug list)
Depends on: ExprEval 825
Blocks: 3.4 646
  Show dependency treegraph
 
Reported: 2012-09-03 06:41 UTC by brett.bernstein
Modified: 2019-01-17 15:34 UTC (History)
9 users (show)



Attachments
Patch moving product temporaries into parent expression (4.39 KB, patch)
2013-02-26 00:27 UTC, Gael Guennebaud
no flags Details | Diff

Description brett.bernstein 2012-09-03 06:41:25 UTC
This very well could be a GCC 4.7 -std=c++11 issue, but here is the code and output:

#include <iostream>
#include <Eigen/Dense>

using namespace std;
using namespace Eigen;

template<class A, class B, class C>
double combine(const A& a, const B& b, const C& c)
{
    auto result = a*b-c;
    cout << result << endl;
    return result(0,0);
}

int main()
{
    Matrix<double,1,1> X, y, w, g;
    X << 4;
    y << 2;
    w << 4;
    
    cout << combine(X,y,w) << endl;
    cout << X*y-w << endl;

    return 0;
}

Build:
g++ -c -O3 -Wall -Ic:/C++/include -std=c++11  main.cpp -o main.o
g++ -Lc:/C++/lib main.o -lgtest -o main.exe
rm main.o

Output:
-4
-4
4

Error goes away when I remove -O3.
Comment 1 brett.bernstein 2012-09-03 06:48:08 UTC
-lgtest is a red herring, and not actually used here.
Comment 2 brett.bernstein 2012-09-04 04:57:39 UTC
As an added data point, I have now had other issues without -O3 using auto.
Comment 3 Christoph Hertzberg 2012-12-19 15:27:27 UTC
I can confirm this bug also for Linux 32bit (and gcc4.7), it even appears with -01 and also for Eigen3.0.6. It seems as if the result of the product is discarded at some point. Running the -O version with valgrinds raises a lot of unitialized values warnings.

Btw: Without -std=c++11 and replacing the auto-line by

    Eigen::CwiseBinaryOp<Eigen::internal::scalar_difference_op<double>, const Eigen::Matrix<double, 1, 1>, const Eigen::Matrix<double, 1, 1> > result = a*b-c;

also leads to the same behavior (invalid results with -O1 or higher).
So I guess in general it is not a good idea to store expressions involving products.
Comment 4 Jitse Niesen 2013-02-18 14:32:08 UTC
I'm pretty sure Christoph is correct and that the result of the product is discarded. The product is evaluated in the line "auto result = a*b-c" and the resulting expression object stores a reference to the result of the product, but since the return value of a*b is a temporary object, it is discarded by the time the next line is executed. 

With the expression evaluator, the evaluation will happen later and this problem should disappear (if I understand it correctly).

The interaction between 'auto' and expression objects may be really confusing to new Eigen users who may write things like 'auto c = a + b' where a and b are matrices and expect that c is again a matrix.
Comment 5 Christoph Hertzberg 2013-02-21 11:13:38 UTC
(In reply to comment #4)
> I'm pretty sure Christoph is correct and that the result of the product is
> discarded. The product is evaluated in the line "auto result = a*b-c" and the
> resulting expression object stores a reference to the result of the product,
> but since the return value of a*b is a temporary object, it is discarded by the
> time the next line is executed. 

As a quick-fix (before bug 99 is solved ...), would it be possible to generally store sub-expressions using the Ref<>-class? That way a copy of the result is stored in the expression itself if required and gets discarded only when the expression itself gets discarded.

Alternatively, is there some way to disallow auto c = ...; for certain classes? E.g. does making the copy-constructor private/protected help? Or would that harm at other places?
Comment 6 Gael Guennebaud 2013-02-25 18:49:10 UTC
I like the idea of disallowing the auto keywords for our expression types, but I'm afraid that's not possible.

Currently the temporary matrix is stored by the product expression itself, and the parent stores a const reference to this temporary object. However storing the temporary by the parent sounds possible. I'll try that.
Comment 7 Gael Guennebaud 2013-02-26 00:27:53 UTC
Created attachment 316 [details]
Patch moving product temporaries into parent expression

Here is patch doing so. Unit tests compile and run fine but it looks to simple to be right. There must be something more subtle that prevented us of doing so in the first place (instead of the current ugly conversion operator trick)...
Comment 8 Gael Guennebaud 2013-02-26 11:05:07 UTC
oh, I remember now why that patch is not good: in a complex expression like ((A*B+C)+D)+E) the temporary holding the result of A*B will copied multiple times from (A*B+C) to each parent...
Comment 9 Christoph Hertzberg 2013-08-19 12:53:18 UTC
*** Bug 646 has been marked as a duplicate of this bug. ***
Comment 10 Christoph Hertzberg 2014-06-26 16:23:24 UTC
*** Bug 834 has been marked as a duplicate of this bug. ***
Comment 11 Christoph Hertzberg 2014-06-26 16:29:28 UTC
Better documentation could avoid many related problems.
Comment 12 Christoph Hertzberg 2014-09-23 16:30:56 UTC
*** Bug 883 has been marked as a duplicate of this bug. ***
Comment 13 Chao Liang 2014-09-24 08:25:52 UTC
I think that my report (Bug 883) is slightly different from this one.
This one has been solved, it's said that "a*b" will not cause any error result now.

While in my report, it is true that the problem is cuased by local variables, as "a*b" in this report.
But this time, the local varible is created because of the use of "eval()" function on part of the expression. 
Here is that statement:
auto EAuto = (A.array().rowwise() ) - ( (A.array() * B.array() ).colwise().sum().eval() );

If we don't evaluate the second part, things will be all right.
Or, we can evalauate the whole statement.
Comment 14 Christoph Hertzberg 2014-09-24 09:53:12 UTC
(In reply to Chao Liang from comment #13)
> I think that my report (Bug 883) is slightly different from this one.
Ok, maybe it's more bug 825 then. Still, the reason why your expression fails is the same why products used to fail: EAuto contains a reference to a temporary expression, which is out of scope (thus destructed) at the point where you use it.
We can barely avoid this without introducing overhead: The only solution I can think of would be some kind of shared_pointer with reference counting -- but that's definitely too much overhead for "normal" use!
Comment 15 Martin Rueckl 2014-09-24 15:34:50 UTC
While i stumbled over this issue in Bug 834, and since then, am aware of the problems with auto and temporaries stored in the expression templates, i still occasionally do the same mistake again.
Because of this I set up some small test case to check whether it would be possible to avoid the problem on the library side. While I do not have a large insight into the eigen3 internals, still, here are some remarks/thoughts of mine.
They are all based on c++11 features. If you guys are interested into more details/thoughts, let me know ;-)

-I agree, that with shared pointers one could keep a single expression template for the sum difference class. 
-alternatively one could create a sum class that can only be constructed from rvalue references, this class could then store a reference to lhs/rhs (if lhs/rhs is real data) or a copy of lhs/rhs (if they are again expression templates).
-i think having copys of nested expression templates as data members does not impose big problems, because i suppose, the build up of the resulting object tree can be highly optimized using move semantics.
- this would result in an expression template tree that in the leaf nodes holds the real references to the matrices/vectors the tree operates on.
- alot of this might be possible by overloading operator+ with rvalue references

One major point:
-I do not see a complete solution for problems that arise by using member functions like .transpose(). To solve this, the object where .transpose() is called with would need to be aware whether *this itself is a temporary. One could of course build up the expression template similar to the operator+/- when *this is an expression template object.

Code like:

Matrix a,b,c;
auto t = (a+b).transpose();
cout << t*c<<endl;

would then work like expected without large overhead
Comment 16 Martin Rueckl 2014-09-24 15:54:07 UTC
"the object where .transpose() is called with would need to be aware whether *this itself is a temporary."

Hmm, straight forward would be:
If *this is an expression template, treat it like beeing temporary and copy *this into the created Transposed<..> expression object.
Comment 17 Martin Rueckl 2014-09-24 15:55:10 UTC
> the object where .transpose() is called with would need to be aware whether *this itself is a temporary.

Hmm, straight forward would be:
If *this is an expression template, treat it like beeing temporary and copy *this into the created Transposed<..> expression object.
Comment 18 Christoph Hertzberg 2014-09-24 16:15:40 UTC
(In reply to Martin Rueckl from comment #15)

I do not understand everything you said, but let me try to summarize the problem:
* Expression-Trees shall never store "big data" (i.e. plain dense objects) by value, even with move support this will result in overhead, or it might even be wrong if the original object is to be kept valid.
* If we store an object by reference, we can't detect whether it has been destructed at the time we use it -- that's a general C++ problem:

  struct A {
    const Matrix4d& x; // reference to 'big data' 
    A(const Matrix4d& x_) : x(x_) {}; 
  };

  A foo() {
    Matrix4d data;
    return A(data);  // after method ends, reference inside A will be invalid.
  }

* Objects like MatrixXd _could_ be implemented by storing a shared pointer, but that is neither ABI compatible with the current implementation, nor possible without overhead. For fixed-size objects, I don't see a solution other than by-value copies (a lot of overhead, unless the compiler is really smart).


Your last code example did and does work without much overhead in Eigen before and after the Evaluator integration.

I think the main remaining problem is to teach users not to use the auto keyword for Eigen expressions, unless they really know what they are doing (bug 825)!
Comment 19 Gael Guennebaud 2014-09-24 16:42:27 UTC
(In reply to Martin Rueckl from comment #15)
I do not see any problem with a code like this:

Matrix a,b,c;
auto t = (a+b).transpose();
cout << t*c<<endl;

Currently, only objects storing values are nested by references (i.e., Matrix<>, Array<>, SparseMatrix<>, etc.), and expressions are already nested by values.

The problem is with:

auto t = ((a+b).eval()).transpose();

because (a+b).eval() returns a Matrix<> which is then stored by reference because it is heavy. If you are not scared by multiple copies, you can already use the .nestByValue() method for that:

auto t = ((a+b).eval().nestByValue()).transpose();

Perhaps, we could imagine a variant of eval() returning something which can be properly nested within expression, aka a shared pointer with ref counting. In this case, no overhead for the normal usage.

However, there are more subtle cases like:

auto t = a + (M*b).normalized();

Here M*b is still silently evaluated by Eigen because normalized() is implemented as (*this)/this->norm(), so 'this' is accessed twice and it thus has to be evaluated inside normalized() to avoid evaluating it twice. Perhaps we shall add a Normalized<> expression...
Comment 20 Christoph Hertzberg 2014-09-24 17:10:29 UTC
(In reply to Gael Guennebaud from comment #19)
> The problem is with:
> 
> auto t = ((a+b).eval()).transpose();

The question here is what the user actually intends by that. 

It might be to avoid aliasing, then a function which essentially sets the EvalBeforeNesting flag would be sufficient. We would need to think of a intuitive name for that function. Btw, I found in src/Core/ReturnByValue.h that EvalBeforeNestingBit should be deprecated (not sure how up-to-date that is).

All attempts of .eval() to save operations, should be automatically be detected once the evaluators are fully complete, and if sub-expressions are used multiple times inside one or more expressions, they should be stored into a proper temporary outside the expression.
E.g., I don't see a way to automatically optimize things like:
  Matrix C = ((A*B) + (A*B).transpose())/2;

> However, there are more subtle cases like:
> 
> auto t = a + (M*b).normalized();
> [...]
> Perhaps we shall add a Normalized<> expression...

Yes, I think you had that example (or something similar) in one of your Evaluator-concept presentations.
Comment 21 Martin Rueckl 2014-09-24 17:43:15 UTC
Ok, obviously you guys are already far ahead of my own thoughts :)

To specify, my latest problems raised when using auto in combination with expressions and the cast<..>() method. Somehow i got all issued resolved turning all my boolean matrices into double matrices and dropping all cast method calls. If you are interested i can see if i can rebuild the failing code and provide a minimal example here.
Does cast internally do something similar as eval() / normalized()?

To be honest, i cant imagine a use case for something like:

auto t = ((a+b).eval()).transpose();

While it might not be reasonable code, from my (a users) point of view, it still should either result in the expected outcome, or be invalid code from the start.

Please do not take this as criticism, your library is awesome! :-)
Comment 22 Christoph Hertzberg 2014-09-24 17:59:28 UTC
(In reply to Martin Rueckl from comment #21)
> To specify, my latest problems raised when using auto in combination with
> expressions and the cast<..>() method. Somehow i got all issued resolved
> turning all my boolean matrices into double matrices and dropping all cast
> method calls. If you are interested i can see if i can rebuild the failing
> code and provide a minimal example here.

If you can provide a failing example (preferable one which does not mis-use 'auto'), please file a separate bug report, or first ask at the mailing list/the user forum if your code is valid.

> Does cast internally do something similar as eval() / normalized()?

That might depend on what you are .cast<>()-ing.

> auto t = ((a+b).eval()).transpose();
> 
> While it might not be reasonable code, from my (a users) point of view, it
> still should either result in the expected outcome, or be invalid code from
> the start.

As long as eval() returns a plain (e.g.) Matrix<...> object, we can't distinguish that call from
  MatrixXd A;
  auto t = A.transpose();

We could make eval() return some other kind of expression, but then we still can't distinguish between:

  auto temp = (a+b).eval();               // temp stays in scope
  auto ok   = (temp).transpose();
// and
  auto bad  = ((a+b).eval()).transpose();  // result of eval gets destructed

Because of the specification of 'auto' both calls to transpose() are from the same base type, thus not distinguishable.
Comment 23 Jan van Dijk 2015-04-26 16:35:15 UTC
Just for the record: I just tried to reproduce the OP's test code. That particular problem seems to have been (magically?) fixed in revision 6830.

jan@linux-pwd6:> g++ --version
g++ (SUSE Linux) 4.8.3 20140627 [gcc-4_8-branch revision 212064]
...
jan@linux-pwd6:> g++ -O3 -std=c++11 -I /home/jan/local/eigen-6829/include/eigen3 505.cpp && ./a.out 
-4
-4
4
jan@linux-pwd6:> g++ -O3 -std=c++11 -I /home/jan/local/eigen-6830/include/eigen3 505.cpp && ./a.out 
4
4
4

The results are also correct with today's trunk.
Comment 24 Gael Guennebaud 2015-04-26 20:30:50 UTC
Thank you for confirming this. Nothing magic here, this results from the big expression template refactoring.

Still remains the question about .eval(), as in statements like:

auto t = ((a+b).eval()).transpose();

I see three options:

1 - Keep the current behavior, i.e., return a Matrix<> and document the incompatibility of .eval() and auto.

2 - Make eval() return an Eval<> expression triggering the actual evaluation at evaluation time from the evaluators.

3 - Use a reference-counting matrix-like class for temporaries created by Eigen. Perhaps this mechanism could be added to Ref<>
Comment 25 Christoph Hertzberg 2015-04-27 10:16:10 UTC
Hard question ...

Option 1 is what we've recently been telling users how to assign expressions to an auto variable. Options 2 and 3 would make this at least inefficient,

I guess option 2 would satisfy what eval() originally was intended for (i.e., preventing aliasing effects), but it would break working code if variables get overwritten (or get out of scope) before the actual evaluation.

Option 3 would add some overhead for dynamic matrices, but would hardly be possible for fixed sized objects (maybe a hack with stack-allocation is possible?)

I guess neither option will solve every use case, so I'm tending for keeping the current behavior. Perhaps we can offer the other options under different names?
Comment 26 Christoph Hertzberg 2015-04-27 14:42:51 UTC
About reference counting:
Did you mean simply generating a plain object but add only an additional counter (i.e. nothing as complicated as transferring ownership?)
This would actually be feasible even for fixed-sized objects but we could only raise an assertion in case an Eval-object gets destructed while still getting referenced by expressions.
Technically, the Nested<EvalObject> constructor would have to increase a mutable counter, the Nested destructor would have to decrease it again and the EvalObject destructor has to check that the counter is zero.

I guess the overhead of that would be ok, perhaps compilers are even smart enough to optimize away the reference counting if all referrers are visible. And of course, we can disable the counting (and asserting) in NDEBUG mode.
Comment 27 Gael Guennebaud 2015-06-09 14:55:16 UTC
I initially thought about a complete reference counting approach but your suggestion to limit it to debugging purpose is interesting. In this case we could start with option 1 (we cannot really change the behavior of eval anyway), and later add a lazyEval() and this debugging mechanism.

If we agree on that, there is nothing mandatory for 3.3.
Comment 28 Christoph Hertzberg 2015-06-09 19:34:25 UTC
I agree. I renamed the bug and moved it to 3.4.
Bug 825 should still be addressed for 3.3, I think.
Comment 29 Gael Guennebaud 2015-09-02 12:57:01 UTC
For the documentation part:

https://bitbucket.org/eigen/eigen/commits/6b92a719210b/
Summary:     Bug 505: add more examples of bad and correct usages of auto and eval().
Comment 30 Gael Guennebaud 2019-01-17 15:34:28 UTC
We now have robust address sanitizers to detect such issue, so I wound not "waste" time on implementing sophisticated debugging mechanisms for that.

A lazyEval() might still be desirable though (-> bug 1662).

Note You need to log in before you can comment on or make changes to this bug.