[MXNET-244] Work around likely compiler bug on nested inlines and temporary acces…

[larroy]

…s to stream

Description

This fixes a segfault in ARMv7 on throwing a fatal error on the lapack undefined functions.

Addresses #13342
We don't crash as before, the test fails which is expected.

I think this is a bug in the compiler due to the temporary LogMessageFatal created on unwrapping the macro https://github.com/apache/incubator-mxnet/blob/master/src/operator/c_lapack_api.h#L369 and too many inlined functions. The segfault is inside the ostream in libc, could be related to the lifetime of the temporary variable with a reference to the stream which is a very corner case of temporary variable lifetime.

Having the functions not inlined solves the problem.

qemu@qemu:~/mxnet/build/tests$ ./mxnet_unit_tests --gtest_filter="inv_khatri_rao.OneInputMatrixTransposed"

Note: Google Test filter = inv_khatri_rao.OneInputMatrixTransposed
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from inv_khatri_rao
[ RUN      ] inv_khatri_rao.OneInputMatrixTransposed
unknown file: Failure
C++ exception with description "[13:27:04] /work/mxnet/src/operator/c_lapack_api.cc:42: MXNet build without lapack. Function dposv is not available.

Stack trace returned 10 entries:
[bt] (0) ./mxnet_unit_tests(dmlc::StackTrace()+0x40) [0x1291afc]
[bt] (1) ./mxnet_unit_tests(dmlc::LogMessageFatal::~LogMessageFatal()+0x40) [0x1291e2c]
[bt] (2) ./mxnet_unit_tests(mxnet_lapack_dposv(...)+0x88) [0x19466dc]
[bt] (3) ./mxnet_unit_tests(int MXNET_LAPACK_posv<double>(int, char, int, int, double*, int, double*, int)+0x58) [0x13d7070]
[bt] (4) ./mxnet_unit_tests(void mxnet::op::inv_khatri_rao<double>(mshadow::Tensor<mshadow::cpu, 2, double>, std::vector<mshadow::Tensor<mshadow::cpu, 2, double>, std::allocator<mshadow::Tensor<mshadow::cpu, 2, double> > > const&, bool)+0x394) [0x13da000]
[bt] (5) ./mxnet_unit_tests(mxnet::op::inv_khatri_rao_OneInputMatrixTransposed_Test::TestBody()+0x238) [0x13cf0e8]
[bt] (6) ./mxnet_unit_tests(void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x74) [0x1491b50]
[bt] (7) ./mxnet_unit_tests(void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)+0x4c) [0x148bb10]
[bt] (8) ./mxnet_unit_tests(testing::Test::Run()+0xe8) [0x146d9f8]
[bt] (9) ./mxnet_unit_tests(testing::TestInfo::Run()+0x138) [0x146e310]

" thrown in the test body.
[  FAILED  ] inv_khatri_rao.OneInputMatrixTransposed (70 ms)
[----------] 1 test from inv_khatri_rao (74 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (90 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] inv_khatri_rao.OneInputMatrixTransposed

 1 FAILED TEST

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

• The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
• Changes are complete (i.e. I finished coding on this PR)
• All changes have test coverage:
• Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
• Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
• Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
• Code is well-documented:
• For user-facing API changes, API doc string has been updated.
• For new C++ functions in header files, their functionalities and arguments are documented.
• For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
• Check the API doc at http:https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
• To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

• Feature1, tests, (and when applicable, API doc)
• Feature2, tests, (and when applicable, API doc)

Comments

• If this change is a backward incompatible change, why must this change be made.
• Interesting edge cases to note here

[jlcontreras]

Why are these files in the src/operator/ dir? (Seems strange to me, as the rest of the files in there belong to operators)

[larroy]

@jlcontreras good point, but that was there already, I think its because lapack is used in some operators. Do you have a better suggestion?

[jlcontreras]

Difficult to say, maybe common/ ?

[larroy]

@jlcontreras do you think it makes sense to keep it in a different PR?

[lebeg]

Why this is needed?

[larroy]

to avoid being defined again in the implementation

[lebeg]

Why commenting out? Maybe it should go to c_lapack_api.cc?

[larroy]

Good point

[asmushetzel]

LGTM. Don't think that it makes sense to move the c_lapack_api to a different directory now. There are pros and cons about where it should sit, but no immediate benefit from moving.

[larroy]

@szha @zhreshold

[larroy]

@mxnet-label-bot [pr-awaiting-merge]

[KellenSunderland]

Any idea why the CI failed? Looks like a timeout right?

[larroy]

@KellenSunderland I guess we can merge now. Thank you.

[larroy]

@szha @KellenSunderland @tqchen could we please merge this? it's needed for fixing crashes on some scenarios. Thank you.