[PATCH] D48653: [XRay][compiler-rt] xray::Array Freelist and Iterator Updates

[Dean Michael Berris via Phabricator via llvm-commits]

dberris marked 9 inline comments as done.
dberris added a comment.

In https://reviews.llvm.org/D48653#1156953, @kpw wrote:

> Seems like half of this change is DCHECKS. Much better coverage! Was that just a result of your strategy for diagnosing what actually caused the problems and leaving those in?


Yep! Turns out that using assertions is a great way to... well... assert things. :)



================
Comment at: compiler-rt/lib/xray/tests/unit/segmented_array_test.cc:150
   for (auto i = ChunkX2; i > 0u; --i) {
-    data.Append({static_cast<s64>(i), static_cast<s64>(i)});
+    Data.Append({static_cast<s64>(i), static_cast<s64>(i)});
+  }
----------------
kpw wrote:
> Why Append here and AppendEmplace above?
No good reason. :) Fixed.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:153
       // that at once.
-      InternalFree(C->Blocks[0].Data);
+      InternalFree(C->BackingStore);
       auto Prev = C->Prev;
----------------
kpw wrote:
> So trim of segmented array was free-ing the "block" with the wrong allocator when trimming the first element?
Yeah, this one was hard to track. We were running into undefined behaviour in a lot of places given alignment issues with reinterpret_cast's.


================
Comment at: compiler-rt/lib/xray/xray_function_call_trie.h:367
       // space. Maybe warn or report once?
-      DFSStack.Append(NodeAndParent{Root, NewRoot});
+      DFSStack.AppendEmplace(Root, NewRoot);
       while (!DFSStack.empty()) {
----------------
kpw wrote:
> Should the NewRoots point back to the old roots as a parent? Don't we want to keep this isolated?
No, but what we're doing here is a parallel traversal of the tries -- we add the root from the original trie alongside the new root in the new new/destination trie. That way we can recreate the structure from the source trie into the new trie.


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:44
   static constexpr size_t ChunkSize = N;
-  static constexpr size_t AllocatorChunkSize = sizeof(T) * ChunkSize;
+  static constexpr size_t ElementStorageSize = next_pow2(sizeof(T));
+  static constexpr size_t AllocatorChunkSize = ElementStorageSize * ChunkSize;
----------------
kpw wrote:
> I assume this is all alignment related?
Yes, we'd like to have a power-of-two size for the structures, to allow easier pointer alignment calculations.


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:171
+      auto PreviousOffset = Offset--;
+      if (PreviousOffset != Size && PreviousOffset % N == 0) {
+        DCHECK_NE(C->Prev, &SentinelChunk);
----------------
kpw wrote:
> This size check is applicable when decrementing end()?
Yes -- we know that when `PreviousOffset == Size` and neither are 0, that the offset is now pointing to a valid object.


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:204-205
+      auto RelOff = Offset % N;
+      return *reinterpret_cast<U *>(reinterpret_cast<char *>(C->Block.Data) +
+                                    (RelOff * ElementStorageSize));
     }
----------------
kpw wrote:
> Lol. Checks out.
Yeah, ensuring correct alignment is hard. :P


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:303
+    auto It = end();
+    --It;
+    return *It;
----------------
kpw wrote:
> I would comment operator-- with the fact that it must not mutate the iterator. It's counterintuitive that it's non-destructive while ++ advances state.
operator-- actually does mutate the iterator. What we're doing here is being explicit about storing the state of the iterator into It first, mutating It, and then de-referencing it.


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:331
+    for (auto ChunksToTrim =
+             nearest_boundary(OldSize - nearest_boundary(Size, N), N) / N;
+         ChunksToTrim > 0; --ChunksToTrim) {
----------------
kpw wrote:
> This feels like it probably simplifies to an equivalent expression.
Actually turns out the math was slightly wrong, updated and fixed.


https://reviews.llvm.org/D48653