Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenBLAS support broken due to extern C linkage #10668

Closed
nightsparc opened this issue Jan 23, 2018 · 6 comments · Fixed by #10677
Closed

OpenBLAS support broken due to extern C linkage #10668

nightsparc opened this issue Jan 23, 2018 · 6 comments · Fixed by #10677

Comments

@nightsparc
Copy link

System information (version)
  • OpenCV => 3.4.0
  • Operating System / Platform => Ubuntu 16.04
  • Compiler => GCC 5.4
Detailed description

Hi!

we build OpenCV with OpenBLAS 0.2.20 (also our own build) ...ah, up to patch #9955, LAPACK detection had been working properly.

Starting with the changes in patch #9955 in the 3.3.1 release (and 3.4.0 too, to which we're trying to upgrade to atm), the LAPACK cmake detection script outputs

-- LAPACK(OpenBLAS): LAPACK_LIBRARIES: /usr/local/lib/libopenblas.so
-- LAPACK(OpenBLAS): Can't build LAPACK check code. This LAPACK version is not supported.

I was able to track it down to the extern "C" encapsulation of the OpenBLAS headers in the lapack proxy which seems similar to issue #10230. Now, if we're removing the extern "C" statement, everything works fine again.

I'm just wondering if it this is due to some trouble with our own build version of OpenBLAS as #9955 was related to the OpenBLAS support in OpenCV, too. @alalek Why did you put the OpenBLAS headers in extern C linkage?

Workaround

Atm, I'd removed the extern C linkage from the detection script to build OpenCV with OpenBLAS...

Thanks in advance and regards

@alalek
Copy link
Member

alalek commented Jan 23, 2018

LAPACK/BLAS is "C" library and it should have "C" LAPACK's symbols (exception here is "Apple"-way with their framework).

our own build

Try to run "nm -g" on your library build and check for names of exported LAPACK symbols.

Some LAPACK libraries/packages doesn't have extern "C" at all in headers (like OpenBLAS many years ago), so it was forced to avoid observed linker error with mangled C++ LAPACK symbols.

Nested extern "C" blocks are compiled pretty well, so it should not be a problem.
Try it here:

Example
extern "C" {
extern "C" {
int square(int num);
}
}

int square(int num) {
    return num * num;
}

@nightsparc
Copy link
Author

Ah, I'm aware of the LAPACK/BLAS origins and the correct usage of extern "C" :) - sorry if my post was irritating...but thanks for the SA link, the std reference for nested linkage is nice 👍
I already tried nm before posting...all symbols are (as expected) easily readable C-symbols, no C++ name mangling there, sorry, forgot adding it.

I don't know what apple is doing in their framework, I was just guessing from #10230 comments and the differences in the CMake detection script.

Ok, I tracked down the error a bit more...fortunately, there was an error output in the OpenCVFindLAPACK.cmake :)

It seems, the error comes from the usage of GCC complex number library:

In file included from /usr/include/c++/5/complex:42:0,

            from /usr/include/c++/5/ccomplex:38,
            from /usr/include/c++/5/complex.h:32,
            from /usr/local/include/openblas/lapacke.h:73,
            from /tmp/opencv-3.4.0/release/opencv_lapack.h:3,
            from /tmp/opencv-3.4.0/cmake/checks/lapack_check.cpp:1:

/usr/include/c++/5/bits/cpp_type_traits.h:72:3: error: template with C linkage

template<typename _Iterator, typename _Container>
...
...

I've set ENABLE_CXX11=ON in my configure options, which forces try_compile to set the compile std to C++11, thus complex.h includes ccomplex which requires templates which are non-existent in C :)

#if __cplusplus >= 201103L
#include <ccomplex>
#endif

So, forcing try_compile to use C++98 for the test compilation fixes the configure bug. But that's a bit of an ugly hack.

So, sorry. I'm still not getting it - why are you putting a library which already has C-linkage for it's symbols into another nested C-linkage layer? And thereby putting the std library headers into extern linkage blocks, from which they're intentionally excluded! See OpenMathLib/OpenBLAS#490

I could provide a patch for the OpenCVFindLAPACK.cmake if you want...

Thank you!

@alalek
Copy link
Member

alalek commented Jan 23, 2018

Because there are packages with straightforward LAPACK/CBLAS headers without any extern "C" at all.
They can't be used without additional extern "C" because we need to avoid C++ symbols mangling.

Perhaps we could add "include <complex.h>" before include "opencv_lapack.h" (in lapack_check.cpp and modules/core/src/hal_internal.cpp) to workaround this.

BTW, GCC 7 is fine (in Fedora + OpenBLAS): headers list looks the same (use -H compiler option to see them), but there is no error message.

https://godbolt.org/ works fine with GCC 6.1+ / Clang:

#include <complex.h> // comment this to get error with "--std=c++11 -H"
extern "C" {
#include <complex.h>
}

@nightsparc
Copy link
Author

Because there are packages with straightforward LAPACK/CBLAS headers without any extern "C" at all.

Oh, my, fault. Sorry - I didn't really thought about other LAPACK/BLAS impl. besides OpenBLAS (and this strange Apple stuff) :/ my fault, sorry again.

BTW, GCC 7 is fine (in Fedora + OpenBLAS): headers list looks the same (use -H compiler option to see them), but there is no error message.

That's strange...what version of OpenBLAS is Fedora shipped with? I'd assumed a bit more up to date than Ubuntu LTS?

Anyway, your workaround suggestion looks fine for me...I'll test it and provide the patch when it's working.

@alalek
Copy link
Member

alalek commented Jan 24, 2018

@nightsparc Could you take a look on #10677 ?

@nightsparc
Copy link
Author

@alalek #10677 looks fine and works for me - thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants