New user self-registration is currently disabled. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Bug 755 - CommaInitializer bug in absence of RVO
CommaInitializer bug in absence of RVO
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Core - expression templates
unspecified
All All
: High major
Assigned To: Nobody
:
Depends on:
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2014-03-07 16:40 UTC by Jitse Niesen
Modified: 2014-03-12 14:04 UTC (History)
4 users (show)



Attachments
Proof-of-concept patch to avoid assertions if RVO is deactivated (3.46 KB, patch)
2014-03-07 17:31 UTC, Christoph Hertzberg
no flags Details | Diff
Simpler and unified workaround (1.75 KB, application/octet-stream)
2014-03-08 09:14 UTC, Gael Guennebaud
no flags Details

Description Jitse Niesen 2014-03-07 16:40:34 UTC
As discussed on IRC. No time to explore further, so recording log below. Bug 716 looks like it is related.

[11:32] <zfmgpu> Hello :-)
[11:32] <zfmgpu> short question
[11:32] <zfmgpu> the comma initializer seems to not work
[11:33] <zfmgpu> because the assert is always throwing an error
[11:33] <zfmgpu> Eigen::Matrix3f m; m << 1, 2, 3,4, 5, 6,7, 8, 9; std::cout << m;
[11:33] <zfmgpu> throws an error in debug mode
[11:34] <zfmgpu> always , because the CommaInitializer class is destructed after the code m << 1 is executed (it returns a CommaInitializer class by value)
[11:35] <zfmgpu> is this correct behaviour or wrong?
[11:38] <jitseniesen> zfmgpu: that should work, and it does work on my computer. What compiler are you using and what version of Eigen?
[11:38] <zfmgpu> eigen 3.2
[11:38] <zfmgpu> gcc c++11
[11:38] <zfmgpu> gcc 4.7
[11:39] <zfmgpu> EIGEN_DEVICE_FUNC
[11:39] <zfmgpu>   inline ~CommaInitializer()
[11:39] <zfmgpu>   {
[11:39] <zfmgpu>     eigen_assert((m_row+m_currentBlockRows) == m_xpr.rows()
[11:39] <zfmgpu>          && m_col == m_xpr.cols()
[11:39] <zfmgpu>          && "Too few coefficients passed to comma initializer (operator<<)");
[11:39] <zfmgpu>   }
[11:40] <zfmgpu> isn't the destructor called after this call:
[11:40] <zfmgpu> template<typename Derived>
[11:40] <zfmgpu> inline CommaInitializer<Derived> DenseBase<Derived>::operator<< (const Scalar& s)
[11:40] <zfmgpu> {
[11:40] <zfmgpu>   return CommaInitializer<Derived>(*static_cast<Derived*>(this), s);
[11:40] <zfmgpu> }
[11:41] <zfmgpu> this creates a temporary, right? which is copied to the return value, the temp gets deleted -> which calls the dtor
[11:41] <ChriSopht> hm, it appears we relied on RVO at that point
[11:41] <zfmgpu> ok,
[11:41] <zfmgpu> not guaranteed in debug mode, hm....
[11:41] <ChriSopht> but that's definitely not good practice
[11:42] <zfmgpu> what is not good practice?
[11:42] <zfmgpu> the code
[11:42] <ChriSopht> on C++11 we could make a clean solution with proper move-constructors
[11:42] <zfmgpu> jeah
[11:42] <ChriSopht> not good practice: relying on compiler optimization to have working code
[11:43] <zfmgpu> but anyway the eigen_assert would stay in the constructor also with Move semantics?
[11:43] <ChriSopht> we do need a solution that is guaranteed to work in C++03 mode as well
[11:44] <ChriSopht> we can make the move constructor set a flag in the deconstructed class to mark it as cleanly destructed somehow (in C++11)
[11:46] <zfmgpu> jeah, that would work with c++11
[11:46] <zfmgpu> but what with c++03
[11:46] <zfmgpu> it works only if compiler uses RVO
[11:46] <zfmgpu> saddly :-)
[11:47] <zfmgpu> would it be possible to put the eigen assert somewhere else?
[11:47] <zfmgpu> move the assert into the finished call if one wants an assert
[11:48] <zfmgpu> would that be an option maybee
[11:48] <ChriSopht> I'm afraid that would lose many valid asserts
[11:49] <ChriSopht> we could only activate asserts after the first operator,() is called
[11:49] <ChriSopht> I think starting at that point it is guaranteed that we have only one object which returns itself by reference
[11:51] <ChriSopht> and it would only not catch (senseless) calls such as A<<1.0;
[11:52] <ChriSopht> and for C++11 we can make a clean, valid solution
[11:55] <jitseniesen> ChriSopht: I'm not very happy with your solution, but I can't think of anything better
[11:55] <ChriSopht> jitseniesen: to reproduce compile with -fno-elide-constructors (on gcc)
[12:00] <ChriSopht> zfmgpu: could you file a bug-report please?
[12:03] <ChriSopht> a slightly-hacked solution in C++03 mode would be to const_cast the source object in the copy-constructor and mark it as clean
[12:04] <ChriSopht> we can disable that in NDEBUG mode (but I don't think perfomance is an issue for that kind of I/O)
[12:06] <ChriSopht> I'm not sure, but I hope we can rely on that the compiler does not do completely unmotivated additional copies
[12:20] <jitseniesen> I guess that's how std::auto_ptr works, transfering ownership in operator=()
[12:20] <jitseniesen> that sounds better to me
[12:21] <jitseniesen> I agree that performance is not an issue
[12:21] <ChriSopht> yes, right. operator=() needs to be re-defined as well
[12:22] <jitseniesen> ah, you talked about copy constructor, not operator=
[12:23] <ChriSopht> and std::auto_ptr is a good example that it (guaranteeing that the constructor is called exactly once) works somehow
[12:23] <jitseniesen> instead of const-cast-ing, can't we declare the copy c'tor to take a CommaInitializer& instead of a const CommaInitializer& ?
[12:24] <ChriSopht> yes, right. I think that is also how auto_ptr does it
[12:47] <ChriSopht> instead of adding a new member, we could also set m_col=m_xpr.cols(); m_row=m_xpr.rows(); m_currentBlockRows = 0;
[12:47] <ChriSopht> if RVO is enabled, we will not even have a performance regression
[12:58] <ChriSopht> simply providing CommaInitializer(CommaInitializer&) is not sufficient, unfortunately
[12:59] <ChriSopht> std::auto_ptr has a helper-class auto_ptr_ref
[13:46] <ChriSopht> I managed to create a working version using a CommaInitializerRef helper class
[13:47] <ChriSopht> however, I'm afraid this is not removed by RVO
Comment 1 Christoph Hertzberg 2014-03-07 17:31:04 UTC
Created attachment 424 [details]
Proof-of-concept patch to avoid assertions if RVO is deactivated

Unfortunately it seems this patch generally prohibits RVO. In C++11 mode with rvalue references it takes the short-cut via the move constructor.
I accidentally worked on the 3.2 branch, but for the devel-branch it should work similarly.

I'm not sure if this has an impact on Bug 716, but I guess not.
Comment 2 Patrick Mihelich 2014-03-08 02:31:12 UTC
Awesome, I just ran into this and you guys are already on top of it :). We're using -fno-elide-constructors in our coverage build due to a gcc/gcov limitation: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12076.

