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 1646 - Aliasing detected while no alias actually occurs
Summary: Aliasing detected while no alias actually occurs
Status: DECISIONNEEDED
Alias: None
Product: Eigen
Classification: Unclassified
Component: General (show other bugs)
Version: 3.3 (current stable)
Hardware: All All
: Normal Crash
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-14 04:34 UTC by taroxd
Modified: 2019-01-16 14:50 UTC (History)
4 users (show)



Attachments

Description taroxd 2018-12-14 04:34:02 UTC
Here is my environment information although I don't think this bug is related to this:
OS: Windows 10 (17763) x64
Compiler: Visual Studio 2017
Eigen: 3.3.7


Take the following code for example (I am aware that SelfAdointView can achieve the same thing; it is just an example):

int n = 100;
MatrixXd A = MatrixXd::Random(n, n);

for (int j = 1; j < n; ++j) {
    A.col(j).head(j) = A.row(j).head(j);
}

std::cout << (A - A.adjoint()).norm() << std::endl;


Running this code in debug mode, I got the assertion failure:

Assertion failed: (!check_transpose_aliasing_run_time_selector <typename Derived::Scalar,blas_traits<Derived>::IsTransposed,OtherDerived> ::run(extract_data(dst), other)) && "aliasing detected during transposition, use transposeInPlace() " "or evaluate the rhs into a temporary using .eval()", file path\to\eigen\src\core\transpose.h, line 378



Apparently A.col(j).head(j) and A.row(j).head(j) does not share the same memory. It makes no sense to evaluate the rhs into a temporary. Also, if the code is compiled in release mode (with NDEBUG defined), the code runs perfectly.

I find out that changing this code to 
  A.col(j).head(j).noalias() = A.row(j).head(j);
does not help. But
  A.col(j).head(j) = A.row(j).adjoint().head(j);
works fine. 

I have no idea when the assertion will be triggered. Do I have a way to explicitly disable it? I am very sure that aliasing does not occur.
Comment 1 Christoph Hertzberg 2018-12-14 11:03:22 UTC
Works fine with g++ and clang++ with Eigen-default and Eigen-3.3.

I would still generally advice to explicitly transpose/adjoint row vectors before assigning them to column vectors (at many places this happens implicitly, but you may get unexpected behavior in some situations):
   A.col(j).head(j) = A.row(j).head(j).transpose();

Also, the shorter version of your loop would be:

   A.triangularView<Eigen::StrictlyUpper>() = A.transpose();

There is no (easy) way to disable only the transpose_aliasing assertions, and I don't think that this would make sense.
Comment 2 taroxd 2018-12-14 11:44:39 UTC
I am well aware that a shorter version
   A.triangularView<Eigen::StrictlyUpper>() = A.transpose();
is available. The code I have provided is only a minimal example that triggers this unexpected behavior.

I will be glad to explicitly transpose row vector before assigning, if doing so is not hiding a bug about alias detection.
Comment 3 Christoph Hertzberg 2018-12-14 15:47:01 UTC
With clang/gcc I don't even get aliasing assertions when removing the `.head(j)`

I do get the assertion if I let your loop start at j=0, instead of 1 (did you mis-copy your code snippet?).
The simplest workaround for this issue would be to disable the assertion, if the size is 0 or 1.

We had that discussion at some place -- but distinguishing all valid from invalid aliasing problems is hard (at least to hard for simple run-time assertions), e.g. for square matrices
   A.row(i) = A.col(i).transpose();
is always fine, since A(i,i) simply gets assigned to itself, whereas for
   A.row(i) = A.col(j).transpose();
it depends on the order values get copied, if (i!=j).
Comment 4 Gael Guennebaud 2018-12-14 21:22:06 UTC
Christoph, don't you think that .noalias() should turn off the check? Since there are false positives, it makes sense to provide a mechanism to shutdown them, and I think .noalias() is the right tool for that. I don't think users put .noalias() everywhere without thinking.
Comment 5 taroxd 2018-12-15 00:46:38 UTC
(In reply to Christoph Hertzberg from comment #3)
> With clang/gcc I don't even get aliasing assertions when removing the
> `.head(j)`
> 
> I do get the assertion if I let your loop start at j=0, instead of 1 (did
> you mis-copy your code snippet?).
> The simplest workaround for this issue would be to disable the assertion, if
> the size is 0 or 1.
> 
> We had that discussion at some place -- but distinguishing all valid from
> invalid aliasing problems is hard (at least to hard for simple run-time
> assertions), e.g. for square matrices
>    A.row(i) = A.col(i).transpose();
> is always fine, since A(i,i) simply gets assigned to itself, whereas for
>    A.row(i) = A.col(j).transpose();
> it depends on the order values get copied, if (i!=j).

Oops, I did mis-copy my code snippet. The loop starts at j=0 when I am testing. Sorry for that.
Comment 6 Christoph Hertzberg 2018-12-15 15:20:14 UTC
(In reply to Gael Guennebaud from comment #4)
> Christoph, don't you think that .noalias() should turn off the check? 

The difference to the product-noalias would be that it just deactivates an assertion, vs actually generating different code. From the current API I would rather expect that .noalias() does more effort to check for accidental aliases (if possible with reasonable effort).

The case which caused this assertion would actually be an alias-issue for any size other than 0 (even though the alias would be harmless). So I'd prefer just deactivating the check for size==0 (or size<=1).
Comment 7 Gael Guennebaud 2019-01-16 13:54:14 UTC
I fixed this false positive:

https://bitbucket.org/eigen/eigen/commits/2ea3c449716a/
User:        ggael
Summary:     Bug 1646: disable aliasing detection for empty and 1x1 expression


but we still have false positives, at least:

A.row(0) = A.col(0).transpose();
Comment 8 Gael Guennebaud 2019-01-16 14:01:10 UTC
It seems to me that our current checks cannot positively detect aliasing in case of vectors anyway. So we could simply disable this check when the destination is empty or a vector. Am I overseeing some corner cases?
Comment 9 Christoph Hertzberg 2019-01-16 14:50:02 UTC
I guess we can't (with reasonable effort) detect valid cases like these anyway:

  A.row(i) = A.col(j).transpose();

(Similar for: v.segment(i,m) = v.segment(j,n), but that is a different issue)
So I'm ok with deactivating this assertion for vectors.

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