tracing v2: minor refactoring to TraceRingBuffer test helpers

components/tracing/core/trace_ring_buffer.cc

Index: components/tracing/core/trace_ring_buffer.cc
diff --git a/components/tracing/core/trace_ring_buffer.cc b/components/tracing/core/trace_ring_buffer.cc
index 2a52bf61db1b64350f7d3e20a7f5acb4da63b2c1..8c5e5a4c391fc0a786af7cc6dc3b3b0b963e8731 100644
--- a/components/tracing/core/trace_ring_buffer.cc
+++ b/components/tracing/core/trace_ring_buffer.cc
@@ -10,7 +10,9 @@ namespace tracing {
 namespace v2 {
 
 TraceRingBuffer::TraceRingBuffer(uint8_t* begin, size_t size)
-    : num_chunks_(size / Chunk::kSize), current_chunk_idx_(0) {
+    : num_chunks_(size / Chunk::kSize),
+      num_chunks_taken_(0),
+      current_chunk_idx_(0) {
   DCHECK_GT(num_chunks_, 0u);
   DCHECK_EQ(0ul, reinterpret_cast<uintptr_t>(begin) % sizeof(uintptr_t));
   chunks_.reset(new Chunk[num_chunks_]);
@@ -23,7 +25,7 @@ TraceRingBuffer::TraceRingBuffer(uint8_t* begin, size_t size)
 
 TraceRingBuffer::~TraceRingBuffer() {}
 
-TraceRingBuffer::Chunk* TraceRingBuffer::TakeChunk() {
+TraceRingBuffer::Chunk* TraceRingBuffer::TakeChunk(uint32_t writer_id) {
   base::AutoLock lock(lock_);
   DCHECK_GT(num_chunks_, 0ul);
   DCHECK_LT(current_chunk_idx_, num_chunks_);
@@ -32,7 +34,9 @@ TraceRingBuffer::Chunk* TraceRingBuffer::TakeChunk() {
     current_chunk_idx_ = (current_chunk_idx_ + 1) % num_chunks_;
     if (!chunk->is_owned()) {
       chunk->Clear();
-      chunk->set_owner(base::PlatformThread::CurrentId());
+      DCHECK_NE(0u, writer_id);
+      chunk->set_owner(writer_id);
+      num_chunks_taken_++;
       return chunk;
     }
   }
@@ -46,20 +50,41 @@ TraceRingBuffer::Chunk* TraceRingBuffer::TakeChunk() {
   return &bankrupcy_chunk_;
 }
 
-void TraceRingBuffer::ReturnChunk(TraceRingBuffer::Chunk* chunk,
-                                  uint32_t used_size) {
+void TraceRingBuffer::ReturnChunk(TraceRingBuffer::Chunk* chunk) {
+  // Returning a chunk without using it is quite odd, very likely a bug.
+  DCHECK_GT(chunk->used_size(), 0u);
+
+  // The caller should never return chunks which are part of a retaining chain.
+  DCHECK(!chunk->next_in_owner_list());
+
+  if (chunk == &bankrupcy_chunk_)
+    return;
+
+  // TODO(primiano): this lock might be removed in favor of acquire/release
+  // semantics on the two vars below. Check once the reader of these are landed.
   base::AutoLock lock(lock_);
-  chunk->set_used_size(used_size);
   chunk->clear_owner();
+  num_chunks_taken_--;
+}
+
+bool TraceRingBuffer::IsBankrupcyChunkForTesting(const Chunk* chunk) const {
+  return chunk->begin() >= chunks_[num_chunks_ - 1].end() ||

[alph, 2016/08/04 19:27:35]

I'm not sure how it will break ;-) but C standard says that comparing two
pointers from distinct objects is an UB.

[Primiano Tucci (use gerrit), 2016/08/05 11:24:14]

"If two pointers p and q of the same type point to different objects that are
not members of the same object or elements of the same array or to different
functions, or if only one of them is null, the results of p<q, p>q, p<=q, and
p>=q are unspecified."

So my reading is that what they are saying here is: if you have
function whatever() {
   int a[4];
   int b[4];
  assert(&a[2] < &a[3]) is defined behavior.
  assert(&a[1] < &b[1]) is UB
}

In other words, you should not rely on what is the stack layout of two variables
(or similarly if they are heap allocated). The only think you can rely on is
arrays and field of a struct/class, as those are defined. 
Here instead my case is:   start() and end() of a chunk contain the address of:
A. either a portion of the a big contiguous array (for the real chunks).
B. a disjoint array (for the bankrupcy chunk).

Now, the UB here would be if I rely on the fact that A is < or > B, because I
can't possibly know where any of them would be allocated relative to each other.
But here I'm just checking whether they overlap or not, which I believe is
defined.

Having said this, reading this code made me realize that my implementation here
is totally moronic.
I just need to check whether chunk == &bankrupcy_chunk_ and that's all :)
I moved this code from the unittest (which did not have access to the TRB
internals) and I didn't realize that now I do :)

+         chunk->end() <= chunks_[0].begin();
+}
+
+size_t TraceRingBuffer::GetNumChunksTaken() const {
+  base::AutoLock lock(lock_);
+  return num_chunks_taken_;
 }
 
 TraceRingBuffer::Chunk::Chunk()
-    : begin_(nullptr), owner_(base::kInvalidThreadId) {}
+    : begin_(nullptr), owner_(kNoChunkOwner), next_in_owner_list_(nullptr) {}
 
 TraceRingBuffer::Chunk::~Chunk() {}
 
 void TraceRingBuffer::Chunk::Clear() {
   set_used_size(0);
+  set_next_in_owner_list(nullptr);
 }
 
 void TraceRingBuffer::Chunk::Initialize(uint8_t* begin) {