Skip to content

Commit

Permalink
Merge "shared_lib: Fix PerfettoDsImplReleaseInstanceLocked()" into main
Browse files Browse the repository at this point in the history
  • Loading branch information
ddiproietto authored and Gerrit Code Review committed Sep 13, 2024
2 parents a1e1392 + 88ce311 commit 1e5f937
Showing 5 changed files with 98 additions and 5 deletions.
11 changes: 8 additions & 3 deletions include/perfetto/tracing/internal/data_source_internal.h
Original file line number Diff line number Diff line change
@@ -152,11 +152,16 @@ struct DataSourceStaticState {
// this data source.
std::atomic<uint32_t> incremental_state_generation{};

// The caller must be sure that `n` was a valid instance at some point (either
// through a previous read of `valid_instances` or because the instance lock
// is held).
DataSourceState* GetUnsafe(size_t n) {
return reinterpret_cast<DataSourceState*>(&instances[n]);
}

// Can be used with a cached |valid_instances| bitmap.
DataSourceState* TryGetCached(uint32_t cached_bitmap, size_t n) {
return cached_bitmap & (1 << n)
? reinterpret_cast<DataSourceState*>(&instances[n])
: nullptr;
return cached_bitmap & (1 << n) ? GetUnsafe(n) : nullptr;
}

DataSourceState* TryGet(size_t n) {
6 changes: 4 additions & 2 deletions src/shared_lib/data_source.cc
Original file line number Diff line number Diff line change
@@ -401,8 +401,10 @@ void* PerfettoDsImplGetInstanceLocked(struct PerfettoDsImpl* ds_impl,

void PerfettoDsImplReleaseInstanceLocked(struct PerfettoDsImpl* ds_impl,
PerfettoDsInstanceIndex idx) {
auto* internal_state = ds_impl->cpp_type.static_state()->TryGet(idx);
PERFETTO_CHECK(internal_state);
// The `valid_instances` bitmap might have changed since the lock has been
// taken, but the instance must still be alive (we were holding the lock on
// it).
auto* internal_state = ds_impl->cpp_type.static_state()->GetUnsafe(idx);
internal_state->lock.unlock();
}

78 changes: 78 additions & 0 deletions src/shared_lib/test/api_integrationtest.cc
Original file line number Diff line number Diff line change
@@ -966,6 +966,84 @@ TEST_F(SharedLibDataSourceTest, IncrementalState) {
PERFETTO_DS_TRACE(data_source_1, ctx) {}
}

// Regression test for a `PerfettoDsImplReleaseInstanceLocked()`. Under very
// specific circumstances, that depends on the implementation details of
// `TracingMuxerImpl`, the following events can happen:
// * `PerfettoDsImplGetInstanceLocked()` is called after
// `TracingMuxerImpl::StopDataSource_AsyncBeginImpl`, but before
// `TracingMuxerImpl::StopDataSource_AsyncEnd`.
// `PerfettoDsImplGetInstanceLocked()` succeeds and returns a valid instance.
// * `TracingMuxerImpl::StopDataSource_AsyncEnd()` is called.
// `DataSourceStaticState::valid_instances` is reset.
// * `PerfettoDsImplReleaseInstanceLocked()` is called.
//
// In this case `PerfettoDsImplReleaseInstanceLocked()` should work even though
// the instance is not there in the valid_instances bitmap anymore.
//
// In order to reproduce the specific failure, the test makes assumptions about
// the internal implementation (that valid_instance is changed outside of the
// lock). If that were to change and the test would fail, the test should be
// changed/deleted.
TEST_F(SharedLibDataSourceTest, GetInstanceLockedStopBeforeRelease) {
bool ignored = false;
void* const kInstancePtr = &ignored;
EXPECT_CALL(ds2_callbacks_, OnSetup(_, _, _, _, kDataSource2UserArg, _))
.WillOnce(Return(kInstancePtr));
TracingSession tracing_session =
TracingSession::Builder().set_data_source_name(kDataSourceName2).Build();

WaitableEvent inside_tracing;
WaitableEvent stopping;
WaitableEvent locked;
WaitableEvent fully_stopped;

std::thread t([&] {
PERFETTO_DS_TRACE(data_source_2, ctx) {
inside_tracing.Notify();
stopping.WaitForNotification();
void* arg =
PerfettoDsImplGetInstanceLocked(data_source_2.impl, ctx.impl.inst_id);
EXPECT_EQ(arg, kInstancePtr);
locked.Notify();
fully_stopped.WaitForNotification();
if (arg) {
PerfettoDsImplReleaseInstanceLocked(data_source_2.impl,
ctx.impl.inst_id);
}
}
});

inside_tracing.WaitForNotification();

struct PerfettoDsAsyncStopper* stopper = nullptr;

EXPECT_CALL(ds2_callbacks_, OnStop(_, _, kDataSource2UserArg, _, _))
.WillOnce([&](struct PerfettoDsImpl*, PerfettoDsInstanceIndex, void*,
void*, struct PerfettoDsOnStopArgs* args) {
stopper = PerfettoDsOnStopArgsPostpone(args);
stopping.Notify();
});

tracing_session.StopAsync();

locked.WaitForNotification();
PerfettoDsStopDone(stopper);
// Wait for PerfettoDsImplTraceIterateBegin to return a nullptr tracer. This
// means that the valid_instances bitmap has been reset.
for (;;) {
PerfettoDsImplTracerIterator iterator =
PerfettoDsImplTraceIterateBegin(data_source_2.impl);
if (iterator.tracer == nullptr) {
break;
}
PerfettoDsImplTraceIterateBreak(data_source_2.impl, &iterator);
std::this_thread::yield();
}
fully_stopped.Notify();
tracing_session.WaitForStopped();
t.join();
}

class SharedLibProducerTest : public testing::Test {
protected:
void SetUp() override {
4 changes: 4 additions & 0 deletions src/shared_lib/test/utils.cc
Original file line number Diff line number Diff line change
@@ -159,6 +159,10 @@ void TracingSession::WaitForStopped() {
stopped_->WaitForNotification();
}

void TracingSession::StopAsync() {
PerfettoTracingSessionStopAsync(session_);
}

void TracingSession::StopBlocking() {
PerfettoTracingSessionStopBlocking(session_);
}
4 changes: 4 additions & 0 deletions src/shared_lib/test/utils.h
Original file line number Diff line number Diff line change
@@ -99,7 +99,11 @@ class TracingSession {
struct PerfettoTracingSessionImpl* session() const { return session_; }

bool FlushBlocking(uint32_t timeout_ms);
// Waits for the tracing session to be stopped.
void WaitForStopped();
// Asks the tracing session to stop. Doesn't wait for it to be stopped.
void StopAsync();
// Equivalent to StopAsync() + WaitForStopped().
void StopBlocking();
std::vector<uint8_t> ReadBlocking();

0 comments on commit 1e5f937

Please sign in to comment.