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

remove multiple debug functions for producing backtraces. #13469

Merged
merged 5 commits into from
Feb 2, 2021

Conversation

jsteemann
Copy link
Contributor

@jsteemann jsteemann commented Feb 2, 2021

Scope & Purpose

Remove multiple debug functions for producing backtraces.

Instead, make CrashHandler::logBacktrace() available to write a
backtrace of the current thread into the log.
Calling this function should work on Linux, because there we build with
libunwind support by default, and this can produce backtraces even for
our static builds using libmusl (in contrast to backtrace(), which is
a no-op there). In addition, using CrashHandler::logBacktrace() does
not require to compile with the -DUSE_BACKTRACE=On CMake option.

It is likely that the change made by this PR removes backtrace
capabilities for MacOS and Windows, but it looks like those were only
enabled previously when compiling with -DUSE_BACKTRACE=On, which is
unlikely.

In addition, this PR removes the USE_BACKTRACE CMake variable and also
the internal ARANGODB_ENABLE_BACKTRACE define that was enabled by this
variable.

The USE_BACKTRACE CMake option does not seem to have been documented.
So this is likely an internal change only.

  • 💩 Bugfix (requires CHANGELOG entry)
  • 🍕 New feature (requires CHANGELOG entry, feature documentation and release notes)
  • 🔥 Performance improvement
  • 🔨 Refactoring/simplification
  • 📖 CHANGELOG entry made

Backports:

  • No backports required

Testing & Verification

  • This change is a trivial rework / code cleanup without any test coverage.
  • The behavior in this PR was manually tested

Link to Jenkins PR run:
http://172.16.10.101:8080/view/PR/job/arangodb-matrix-pr/13816/

Instead, make CrashHandler::logBacktrace() available to write a
backtrace of the current thread into the log.
Calling this function should work on Linux, because there we build with
libunwind support by default, and this can produce backtraces even for
our static builds using libmusl (in contrast to `backtrace()`, which is
a no-op there). In addition, using `CrashHandler::logBacktrace()` does
not require to compile with the `-DUSE_BACKTRACE=On` CMake option.

It is likely that the change made by this PR removes backtrace
capabilities for MacOS and Windows, but it looks like those were only
enabled previously when compiling with `-DUSE_BACKTRACE=On`, which is
unlikely.

To finalize this PR, we need to decide whether these potential
restrictions are ok for us, and if so, we can also remove the
`USE_BACKTRACE` CMake variable and remove the one or two remainders that
use the `ARNAGODB_ENABLE_BACKTRACE` define in the code.
@jsteemann jsteemann added this to the devel milestone Feb 2, 2021
@jsteemann jsteemann requested a review from dothebart February 2, 2021 02:32
Copy link
Contributor

@dothebart dothebart left a comment

Choose a reason for hiding this comment

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

please add stack trace logging to invalid handle value - without its completely useless.
Else LGTM.

@jsteemann
Copy link
Contributor Author

Tests blue

@jsteemann
Copy link
Contributor Author

As discussed with @mpoeter and @dothebart offline, we will not add Windows stacktrace logging, but instead we will try to leverage minidumps in case an unhandled exception flies by.

@jsteemann
Copy link
Contributor Author

@jsteemann
Copy link
Contributor Author

@jsteemann jsteemann merged commit 1e8918f into devel Feb 2, 2021
@jsteemann jsteemann deleted the feature/simplify-backtrace-logging branch February 2, 2021 15:24
elfringham pushed a commit to elfringham/arangodb that referenced this pull request Apr 20, 2021
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.

2 participants