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

914 Fix stacktrace symbols when dladdr fails #915

Merged
merged 2 commits into from
Jul 22, 2020
Merged

Conversation

lifflander
Copy link
Collaborator

Fixes #914

@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #915 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #915   +/-   ##
========================================
  Coverage    82.79%   82.79%           
========================================
  Files          356      356           
  Lines        11232    11232           
========================================
  Hits          9300     9300           
  Misses        1932     1932           

@PhilMiller
Copy link
Member

Before going down this path, why not try out libbacktrace? It's part of gcc, but it's BSD licensed

@lifflander
Copy link
Collaborator Author

Before going down this path, why not try out libbacktrace? It's part of gcc, but it's BSD licensed

I considered that, but it's another dependency (which, by the way, doesn't have cmake). Our current stack traces work correctly except for extracting the symbols for some gcc versions. I remedy that by calling addr2line or atos depending on the platform. I don't see a strong advantage to using an external library compared to the existing simpler solution.

@PhilMiller
Copy link
Member

In a process failure setting, especially, calling popen seems super sketchy. That's a fork() and exec(), per frame, when we may have already blown up memory.

Given the license, libbacktrace could be vendored into our source tree as a stand-alone piece.

There's also boost stacktrace, which again, could be imported into our tree if we didn't want an external dependency.

@lifflander
Copy link
Collaborator Author

In a process failure setting, especially, calling popen seems super sketchy. That's a fork() and exec(), per frame, when we may have already blown up memory.

Given the license, libbacktrace could be vendored into our source tree as a stand-alone piece.

There's also boost stacktrace, which again, could be imported into our tree if we didn't want an external dependency.

Yes, I'm familiar. There's a proposal to put stack traces in the standard that follows the boost implementation:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0881r5.html

@PhilMiller
Copy link
Member

Plus, with an external binary, there's no guarantee that its presence in the build environment means it's available in the execution environment.

@lifflander
Copy link
Collaborator Author

Plus, with an external binary, there's no guarantee that its presence in the build environment means it's available in the execution environment.

Another option would be to put addr2line in lib. It's a simple utility with about 400 lines of code. Do you think that libbacktrace compiles everywhere we need it?

@PhilMiller
Copy link
Member

addr2line is GPL licensed as part of GNU binutils, and depends on libbfd, which is also part of the GPL licensed GNU binutils

@PhilMiller
Copy link
Member

Do you think that libbacktrace compiles everywhere we need it?

'Everywhere we need it' is relatively modern Linux and macOS? It has ELF and Mach-O support, so we should be good.

@PhilMiller
Copy link
Member

For a separate concern with anything using popen of an external binary, how do MPI, Kokkos (including OpenMP usage as well), and CUDA feel about fork() calls?

@lifflander
Copy link
Collaborator Author

lifflander commented Jul 8, 2020

Here is a project that adds cmake support for libbacktrace with an older version. We should use it's cmake with the latest libbacktrace code:
https://github.com/ErwanLegrand/libbacktrace

There is also a PR open in the base libbacktrace for adding cmake support, but it is incomplete and it refers to this other fork:

ianlancetaylor/libbacktrace#19

edit: actually someone tried to use it with the newer libbacktrace code and was more trouble than it was worth

@lifflander
Copy link
Collaborator Author

The boost stacktrace library relies on a lot of boostisms and macros and might be hard to extract. It also doesn't have cmake:

https://github.com/boostorg/stacktrace

@lifflander
Copy link
Collaborator Author

The other thing to note is that most these libraries depend on a low-level system function: _Unwind_Backtrace (and _Unwind_GetIP in some cases). Boost stack trace is really just a very pretty wrapper.

@lifflander
Copy link
Collaborator Author

Rust supports backtraces and has recently moved away from libbacktrace due to several portability and maintainability issues outlined here: rust-lang/rust#73441

Their new solution is rolled from the ground up and it written in rust (not super helpful): https://github.com/gimli-rs

@lifflander lifflander marked this pull request as draft July 8, 2020 20:02
@lifflander
Copy link
Collaborator Author

@PhilMiller Actually, I might have just found a really good alternative with cmake support: https://github.com/bombela/backward-cpp

@PhilMiller
Copy link
Member

