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

[FR]: std::stacktrace support #4336

Open
strager opened this issue Aug 10, 2023 · 5 comments
Open

[FR]: std::stacktrace support #4336

strager opened this issue Aug 10, 2023 · 5 comments
Assignees

Comments

@strager
Copy link

strager commented Aug 10, 2023

Does the feature exist in the most recent commit?

no

Why do we need this feature?

I want to get pretty stack traces in my tests without depending on Abseil.

In addition, Microsoft STL's implementation of std::stacktrace provides more information than Abseil, such as file and line numbers.

I think this feature would be generally useful, thus I think this feature should be built into googletest instead of users leveraging the (currently broken #1020) OsStackTraceGetterInterface extension point.

Describe the proposal.

Approach A:
Change the existing OsStackTraceGetter class to use std::stacktrace if available, falling back to Absel or a no-op implementation if std::stacktrace unavailable.

Problem with approach A:
This would involve adding #ifs in header files which might lead to ODR problems.

Approach B:

  1. Split OsStackTraceGetter into two classes: AbslStackTraceGetter (which uses Abseil; conditionally defined) and NoopStackTraceGetter (always defined).
  2. Add StdStackTraceGetter (which uses std::stacktrace; conditionally defined).
  3. Make UnitTestImpl::os_stack_trace_getter choos StdStackTraceGetter if available, falling back to AbslStackTraceGetter or NoopStackTraceGetter if std::stacktrace is unavailable.

(This seems to be how OsStackTraceGetterInterface was meant to be used.)

Is the feature specific to an operating system, compiler, or build system version?

Currently, std::stacktrace is available on recent MSVC versions for Windows. It might be available with effort on GCC/libstdc++. libc++ does not have an implementation.

Boost has a somewhat-portable implementation which looks similar to std::stacktrace. I don't propose using Boost's version.

@strager
Copy link
Author

strager commented Aug 10, 2023

I am willing to implement this feature myself. I prefer approach B. As an alternative, we could make OsStackTraceGetterInterface a public API and leave users to define their own std::stacktrace-using implementations (i.e. implement #1020).

@strager
Copy link
Author

strager commented Aug 10, 2023

I implemented approach B in #4337.

@derekmauro derekmauro self-assigned this Aug 16, 2023
@derekmauro
Copy link
Member

If I recall correctly there are two uses of stack tracing in GoogleTest. One of them is to print a stack trace when an expectation fails. The other is to print a stack trace when your test crashes (via the environment variable GTEST_INSTALL_FAILURE_SIGNAL_HANDLER=1).

std::stacktrace makes no guarantees about signal safety, while Abseil's is async signal safe, at least on Linux. The lack of signal safety is a problem for the crash handler, even if it happens to work some of the time.

We have an async signal safe implementation of stack tracing that also has line numbers, but we haven't open sourced it yet. I should also note that the async signal safe implementations are quite slow compared to alternatives, but that is the price we pay.

I'm sure it is possible to use an implementation based on std::stacktrace, but I'd have to spend some more time looking into what we currently have, and probably reorganize some code so that it doesn't interfere with Google-internal systems that make assumptions about the implementation.

All this is to say I'm sorry because big changes are quite difficult to make for an outsider since Google-internal systems need to be prioritized.

@strager
Copy link
Author

strager commented Aug 21, 2023

I did not know about the GTEST_INSTALL_FAILURE_SIGNAL_HANDLER feature. I don't see it in the main branch. Is it Google-only? (I do see a similar feature on Windows.)

However, I don't think that feature is relevant to this issue. The current OsStackTraceGetter implementation is not async-signal safe. CurrentStackTrace heap-allocates data and returns an std::string, which is not safe to do in signal handlers. Even if Absl's implementation is async-signal safe, Google Test's internal use of it is not.

If OsStackTraceGetterInterface is fixed to not require std::string, then implementations could be made async-signal safe. The Absl implementation could have bool IsAsyncSignalSafe() override { return true; }, and the std::stacktrace implementation could have bool IsAsyncSignalSafe() override { return false; }.

@derekmauro
Copy link
Member

Ah, I am misremembering a bit here. GTEST_INSTALL_FAILURE_SIGNAL_HANDLER does not use OsStackTraceGetter, it installs Abseil's signal handler. I will try to take a look at your PR soon.

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

No branches or pull requests

2 participants