Skip to content

Commit

Permalink
Reland "Use thread_local: base/task/"
Browse files Browse the repository at this point in the history 
This also marks some scopers as [[maybe_unused, no_discard]] to try to
get better compiler warnings around their (mis)use.

Now with workarounds for MSAN false positives.

Bug: 1416710
Change-Id: I541a0976f458f07734fbc0c98516fe21b26b6fc4
Cq-Include-Trybots: luci.chromium.try:linux_chromium_chromeos_msan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335693
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Carlos Caballero Grolimund <carlscab@google.com>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1117866}
  • Loading branch information
@pkasting
pkasting authored and Chromium LUCI CQ committed Mar 16, 2023
1 parent 22c43aa commit 86c1a45
Show file tree
Hide file tree
Showing 17 changed files with 271 additions and 291 deletions.
19 changes: 11 additions & 8 deletions base/task/common/scoped_defer_task_posting.cc
View file
Expand Up @@ -4,19 +4,17 @@

#include "base/task/common/scoped_defer_task_posting.h"

#include "base/no_destructor.h"
#include "base/threading/thread_local.h"
#include "base/compiler_specific.h"
#include "third_party/abseil-cpp/absl/base/attributes.h"

namespace base {

namespace {

// Holds a thread-local pointer to the current scope or null when no
// scope is active.
ThreadLocalPointer<ScopedDeferTaskPosting>& GetScopedDeferTaskPostingTLS() {
static NoDestructor<ThreadLocalPointer<ScopedDeferTaskPosting>> tls;
return *tls;
}
ABSL_CONST_INIT thread_local ScopedDeferTaskPosting* scoped_defer_task_posting =
nullptr;

} // namespace

Expand All @@ -38,7 +36,12 @@ void ScopedDeferTaskPosting::PostOrDefer(

// static
ScopedDeferTaskPosting* ScopedDeferTaskPosting::Get() {
return GetScopedDeferTaskPostingTLS().Get();
// Workaround false-positive MSAN use-of-uninitialized-value on
// thread_local storage for loaded libraries:
// https://github.com/google/sanitizers/issues/1265
MSAN_UNPOISON(&scoped_defer_task_posting, sizeof(ScopedDeferTaskPosting*));

return scoped_defer_task_posting;
}

// static
Expand All @@ -47,7 +50,7 @@ bool ScopedDeferTaskPosting::Set(ScopedDeferTaskPosting* scope) {
// get nested scopes. In this case ignore all except the top one.
if (Get() && scope)
return false;
GetScopedDeferTaskPostingTLS().Set(scope);
scoped_defer_task_posting = scope;
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion base/task/common/scoped_defer_task_posting.h
View file
Expand Up @@ -23,7 +23,7 @@ namespace base {
// TODO(altimin): It should be possible to get rid of this scope, but this
// requires refactoring TimeDomain to ensure that TimeDomain never changes and
// we can read current time without grabbing a lock.
class BASE_EXPORT ScopedDeferTaskPosting {
class BASE_EXPORT [[maybe_unused, nodiscard]] ScopedDeferTaskPosting {
public:
static void PostOrDefer(scoped_refptr<SequencedTaskRunner> task_runner,
const Location& from_here,
Expand Down
175 changes: 86 additions & 89 deletions base/task/common/task_annotator.cc
View file
Expand Up @@ -8,17 +8,18 @@
#include <algorithm>
#include <array>

#include "base/auto_reset.h"
#include "base/check_op.h"
#include "base/compiler_specific.h"
#include "base/debug/alias.h"
#include "base/hash/md5.h"
#include "base/logging.h"
#include "base/no_destructor.h"
#include "base/ranges/algorithm.h"
#include "base/sys_byteorder.h"
#include "base/threading/thread_local.h"
#include "base/trace_event/base_tracing.h"
#include "base/tracing_buildflags.h"
#include "build/build_config.h"
#include "third_party/abseil-cpp/absl/base/attributes.h"

#if BUILDFLAG(ENABLE_BASE_TRACING)
#include "third_party/perfetto/protos/perfetto/trace/track_event/chrome_mojo_event_info.pbzero.h" // nogncheck
Expand All @@ -30,57 +31,71 @@ namespace {

TaskAnnotator::ObserverForTesting* g_task_annotator_observer = nullptr;

// Returns the TLS slot that stores the PendingTask currently in progress on
// each thread. Used to allow creating a breadcrumb of program counters on the
// stack to help identify a task's origin in crashes.
ThreadLocalPointer<PendingTask>* GetTLSForCurrentPendingTask() {
static NoDestructor<ThreadLocalPointer<PendingTask>> instance;
return instance.get();
// The PendingTask currently in progress on each thread. Used to allow creating
// a breadcrumb of program counters on the stack to help identify a task's
// origin in crashes.
ABSL_CONST_INIT thread_local PendingTask* current_pending_task = nullptr;

// Scoped IPC-related data (IPC hash and/or IPC interface name). IPC hash or
// interface name can be known before the associated task object is created;
// thread-local so that this data can be affixed to the associated task.
ABSL_CONST_INIT thread_local TaskAnnotator::ScopedSetIpcHash*
current_scoped_ipc_hash = nullptr;

ABSL_CONST_INIT thread_local TaskAnnotator::LongTaskTracker*
current_long_task_tracker = nullptr;

// These functions can be removed, and the calls below replaced with direct
// variable accesses, once the MSAN workaround is not necessary.
TaskAnnotator::ScopedSetIpcHash* GetCurrentScopedIpcHash() {
// Workaround false-positive MSAN use-of-uninitialized-value on
// thread_local storage for loaded libraries:
// https://github.com/google/sanitizers/issues/1265
MSAN_UNPOISON(&current_scoped_ipc_hash,
sizeof(TaskAnnotator::ScopedSetIpcHash*));

return current_scoped_ipc_hash;
}

// Returns the TLS slot that stores scoped IPC-related data (IPC hash and/or
// IPC interface name). IPC hash or interface name can be known before the
// associated task object is created; store in the TLS so that this data can be
// affixed to the associated task.
ThreadLocalPointer<TaskAnnotator::ScopedSetIpcHash>*
GetTLSForCurrentScopedIpcHash() {
static NoDestructor<ThreadLocalPointer<TaskAnnotator::ScopedSetIpcHash>>
instance;
return instance.get();
}
TaskAnnotator::LongTaskTracker* GetCurrentLongTaskTracker() {
// Workaround false-positive MSAN use-of-uninitialized-value on
// thread_local storage for loaded libraries:
// https://github.com/google/sanitizers/issues/1265
MSAN_UNPOISON(&current_long_task_tracker,
sizeof(TaskAnnotator::LongTaskTracker*));

ThreadLocalPointer<TaskAnnotator::LongTaskTracker>*
GetTLSForCurrentLongTaskTracker() {
static NoDestructor<ThreadLocalPointer<TaskAnnotator::LongTaskTracker>>
instance;
return instance.get();
return current_long_task_tracker;
}

} // namespace

const PendingTask* TaskAnnotator::CurrentTaskForThread() {
return GetTLSForCurrentPendingTask()->Get();
// Workaround false-positive MSAN use-of-uninitialized-value on
// thread_local storage for loaded libraries:
// https://github.com/google/sanitizers/issues/1265
MSAN_UNPOISON(&current_pending_task, sizeof(PendingTask*));

return current_pending_task;
}

void TaskAnnotator::OnIPCReceived(const char* interface_name,
uint32_t (*method_info)(),
bool is_response) {
base::TaskAnnotator::LongTaskTracker* current_long_task_tracker =
GetTLSForCurrentLongTaskTracker()->Get();

if (!current_long_task_tracker)
auto* const tracker = GetCurrentLongTaskTracker();
if (!tracker) {
return;
}

current_long_task_tracker->SetIpcDetails(interface_name, method_info,
is_response);
tracker->SetIpcDetails(interface_name, method_info, is_response);
}

void TaskAnnotator::MarkCurrentTaskAsInterestingForTracing() {
auto* current_task = GetTLSForCurrentLongTaskTracker()->Get();
if (!current_task)
auto* const tracker = GetCurrentLongTaskTracker();
if (!tracker) {
return;
}

current_task->is_interesting_task = true;
tracker->is_interesting_task = true;
}

TaskAnnotator::TaskAnnotator() = default;
Expand All @@ -100,10 +115,10 @@ void TaskAnnotator::WillQueueTask(perfetto::StaticString trace_event_name,

DCHECK(!pending_task->ipc_interface_name);
DCHECK(!pending_task->ipc_hash);
auto* current_ipc_hash = GetTLSForCurrentScopedIpcHash()->Get();
if (current_ipc_hash) {
pending_task->ipc_interface_name = current_ipc_hash->GetIpcInterfaceName();
pending_task->ipc_hash = current_ipc_hash->GetIpcHash();
const auto* const hash = GetCurrentScopedIpcHash();
if (hash) {
pending_task->ipc_interface_name = hash->GetIpcInterfaceName();
pending_task->ipc_hash = hash->GetIpcHash();
}

const auto* parent_task = CurrentTaskForThread();
Expand Down Expand Up @@ -153,35 +168,35 @@ void TaskAnnotator::RunTaskImpl(PendingTask& pending_task) {
reinterpret_cast<void*>(pending_task.ipc_hash);
debug::Alias(&task_backtrace);

auto* tls = GetTLSForCurrentPendingTask();
auto* previous_pending_task = tls->Get();
tls->Set(&pending_task);
{
const AutoReset<PendingTask*> resetter(&current_pending_task,
&pending_task);

if (g_task_annotator_observer)
g_task_annotator_observer->BeforeRunTask(&pending_task);
std::move(pending_task.task).Run();
if (g_task_annotator_observer) {
g_task_annotator_observer->BeforeRunTask(&pending_task);
}
std::move(pending_task.task).Run();
#if BUILDFLAG(IS_WIN) && defined(ARCH_CPU_X86_FAMILY)
// Some tasks on some machines clobber the non-volatile XMM registers in
// violation of the Windows ABI. This empty assembly language block with
// clobber directives tells the compiler to assume that these registers
// may have lost their values. This ensures that this function will not rely
// on the registers retaining their values, and it ensures that it will
// restore the values when this function ends. This is needed because the
// code-gen for at least one caller of this function in official builds relies
// on an XMM register (usually XMM7, cleared to zero) maintaining its value as
// multiple tasks are run, which causes crashes if it is corrupted, since
// "zeroed" variables end up not being zeroed.
// The third-party issue is believed to be fixed but will take a while to
// propagate to users which is why this mitigation is needed.
// For details see https://crbug.com/1218384
asm(""
:
:
: "%xmm6", "%xmm7", "%xmm8", "%xmm9", "%xmm10", "%xmm11", "%xmm12",
"%xmm13", "%xmm14", "%xmm15");
// Some tasks on some machines clobber the non-volatile XMM registers in
// violation of the Windows ABI. This empty assembly language block with
// clobber directives tells the compiler to assume that these registers
// may have lost their values. This ensures that this function will not rely
// on the registers retaining their values, and it ensures that it will
// restore the values when this function ends. This is needed because the
// code-gen for at least one caller of this function in official builds
// relies on an XMM register (usually XMM7, cleared to zero) maintaining its
// value as multiple tasks are run, which causes crashes if it is corrupted,
// since "zeroed" variables end up not being zeroed. The third-party issue
// is believed to be fixed but will take a while to propagate to users which
// is why this mitigation is needed. For details see
// https://crbug.com/1218384.
asm(""
:
:
: "%xmm6", "%xmm7", "%xmm8", "%xmm9", "%xmm10", "%xmm11", "%xmm12",
"%xmm13", "%xmm14", "%xmm15");
#endif

tls->Set(previous_pending_task);
}

// Stomp the markers. Otherwise they can stick around on the unused parts of
// stack and cause |task_backtrace| to be associated with an unrelated stack
Expand Down Expand Up @@ -258,14 +273,10 @@ TaskAnnotator::ScopedSetIpcHash::ScopedSetIpcHash(

TaskAnnotator::ScopedSetIpcHash::ScopedSetIpcHash(
uint32_t ipc_hash,
const char* ipc_interface_name) {
auto* tls_ipc_hash = GetTLSForCurrentScopedIpcHash();
auto* current_ipc_hash = tls_ipc_hash->Get();
old_scoped_ipc_hash_ = current_ipc_hash;
ipc_hash_ = ipc_hash;
ipc_interface_name_ = ipc_interface_name;
tls_ipc_hash->Set(this);
}
const char* ipc_interface_name)
: resetter_(&current_scoped_ipc_hash, this),
ipc_hash_(ipc_hash),
ipc_interface_name_(ipc_interface_name) {}

// Static
uint32_t TaskAnnotator::ScopedSetIpcHash::MD5HashMetricName(
Expand All @@ -279,32 +290,24 @@ uint32_t TaskAnnotator::ScopedSetIpcHash::MD5HashMetricName(
}

TaskAnnotator::ScopedSetIpcHash::~ScopedSetIpcHash() {
auto* tls_ipc_hash = GetTLSForCurrentScopedIpcHash();
DCHECK_EQ(this, tls_ipc_hash->Get());
tls_ipc_hash->Set(old_scoped_ipc_hash_.get());
DCHECK_EQ(this, GetCurrentScopedIpcHash());
}

TaskAnnotator::LongTaskTracker::LongTaskTracker(const TickClock* tick_clock,
PendingTask& pending_task,
TaskAnnotator* task_annotator)
: tick_clock_(tick_clock),
: resetter_(&current_long_task_tracker, this),
tick_clock_(tick_clock),
pending_task_(pending_task),
task_annotator_(task_annotator) {
auto* tls_long_task_tracker = GetTLSForCurrentLongTaskTracker();
old_long_task_tracker_ = tls_long_task_tracker->Get();

TRACE_EVENT_CATEGORY_GROUP_ENABLED("scheduler.long_tasks", &is_tracing_);
if (is_tracing_) {
task_start_time_ = tick_clock_->NowTicks();
}

tls_long_task_tracker->Set(this);
}

TaskAnnotator::LongTaskTracker::~LongTaskTracker() {
auto* tls_long_task_tracker = GetTLSForCurrentLongTaskTracker();
DCHECK_EQ(this, tls_long_task_tracker->Get());
tls_long_task_tracker->Set(old_long_task_tracker_.get());
DCHECK_EQ(this, GetCurrentLongTaskTracker());

if (!is_tracing_)
return;
Expand All @@ -323,12 +326,6 @@ TaskAnnotator::LongTaskTracker::~LongTaskTracker() {
perfetto::Track::ThreadScoped(task_annotator_),
task_end_time_);
}
#if !BUILDFLAG(ENABLE_BASE_TRACING)
// Suppress the unused variable warning when TRACE_EVENT macros are turned
// into no-op.
(void)pending_task_;
(void)task_annotator_;
#endif // !BUILDFLAG(ENABLE_BASE_TRACING)
}

void TaskAnnotator::LongTaskTracker::SetIpcDetails(const char* interface_name,
Expand Down
19 changes: 11 additions & 8 deletions base/task/common/task_annotator.h
View file
Expand Up @@ -7,6 +7,7 @@

#include <stdint.h>

#include "base/auto_reset.h"
#include "base/base_export.h"
#include "base/memory/raw_ptr.h"
#include "base/memory/raw_ref.h"
Expand Down Expand Up @@ -116,7 +117,7 @@ class BASE_EXPORT TaskAnnotator {
#endif // BUILDFLAG(ENABLE_BASE_TRACING)
};

class BASE_EXPORT TaskAnnotator::ScopedSetIpcHash {
class BASE_EXPORT [[maybe_unused, nodiscard]] TaskAnnotator::ScopedSetIpcHash {
public:
explicit ScopedSetIpcHash(uint32_t ipc_hash);

Expand All @@ -136,12 +137,13 @@ class BASE_EXPORT TaskAnnotator::ScopedSetIpcHash {

private:
ScopedSetIpcHash(uint32_t ipc_hash, const char* ipc_interface_name);
raw_ptr<ScopedSetIpcHash> old_scoped_ipc_hash_ = nullptr;
uint32_t ipc_hash_ = 0;
const char* ipc_interface_name_ = nullptr;

const AutoReset<ScopedSetIpcHash*> resetter_;
uint32_t ipc_hash_;
const char* ipc_interface_name_;
};

class BASE_EXPORT TaskAnnotator::LongTaskTracker {
class BASE_EXPORT [[maybe_unused, nodiscard]] TaskAnnotator::LongTaskTracker {
public:
explicit LongTaskTracker(const TickClock* tick_clock,
PendingTask& pending_task,
Expand All @@ -166,6 +168,8 @@ class BASE_EXPORT TaskAnnotator::LongTaskTracker {
private:
void EmitReceivedIPCDetails(perfetto::EventContext& ctx);

const AutoReset<LongTaskTracker*> resetter_;

// For tracking task duration
raw_ptr<const TickClock> tick_clock_; // Not owned.
TimeTicks task_start_time_;
Expand All @@ -176,16 +180,15 @@ class BASE_EXPORT TaskAnnotator::LongTaskTracker {
// Use this to ensure that tracing and NowTicks() are not called
// unnecessarily.
bool is_tracing_;
raw_ptr<LongTaskTracker> old_long_task_tracker_ = nullptr;
const char* ipc_interface_name_ = nullptr;
uint32_t ipc_hash_ = 0;

// IPC method info to retrieve IPC hash and method address from trace, if
// known. Note that this will not compile in the Native client.
uint32_t (*ipc_method_info_)();
bool is_response_ = false;
const raw_ref<PendingTask> pending_task_;
raw_ptr<TaskAnnotator> task_annotator_;
[[maybe_unused]] const raw_ref<PendingTask> pending_task_;
[[maybe_unused]] raw_ptr<TaskAnnotator> task_annotator_;
};

} // namespace base
Expand Down

0 comments on commit 86c1a45

Please sign in to comment.