Thought that occurred to me this morning:

Suggestion for splitting this up.

Consider creating wrapper functions around DataExtractor's getU64, getU32, getU16, and getU8 that take a DataExtractor&, an Offset&, a descriptive twine, and return an Expected<T>. I'm trying to come up with ideas to keep all the granular checks as you desire, but pull some of the ceremonial noise out of the flow of the main logic.

This is way easier to read now. Did you sanity check loading a trace into chrome trace viewer that it outputs?

Please consider my comments about the tests before pushing.

Haven't read the test yet. Logic looks sound.

Publishing existing comments. Haven't reviewed the whole patch.

Just one concern about exit handler being too aggressive if normal operation finalized and flushed.

Is this still a work in progress patch as noted? I'll make some time to look at this today.

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?

I'm going to have to download the renamed files to diff locally. Is there a way to do this in Differential that I'm missing?

Aside from the discussion about a more invasive change to the way cooperative state transitions, this is fine. Thanks for writing the test for serialization.

LGTM, once the dependent changes get in.

in the chain* I meant.

This one is more straightforward than the previous in the change. The main ideas all fall into place nicely, but I have pointed out some details that could use some attention.

My overall feedback about this code is that after reading it, it probably deserves another pass over just to focus on what allocates and frees and which state transitions are responsible for closing the loops.

This was easier to review than I expected. I found it easier to follow than the allocator/array CL. Sorry for the long delay!

LGTM.

Can you add some Reports, which could be guarded behind sanitizer verbose, for warning messages when users do deprecated things? This looks sensible, but a bit of nagging incentives people to change.

Everything looks good except I'd like to see us flexible for mode configuration that may contain nulls.

This looks better now, although there were a few issues that slipped through the tests and I wonder if there are others lurking.

Finally reached the end. Phew!

Haven't made any changes yet. Thanks for the comments Dean.

Haven't looked at segmented array yet, but I had a good look at the allocator. I think I caught one serious bug, and the rest is just suggestions.

I'm going to land this and the llvm/clang changes and in a separate CL make some improvements as discussed offline for event type registration (handling deduping event descriptors).

Undoing formatting change.

My editor got a bit carried away with automatically clang-formatting lib/CodeGen/CodeGenFunction.cpp. I'll fix that so that I'm not messing up the revision history.

Added flags and bundle options.

Adding a comment to the test to encourage getting the event types from
compiler-rt

Adding some comments for questions I had. Please chime in if you can shed light on these.

[XRay] [compiler-rt] Implement trampoline and handler for typed xray event tracing.

These comments were dangling for a while, so I'm posting an update. Currently compiling llvm with a new intrinsic to match. Fingers crossed!

[XRay] [compiler-rt] Implement trampoline and handler for typed xray event tracing.

Thanks for the cleanup.

Good point about preserving the size and structure of XRayLogImpl for ABI compatibility. Piper monorepo has lulled me into a false sense of security!

Thanks for the feedback Dean.

Mostly looks good, but I think you should consider making the buffer iterator interface part of XRayLogImpl or some other mechanism to associate it with a logging mode.

• Jump 20

Thanks for pointing out the scope cleanup from MProtectHelper and making some improvements/clarifications.

Suggestion for future changes:

Here are the revisions pre-landing. I'll probably land this tomorrow and follow up with a separate review of simple lit test cases for these conversions.

• Phabricator comments.

Made some changes, particularly regarding string formatting.

Thanks for taking a look Dean. Here's a link to an example trace that can be loaded up at chrome://tracing. I'll respond to your other comments tomorrow.

Updating with Dean's review comments.

Switch from std::for_each to range based for.

David Blakie convinced me over email that std::for_each doesn't add value over ranged based for.

Update the way an iterator type is referenced to not make assumptions about references.