Here is a project that adds cmake support for libbacktrace with an older version. We should use it's cmake with the latest libbacktrace code:
https://github.com/ErwanLegrand/libbacktrace

There is also a PR open in the base libbacktrace for adding cmake support, but it is incomplete and it refers to this other fork:

ianlancetaylor/libbacktrace#19

edit: actually someone tried to use it with the newer libbacktrace code and was more trouble than it was worth

Ian Lance Taylor is the upstream maintainer of libbacktrace. I suspect that if you gave that PR a nudge, possibly with suitable testing report, that it would move ahead.

@lifflander
Copy link
Collaborator Author

Here is a project that adds cmake support for libbacktrace with an older version. We should use it's cmake with the latest libbacktrace code:
https://github.com/ErwanLegrand/libbacktrace
There is also a PR open in the base libbacktrace for adding cmake support, but it is incomplete and it refers to this other fork:
ianlancetaylor/libbacktrace#19
edit: actually someone tried to use it with the newer libbacktrace code and was more trouble than it was worth

Ian Lance Taylor is the upstream maintainer of libbacktrace. I suspect that if you gave that PR a nudge, possibly with suitable testing report, that it would move ahead.

I'm now thinking that https://github.com/bombela/backward-cpp might be the best library to embed. It's more modern, handles reading dwarf, has wide support across many platforms, and an MIT license.

@PhilMiller
Copy link
Member

Backward depends on either libbfd or libdw from elfutils. The former is GPL, the latter is dual GPL/LGPL. Either way, it's doing the pretty-printing, not the actual DWARF-reading.

@lifflander
Copy link
Collaborator Author

Backward depends on either libbfd or libdw from elfutils. The former is GPL, the latter is dual GPL/LGPL. Either way, it's doing the pretty-printing, not the actual DWARF-reading.

We don't need to include those in the source, right? It makes the call to those libraries to read the symbols (that's what I meant): load_object_with_dwarf.

I'm thinking we embed backward; we will only need those libs for the pretty backtrace.

You will need to install some dependencies to get the ultimate stack trace. Two libraries are currently supported, the only difference is which one is the easiest for you to install, so pick your poison:

@PhilMiller
Copy link
Member

Re -rdynamic: https://stackoverflow.com/a/8624223/90002 See point number 4 at the bottom about static linking

@PhilMiller
Copy link
Member

How about libunwind:
https://www.nongnu.org/libunwind/man/libunwind(3).html
https://github.com/libunwind/libunwind
MIT licensed, seemingly standalone

@PhilMiller
Copy link
Member

How about libunwind:
https://www.nongnu.org/libunwind/man/libunwind(3).html
https://github.com/libunwind/libunwind
MIT licensed, seemingly standalone

Looks like it will similarly be limited in symbol naming by lack of .dynsym as the general backtrace()

@lifflander lifflander force-pushed the 914-fix-stack-trace branch 2 times, most recently from b823eac to 675630f Compare July 10, 2020 16:47
@lifflander lifflander marked this pull request as ready for review July 10, 2020 16:47
@lifflander
Copy link
Collaborator Author

@PhilMiller I have -rdynamic working now and the stack traces are printing correctly on gcc.

@PhilMiller
Copy link
Member

Even with fully static builds? That's great news.

@PhilMiller
Copy link
Member

I'm +1 on passing the -rdynamic flag as described, but can't at all knowledgeably comment on the CMake aspects of how you're doing this. Probably good to get someone who knows it better to look at that.

@lifflander
Copy link
Collaborator Author

@nlslatt Can you look at the cmake here?

Copy link
Member

@PhilMiller PhilMiller left a comment

Choose a reason for hiding this comment

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

OK, CMake's policy and variable scoping rules are really weird, but they make good sense for the use case.

For anyone else following along:

The include(CheckLinkerFlag) pushes a fresh frame on the 'policy stack', and the end of the file pops that frame. Thus, the cmake_policy command only applies inside that file.

The function command captures the top of the policy stack, and all invocations of the function are evaluated with that policy set in place.

Files input and calls to functions each create their own variable scope. So, the set only applies within the function.

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

Successfully merging this pull request may close these issues.

Fix stack traces with gcc
4 participants