Christoph's patch does work for me locally, with or without C++11. We are using C++11 in our build, so I'm happy to see move constructor support.
Comment 3 Gael Guennebaud 2014-03-08 09:14:00 UTC
Created attachment 425 [details]
Simpler and unified workaround

The previous patch does not work with clang. For some weird reasons, in the ctor from a CommaInitRef we get corrupted values. I tried to add a manual copy ctor in CommaInitRef but it is not called, If I make this copy ctor private, then it refuse to compile though.

Nevertheless, I think we can apply the same trick for c++11 or c++03, simply using a const_cast... This simpler workaround is still compatible with RVO in c++03 mode.
Comment 4 Christoph Hertzberg 2014-03-08 10:35:27 UTC
(In reply to comment #3)
> Nevertheless, I think we can apply the same trick for c++11 or c++03, simply
> using a const_cast... This simpler workaround is still compatible with RVO in
> c++03 mode.

Yes, simply doing a const_cast was what I also had in mind (comment 0, [12:03]). 
I guess it is safe enough here, so go ahead and push that simpler patch.
Comment 5 Christoph Hertzberg 2014-03-12 14:04:32 UTC
I applied Gael's workaround and also added some comments to the constructor:
https://bitbucket.org/eigen/eigen/commits/a2b4a00
https://bitbucket.org/eigen/eigen/commits/2c5d4f0

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