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

Add mlpack::backtrace utility. #535

Merged
merged 29 commits into from Mar 9, 2016
Merged

Add mlpack::backtrace utility. #535

merged 29 commits into from Mar 9, 2016

Conversation

Kirizaki
Copy link
Contributor

@Kirizaki Kirizaki commented Mar 1, 2016

When build: cmake -DDEBUG=ON ../, Log::Assert() & Log::Fatal prints backtrace.
ONLY FOR LINUX

@rcurtin
Copy link
Member

rcurtin commented Mar 1, 2016

Sweet! I will look through this and test it tomorrow.

@Kirizaki
Copy link
Contributor Author

Kirizaki commented Mar 2, 2016

Why it doesn't build correctly? I didn't change anything in tests... Maybe I just merge my branch with master which has some bug and then make my PR or my changes has conflicts with test no. 4 on which travis is crushing?

@rcurtin
Copy link
Member

rcurtin commented Mar 2, 2016

The travis build was a random test failure, it has nothing to do with your PR, so don't worry about it. Probabilistic tests can be difficult to deal with for this reason...

@Kirizaki
Copy link
Contributor Author

Kirizaki commented Mar 3, 2016

Ohh, that's the thing!

if(LIBBFD_FOUND AND LIBDL_FOUND)
include_directories(${LIBBFD_INCLUDE_DIRS})
include_directories(${LIBDL_INCLUDE_DIRS})
add_definitions(-DHAS_BFD_DL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to add set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -rdynamic") here; if I didn't do this, then if an assert or fatal error was thrown in any of the mlpack programs (like mlpack_allknn), it wouldn't give a backtrace but would tell me that I needed to compile with -rdynamic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@rcurtin
Copy link
Member

rcurtin commented Mar 3, 2016

I tested this on Linux, it works great when I make the necessary change to CMakeLists.txt. I tried to compile on OS X and I get this:

[2%] Building CXX object src/mlpack/CMakeFiles/mlpack.dir/core/util/backtrace.cpp.o
clang: warning: argument unused during compilation: '-rdynamic'
In file included from /Users/ryan_curtin/src/mlpack-kirizaki/src/mlpack/core/util/backtrace.cpp:22:
/opt/local/include/bfd.h:35:2: error: config.h must be included before this header
#error config.h must be included before this header
 ^
1 error generated.

I think that this has been tested on OS X and is known to not work, so maybe we disable HAS_BFD_DL in the CMake configuration if we are on OS X? Either way we should be sure that compilation works out-of-the-box on OS X.

Also there is the warning from clang that -rdynamic is unsupported; maybe different handling is necessary there?
http://stackoverflow.com/questions/21279036/what-is-clangs-equivalent-to-rdynamic-gcc-flag/21279573#21279573
That one will be a little bit weird because apparently the flag is different on OS X!

I haven't been able to actually test with clang because of a Debian bug that affects the system I was testing on:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=797917

It would also be cool to test this on FreeBSD but my FreeBSD box is not running right now... :(

@rcurtin
Copy link
Member

rcurtin commented Mar 3, 2016

I should add, I'm very excited about this, I think this backtrace is a really nice improvement. 👍

@Kirizaki
Copy link
Contributor Author

Kirizaki commented Mar 3, 2016

Part You pointed in code is done.

Second thing:

I think that this has been tested on OS X and is known to not work, so maybe we disable HAS_BFD_DL in the CMake configuration if we are on OS X? Either way we should be sure that compilation works out-of-the-box on OS X.

Disabling HAS_BFD_DL is temporary but good idea at the moment. Tomorrow I should take care about this.

But in near future it has to be done properly. @zoq did some research about compiling mlpack::backtrace in OS X. There is also problem with dependencies in binutils.

[Friday, December 11, 2015] [09:15:03 PM] Okay, it turns out that if you install binutils using homebrew it doesn't come with libiberty because of 'Homebrew/legacy-homebrew#35881'.
[Friday, December 11, 2015] [09:15:17 PM] Unfortunately there is no brew formula for libiberty. So you need to install it manually. Afterwards, you can link against '-liberty'.
[Friday, December 11, 2015] [09:15:30 PM] Unfortunately there is no brew formula for libiberty. So you need to install it manually. Afterwards, you can link against '-liberty'. But now it turns out you also need to link against libintl which comes with the gettext package. Turns out gettext ships with OS X (default). So my gettext package conflicts with the default version.
[Friday, December 11, 2015] [09:15:44 PM] No problem adjust the path and we are fine. At the end we can link against the lib '-lintl'.
[Friday, December 11, 2015] [09:15:52 PM] Now the source code build fine, but prints an error message 'Failed to open executable! : No such file or directory'. On OS X there is no '/proc/self/exe', any ideas?

So my next step is to prevent building mlpack::backtrace in OS X and FreeBSD. This should help for a while. Later on I will just work on extending this feature for other platforms.

@rcurtin
Copy link
Member

rcurtin commented Mar 3, 2016

Awesome, this sounds like a good approach. I'm fine with this only working on Linux for now (because I think what we have implemented only works on Linux, and it doesn't work very well), and we can extend support later. We just have to make sure that it still compiles correctly on other platforms before we merge it in. :)

@Kirizaki
Copy link
Contributor Author

Kirizaki commented Mar 3, 2016

I agree. I forget that mlpack is not only for Linux users. :P

Use CreateNVP from the data namespace.
@zoq
Copy link
Member

zoq commented Mar 3, 2016

Okay, I tested the code on FreeBSD, and ended up with the same error as for OSX: there is no '/proc/self/exe'. I changed the CMakeLists.txt file so that it doesn't look for the libdl library. libdl is is part of libc so FindLibDL always returns "libdl not found". Anyway, everything builds fine, without changing the CMakeLists.txt file.

Find and compile libbfd & libdl only when OS is Linux.
...find and compile libbfd & libdl only when OS is Linux.
...compile backtrace.hpp only when OS is Linux
zoq added a commit that referenced this pull request Mar 9, 2016
Add mlpack::backtrace utility.
@zoq zoq merged commit 1a9c41a into mlpack:master Mar 9, 2016
@zoq
Copy link
Member

zoq commented Mar 9, 2016

Since, we tested this on FreeBSD, OSX and windows, yes right Microsoft Windows. I'll go and merge this in. Thanks for the contribution!

@rcurtin
Copy link
Member

rcurtin commented Mar 14, 2016

Thanks again, this is great! :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants