Commit 8bf9fbd7 authored by Etienne Pierre-doray's avatar Etienne Pierre-doray Committed by Chromium LUCI CQ

[Clank SSM]: Make StackCopierSignal robust to TimeTicks failures.

Sampling profiler uses TimeTicks::Now in signal handler to get a
timestamp associated with the sample.
When called from signal handler, TimeTicks::Now occasionally crashes
the  following check:
CHECK(clock_gettime(clk_id, &ts) == 0)

In order to get good time precision in profiler, we'd like to get
a timestamp while the sampled thread is paused. Since failures are rare,
a solution is to allow clock_gettime to fail, and fallback to getting
an 'async' timestamp after resume.

To reuse some of the ConvertTimespecToMicros boilerplate common with
TimeTicks::Now, this CL exposes
subtle::MaybeTimeTicksNowIgnoringOverride() on posix, used in
stack_coper_signal.


Bug: 1155269
Change-Id: I4d8f5a3e050f5285b87297f13dfd29e888e40257
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2584603
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: default avatarGabriel Charette <gab@chromium.org>
Reviewed-by: default avatarMike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836298}
parent be3a1582
......@@ -15,6 +15,7 @@
#include "base/profiler/register_context.h"
#include "base/profiler/stack_buffer.h"
#include "base/profiler/suspendable_thread_delegate.h"
#include "base/time/time_override.h"
#include "base/trace_event/base_tracing.h"
#include "build/build_config.h"
......@@ -93,7 +94,7 @@ struct HandlerParams {
const uint8_t** stack_copy_bottom;
// The timestamp when the stack was copied.
TimeTicks* timestamp;
base::Optional<TimeTicks>* maybe_timestamp;
// The delegate provided to the StackCopier.
StackCopier::Delegate* stack_copier_delegate;
......@@ -110,9 +111,12 @@ std::atomic<HandlerParams*> g_handler_params;
void CopyStackSignalHandler(int n, siginfo_t* siginfo, void* sigcontext) {
HandlerParams* params = g_handler_params.load(std::memory_order_acquire);
// TimeTicks::Now() is implemented in terms of clock_gettime on Linux, which
// is signal safe per the signal-safety(7) man page.
*params->timestamp = TimeTicks::Now();
// MaybeTimeTicksNowIgnoringOverride() is implemented in terms of
// clock_gettime on Linux, which is signal safe per the signal-safety(7) man
// page, but is not garanteed to succeed, in which case base::nullopt is
// returned. TimeTicks::Now() can't be used because it expects clock_gettime
// to always succeed and is thus not signal-safe.
*params->maybe_timestamp = subtle::MaybeTimeTicksNowIgnoringOverride();
ScopedEventSignaller e(params->event);
*params->success = false;
......@@ -195,9 +199,10 @@ bool StackCopierSignal::CopyStack(StackBuffer* stack_buffer,
bool copied = false;
const uint8_t* stack_copy_bottom = nullptr;
const uintptr_t stack_base_address = thread_delegate_->GetStackBaseAddress();
base::Optional<TimeTicks> maybe_timestamp;
HandlerParams params = {stack_base_address, &wait_event, &copied,
thread_context, stack_buffer, &stack_copy_bottom,
timestamp, delegate};
&maybe_timestamp, delegate};
{
ScopedSetSignalHandlerParams scoped_handler_params(&params);
......@@ -228,6 +233,16 @@ bool StackCopierSignal::CopyStack(StackBuffer* stack_buffer,
NOTREACHED();
return false;
}
// Ideally, an accurate timestamp is captured while the sampled thread is
// paused. In rare cases, this may fail, in which case we resort to
// capturing an delayed timestamp here instead.
if (maybe_timestamp.has_value())
*timestamp = maybe_timestamp.value();
else {
TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cpu_profiler.debug"),
"Fallback on TimeTicks::Now()")
*timestamp = TimeTicks::Now();
}
}
const uintptr_t bottom = RegisterContextStackPointer(params.context);
......
......@@ -53,6 +53,15 @@ int64_t ClockNow(clockid_t clk_id) {
CHECK(clock_gettime(clk_id, &ts) == 0);
return ConvertTimespecToMicros(ts);
}
base::Optional<int64_t> MaybeClockNow(clockid_t clk_id) {
struct timespec ts;
int res = clock_gettime(clk_id, &ts);
if (res == 0)
return ConvertTimespecToMicros(ts);
return base::nullopt;
}
#else // _POSIX_MONOTONIC_CLOCK
#error No usable tick clock function on this platform.
#endif // _POSIX_MONOTONIC_CLOCK
......@@ -88,6 +97,13 @@ namespace subtle {
TimeTicks TimeTicksNowIgnoringOverride() {
return TimeTicks() + TimeDelta::FromMicroseconds(ClockNow(CLOCK_MONOTONIC));
}
base::Optional<TimeTicks> MaybeTimeTicksNowIgnoringOverride() {
base::Optional<int64_t> now = MaybeClockNow(CLOCK_MONOTONIC);
if (now.has_value())
return TimeTicks() + TimeDelta::FromMicroseconds(now.value());
return base::nullopt;
}
} // namespace subtle
// static
......
......@@ -7,7 +7,9 @@
#include "base/base_export.h"
#include "base/macros.h"
#include "base/optional.h"
#include "base/time/time.h"
#include "build/build_config.h"
namespace base {
......@@ -52,6 +54,12 @@ BASE_EXPORT Time TimeNowFromSystemTimeIgnoringOverride();
BASE_EXPORT TimeTicks TimeTicksNowIgnoringOverride();
BASE_EXPORT ThreadTicks ThreadTicksNowIgnoringOverride();
#if defined(OS_POSIX)
// Equivalent to TimeTicksNowIgnoringOverride(), but is allowed to fail and
// return base::nullopt. This may safely be used in a signal handler.
BASE_EXPORT base::Optional<TimeTicks> MaybeTimeTicksNowIgnoringOverride();
#endif
} // namespace subtle
namespace internal {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment