LGTM -- thanks!

Do you think we should fix this by handling the xray-never attribute in XRayInstrumentation, or having CodeGenFunction remove the threshold if xray-never is set?

Okay, I think I understand the motivation a little bit more now (i.e. sampling seems to be a useful thing to do anyway, regardless of whether we have the other facilities).

There's two tensions I'm trying to resolve in my mind with this patch:

LGTM

LGTM from me if XRay actually does work on non-x86_64 macOS.

LGTM -- thank you, and I'm happy this has been useful for your needs!

LGTM

In D74362#2065160, @MaskRay wrote:

A high-level comment: does this pass achieve something not doable with XRay, a mature instrumentation framework in LLVM?

In D78590#1995525, @MaskRay wrote:

@dberris xray_fn_idx is still using absolute relocations and thus must have the SHF_WRITE flag. If we are also going to make it read-only, how can we address compatibility issues in compiler-rt/lib/xray ?

I'll defer to @MaskRay on the ARM bits.

LGTM for the x86_64 bits.

I'm not well versed in AArch64 assembler here, so I'll defer to @MaskRay who has more knowledge in this area.

LGTM -- thanks!

In D78082#1987960, @MaskRay wrote:

In D78082#1982766, @dberris wrote:

In D78082#1982713, @MaskRay wrote:

In D78082#1982695, @dberris wrote:

LGTM, but I'm concerned about the backwards compatibility story.

We could pick a different section name. Do you have a suggestion? Doing that can change a lot of tests and I need some ack before moving toward that goal.

I want to avoid something like xray_instr_maps_ro because really the read-only section is commonplace, and I don't want to penalize the good readonly section name...

As I mentioned in the other inline comment, you can use the 'Version' field for each entry. If you increment the version number in the compiler, then you can change the runtime code to support some older versions. Doing that allows you to avoid renaming the section and maintaining some backwards compatibility.

Supporting two versions of Sled.Address can make the runtime overly complex I think?

--- a/compiler-rt/lib/xray/xray_interface.cpp
+++ b/compiler-rt/lib/xray/xray_interface.cpp
@@ -264,14 +264,14 @@ XRayPatchingStatus controlPatching(bool Enable) XRAY_NEVER_INSTRUMENT {
   // now we're assuming we can mprotect the whole section of text between the
   // minimum sled address and the maximum sled address (+ the largest sled
   // size).
-  auto MinSled = InstrMap.Sleds[0];
-  auto MaxSled = InstrMap.Sleds[InstrMap.Entries - 1];
+  auto *MinSled = &InstrMap.Sleds[0];
+  auto *MaxSled = &InstrMap.Sleds[InstrMap.Entries - 1];
   for (std::size_t I = 0; I < InstrMap.Entries; I++) {
     const auto &Sled = InstrMap.Sleds[I];
-    if (Sled.Address < MinSled.Address)
-      MinSled = Sled;
-    if (Sled.Address > MaxSled.Address)
-      MaxSled = Sled;
+    if (Sled.address() < MinSled->address())
+      MinSled = &Sled;
+    if (Sled.address() > MaxSled->address())
+      MaxSled = &Sled;
   }

One approach is to teach the member function about return Version < 1 ? Address : reinterpret_cast<uint64_t>(&Address) + Address; but this will impose overhead on every operation.

x86 CUSTOM_EVENT has used version 1. Bump versions of all SledKind's to 2?

--- a/llvm/lib/Target/X86/X86MCInstLower.cpp
+++ b/llvm/lib/Target/X86/X86MCInstLower.cpp
@@ -1510,7 +1510,7 @@ void X86AsmPrinter::LowerPATCHABLE_EVENT_CALL(const MachineInstr &MI,
   // Record the sled version. Older versions of this sled were spelled
   // differently, so we let the runtime handle the different offsets we're
   // using.
-  recordSled(CurSled, MI, SledKind::CUSTOM_EVENT, 1);
+  recordSled(CurSled, MI, SledKind::CUSTOM_EVENT, 2);
 }

In D78082#1982713, @MaskRay wrote:

In D78082#1982695, @dberris wrote:

LGTM, but I'm concerned about the backwards compatibility story.

We could pick a different section name. Do you have a suggestion? Doing that can change a lot of tests and I need some ack before moving toward that goal.

I want to avoid something like xray_instr_maps_ro because really the read-only section is commonplace, and I don't want to penalize the good readonly section name...

LGTM, but I'm concerned about the backwards compatibility story.

LGTM

Do you mind adding x86 tests too?

I'm wondering how you're imbuing these attributes to the functions from the front-ends -- is this something you intend to support as a flag in Clang for example?

In D58621#1774144, @nbaksalyar wrote:

In D58621#1409962, @dberris wrote:

Now, an alternative here is to migrate the Basic Mode implementation to use a more compact log record (i.e. using the FDR mode format), and use a different converter approach <..> This is a more intensive project but one that isn't terribly hard to accomplish. If you'd like to take that on, I'd be happy to review patches going in that direction instead

@dberris, is this still valid & desirable?
I would like to dip my toes into XRay/LLVM, and this seems like a good first mini project to me. :) This migration should also bring typed and custom events support for the basic mode, which is a nice bonus.

In D69461#1739991, @tetsuo-cpp wrote:

I compiled multiple older versions of dot and gave them a try (I went back to 2.30.1) but still didn't see this problem. I've been testing like this (mainly testing SVG and JSON):

tetsuo@garland-1572667608860-c-16-sgp1-01:~/dev/llvm-project$ build/bin/llvm-xray graph ./xray-log.a.out.uRHPKt -instr_map ./a.out | dot -Tsvg

@dberris Since I can't reproduce this, should I abandon this revision? Or would you still like the patch.

LGTM

Do you have any suggestions on how to achieve what the FIXME'ed areas are attempting to do?

LGTM with one nit. Thanks!

LGTM -- Thanks!

LGTM from the XRay parts.

LGTM -- apologies for the delay, I hope this isn't too late.

In D63973#1645986, @thegameg wrote:

Thanks, this LGTM. I quickly looked at the X86 tests, it seems that PATCHABLE_EVENT_CALL and PATCHABLE_TYPED_EVENT_CALL are always created with a register but expect immediates in Target.td. I'm not sure what was the intention so I'll leave it to @dberris.

I don't see the XRay specific changes... is there something I specifically need to look at here? Looks like the patchable tail call stuff is untouched.

LGTM -- really sorry for the delay here, for some reason I didn't get to this earlier. :(

LGTM

LGTM -- thanks for looking into this!

In D56383#1411871, @kristina wrote:

Gentle ping for author, has this landed or still in the backlog? (If it's landed can you attach the commit and close it, please? Just trying to to avoid diffs on Phab going stale.)

In D58621#1410322, @lebedev.ri wrote:

In D58621#1409962, @dberris wrote:

In D58621#1409715, @lebedev.ri wrote:

In D58621#1409704, @dberris wrote:

This is a RFC because of the uint8_t CPU change.
That chance needs discussing.

So, this is an accident of history, which should be changed, but to the other direction. I've learned some time ago that it turns out there are some platforms that can have enough CPU IDs which can't be represented by a uint8_t. For future-proofing, we really should change this to be larger (uint16_t) and change basic mode to store 16-bit CPU IDs.

Boo :)

Boo indeed. :)

Unfortunately that won't just cost that one extra byte, it will have ripple effect on the padding in this struct.
I'm not sure as to exact numbers.

I like the idea of reducing top-line memory requirements, but it shouldn't be at the cost of functionality. The current state is a bug that we're only using 8 bits for the CPU ID.

No, i totally understand.
That is why i said it's RFC and is only valid if it is actually
only ever 8 bits. (which it is, but only due to the bug elsewhere)
I think this is still worth it even with 16-bit CPU, i'll take a look.

Now, an alternative here is to migrate the Basic Mode implementation to use a more compact log record (i.e. using the FDR mode format), and use a different converter approach, one that doesn't require reconstituting the whole Trace consisting of XRayRecord instances. The FDR log loading libraries/framework allow us to do this now (see llvm-xray fdr-dump). This is a more intensive project but one that isn't terribly hard to accomplish. If you'd like to take that on, I'd be happy to review patches going in that direction instead (really the only difference between the current basic mode implementation and the FDR mode implementation is that, basic mode threads returning a buffer to the queue will write out the contents before returning the buffer to the central buffer queue). In that process we can migrate FDR mode to use 16-bit CPU IDs.

So it's basically three co-dependent steps:

1. Teach xray convert to also work on FDR input
2. Afterwards, switch the compiler-rt X-Ray basic log to output FDR log format.
3. Finally, fix the truncation of CPU id in the compiler-rt xray code

In D58621#1409715, @lebedev.ri wrote:

In D58621#1409704, @dberris wrote:

This is a RFC because of the uint8_t CPU change.
That chance needs discussing.

So, this is an accident of history, which should be changed, but to the other direction. I've learned some time ago that it turns out there are some platforms that can have enough CPU IDs which can't be represented by a uint8_t. For future-proofing, we really should change this to be larger (uint16_t) and change basic mode to store 16-bit CPU IDs.

Boo :)

This is a RFC because of the uint8_t CPU change.
That chance needs discussing.

What I wish existed was a stateful JSON output stream implementation instead of the build-everything-in-memory model. Maybe someday. :)

LGTM

LGTM

LGTM

LGTM

LGTM -- thanks! Yes, this should have been public all along.

More updates to address some comments.

Address comments by @greened.

Update description.

Add link to new doc in index.

LGTM

LGTM -- cannot reproduce the original issue with this patch.

Apologies for the delay, but I'm giving this a go now.

LGTM

LGTM

LGTM still, thanks @phosek!

LGTM -- Thanks @MaskRay!

LGTM -- Thanks @phosek!

Rebase again. This is now ready for another look.

Rebase.

Addressing comment by @mboerger.

Addressing comments by @mboerger.

LGTM

clang-format with LLVM style