[XRay] ARM 32-bit no-Thumb support in LLVM

[rSerge]

Summary

This is a port of XRay to ARM 32-bit, without Thumb support yet. The XRay instrumentation support is moving up to AsmPrinter.
This is one of 3 commits to different repositories of XRay ARM port. The other 2 are:

1. https://reviews.llvm.org/D23932 (Clang test)
2. https://reviews.llvm.org/D23933 (compiler-rt)

[dberris]

include/llvm/CodeGen/AsmPrinter.h

209

Do you need to spell out 'class' here? Wouldn't const Function* suffice?

include/llvm/Target/Target.td

969

AFAICT, yes, this is correct. The expectation is that this instruction should only ever show up in the assembler as a pseudo instruction (unless this is doing something else).

970

This one is a little harder. At least in x86, we weren't able to get this to work this way, because stack adjustments may happen later than the insertion of the marker instruction. Unless you can control exactly when this instruction is inserted and that the stack adjustment code doesn't ever move this (or add things after this instruction) then you might want to go do the same thing that we're doing in X86.

include/llvm/Target/TargetOpcodes.def

161

Is there any reason to do this instead of following the same convention used in x86 of having the nops be after the return instruction?

lib/CodeGen/XRayInstrumentation.cpp

46–53

This is a great explanation. Can you say something similar in the description just so it's clear why there's a difference in the approach?

lib/Target/X86/X86AsmPrinter.h

74–94

I think it's worth noting in the description that we're moving the XRay instrumentation support up to AsmPrinter too.

[rSerge]

include/llvm/CodeGen/AsmPrinter.h

209

I just moved (copy-pasted) this from X86AsmPrinter.h . Without class it does not compile because XRayFunctionEntry already has a member wih the same name: const MCSymbol *Function .

include/llvm/Target/Target.td

970

The same thing as in x86_64 is not possible for ARM because it has multiple return instructions. Furthermore, CPU allows parametrized and even conditional return instructions. In the current ARM implementation we are making use of the fact that currently LLVM doesn't seem to generate conditional return instructions.
On ARM, the same instruction can be used for popping multiple registers from the stack and returning (it just pops pc register too), and LLVM generates it sometimes. So we can't insert the sled between this stack adjustment and the return without splitting the original instruction into 2 instructions. So on ARM, rather than jumping into the exit trampoline, we call it, it does the tracing, preserves the stack and returns.

include/llvm/Target/TargetOpcodes.def

161

Yes, as I've explained above, the problem is that ARM has multiple return instructions, so we have to preserve the original return instruction and call the exit tracing trampoline instead of jumping into it. I'm adding a comment in the code too.

lib/CodeGen/XRayInstrumentation.cpp

46–53

I'm adding it to llvm\include\llvm\Target\TargetOpcodes.def .

[rSerge]

Implemented the requested changes (more comments).

[rengolin]

Hi, I see a number of problems with this patch. The most common one is the direct emission of binary patterns, which is not clear nor maintainable. Please, use the builders to emit instructions.

Also, I'm worried that the space you're reserving for the binary patch won't be enough for all cases. There are a number of PCS issues (hard vs soft, larger-than-32bit returns, arch and sub-arch support of return styles) which you're not accounting for any of them.

Furthermore, you need to make sure thumb-interworking works. You're outputting ARM code, but the user code can very well be Thumb, so you need to make sure it works. Not all architectures support BLX either (ex. v4T), and POP { lr } has been deprecated.

Finally, you need tests. A lot of them. To make sure you are covering the architectures you intend, in all the configurations you intend, and to actively fail if you don't intend, by adding checks in the code that error out when the arch / sub-arch is in a combination you don't expect.

[rengolin]

lib/CodeGen/XRayInstrumentation.cpp

46–53

Agreed. Probably move the separate comments to their implementations?

92

Good point. Probably not correct.

126

nit: this comment is better applied to the function "prependRetWithPatchableExit" after the case. People will know what to do in the future. You don't need a comment on the default case, too.

lib/Target/ARM/ARMAsmPrinter.cpp

1983

No need for braces if you're not declaring variables.

lib/Target/ARM/ARMMCInstLower.cpp

158

There isn't, as nop is currently only an alias, not an instruction. But take a look at:

ARMInstrInfo::getNoopForMachoTarget()

and do the same for ELF.

181

Why just save r0? AAPCS can use all four r0-r3 for return results.

187

BLX is unconditional, POP will never be executed. Is that intended?

198

Please, don't emit binary directly. Use the builders.

[dberris]

lib/CodeGen/XRayInstrumentation.cpp

92

Yes, this is definitely not correct. This is a remnant of some refactoring I've done and it stuck around. :(

Let me add a test and fix, should be trivial.

[dberris]

lib/CodeGen/XRayInstrumentation.cpp

92

This is now fixed in rL280192 -- please rebase to get the change (and tests).

[rSerge]

In D23931#529004, @rengolin wrote:

Hi, I see a number of problems with this patch. The most common one is the direct emission of binary patterns, which is not clear nor maintainable. Please, use the builders to emit instructions.

Also, I'm worried that the space you're reserving for the binary patch won't be enough for all cases. There are a number of PCS issues (hard vs soft, larger-than-32bit returns, arch and sub-arch support of return styles) which you're not accounting for any of them.

Furthermore, you need to make sure thumb-interworking works. You're outputting ARM code, but the user code can very well be Thumb, so you need to make sure it works. Not all architectures support BLX either (ex. v4T), and POP { lr } has been deprecated.

Finally, you need tests. A lot of them. To make sure you are covering the architectures you intend, in all the configurations you intend, and to actively fail if you don't intend, by adding checks in the code that error out when the arch / sub-arch is in a combination you don't expect.

Hi,
Ok, I'll look if the same can be done with builders.
I'm not targeting all ARM architectures at once, at least not in the first commit. I think we should choose 1 ARM architecture for which XRay works, and assume the others not supported or experimental. Currently I am building and experimenting with armhf (32-bit).
Sled sizes do not have to fit all architectures either (this would result in waste of space for some, thus worse performance due to cache misses). Currently sleds are 11 bytes on x86_64 and 28 bytes on armhf.
What is PCS ?
Thumb is not supported yet.
Architectures which do not support BLX are not supported.
Any evidence that POP {lr} is deprecated? I could only find on the internet that "These instructions that include both PC and LR in the reglist are deprecated in ARMv6T2 and above.": http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0588b/Babefbce.html . I'm not using both pc and lr in PUSH or POP.
Any specific examples which more tests should be added for the single supported architecture armhf?

lib/CodeGen/XRayInstrumentation.cpp

126

Moving the comments towards the function calls.

lib/Target/ARM/ARMAsmPrinter.cpp

1983

Removing.

lib/Target/ARM/ARMMCInstLower.cpp

181

We save the other registers in the trampoline (__xray_FunctionEntry and __xray_FunctionExit assembly functions).

187

POP is intended to execute after return from the subroutine, which BLX calls.

[rSerge]

Updated with the changes requested in the comments.

[rengolin]

In D23931#530324, @rSerge wrote:

I'm not targeting all ARM architectures at once, at least not in the first commit. I think we should choose 1 ARM architecture for which XRay works, and assume the others not supported or experimental. Currently I am building and experimenting with armhf (32-bit).

Right, "armhf" is not one ARM architecture, but dozens. It can be anything from v6T2 to V8.2A, including all sub-architectures, features and variations. Though, from what I've seen so far, the code you use would work on any architecture of that range.

It would be safer, though, to document the *intended* target specifically, like "ARMv7A with VFPv3 support". So that people with "ARMv6T2 with VFPv2" support are not surprised when you assumed something "wrong" for them. Adding a "hasV6T2Ops()" check on the entry-point would help.

Sled sizes do not have to fit all architectures either (this would result in waste of space for some, thus worse performance due to cache misses). Currently sleds are 11 bytes on x86_64 and 28 bytes on armhf.

Check.

What is PCS ?

Procedure Call Standard. This is the part of the ABI that defines how functions are called to be compatible with the ABI. Mostly about how to serialise arguments and return values in registers, stack, etc.

Both C and C++, as well as any other language that wants to be compatible with ARM's EABI standard *have* to abide to those terms.

Thumb is not supported yet.

Do you mean not supported in the Sled code, or inserting ARM Sled code into Thumb functions?

If the former, then you have to check if the architecture/OS/ABI you're supporting allows ARM code. For instance, Windows doesn't.

If the latter, than you need to check if the architecture/OS/ABI you're supporting allows Thumb code. For instance, there could be libraries around, or even inline assembly with ".thumb" in it (yes, this does happen). I can't remember how, but there's a way to know what's the ISA for a specific function, this could help you. OTOH, this could be an assembler things, can't remember.

Any way, you need to check if the architecture/OS/ISA/ABI you have is compatible with your assumptions before you emit code.

Architectures which do not support BLX are not supported.

Fair enough. But as I said earlier, this has to be clearly encoded (via error messages) on the entry-point of your code.

Any evidence that POP {lr} is deprecated?

Sorry, my bad. I was thinking about a different case. Ignore me.

Any specific examples which more tests should be added for the single supported architecture armhf?

As I said earlier, you need to make sure you only emit your stubs on architectures that you know works. Checking the target for architecture level, ISA support and ABI should be enough, at least on the entry-point.

Adding tests is, then, easily done by having two files: one where everything should fail, RUNning with a "not" before "llc", CHECKing for error messages, and one where everything should pass, CHECKing for the correct sequence of Nops, etc.

It should be fine to add all error messages to one file and all cases that should pass to another.

cheers,
--renato

lib/Target/ARM/ARMMCInstLower.cpp

181

Right, and these are guaranteed to only use one 32-bit argument. Check.

187

D'oh, Branch&Link, sorry, you're correct.

test/CodeGen/ARM/xray-attribute-instrumentation.ll

6

I was expecting Nops...

[rSerge]

lib/Target/X86/X86AsmPrinter.h

74–94

Do you mean the description for the diff? Or a comment in the source code?

test/CodeGen/ARM/xray-attribute-instrumentation.ll

6

The first instruction is a jump over the NOPs. The other 6 instructions are NOPs.

[rengolin]

test/CodeGen/ARM/xray-attribute-instrumentation.ll

6

Right, I was referring to the .ascii... When you use builders, this won't happen any more. It will also work in big-endian. :)

[rSerge]

In D23931#530369, @rengolin wrote:

In D23931#530324, @rSerge wrote:

I'm not targeting all ARM architectures at once, at least not in the first commit. I think we should choose 1 ARM architecture for which XRay works, and assume the others not supported or experimental. Currently I am building and experimenting with armhf (32-bit).

Right, "armhf" is not one ARM architecture, but dozens. It can be anything from v6T2 to V8.2A, including all sub-architectures, features and variations. Though, from what I've seen so far, the code you use would work on any architecture of that range.

Thanks for explaining. I am still starting with ARM and LLVM.

It would be safer, though, to document the *intended* target specifically, like "ARMv7A with VFPv3 support". So that people with "ARMv6T2 with VFPv2" support are not surprised when you assumed something "wrong" for them. Adding a "hasV6T2Ops()" check on the entry-point would help.

Ok, I'll try to select something more specific than armhf.

Sled sizes do not have to fit all architectures either (this would result in waste of space for some, thus worse performance due to cache misses). Currently sleds are 11 bytes on x86_64 and 28 bytes on armhf.

Check.

What is PCS ?

Procedure Call Standard. This is the part of the ABI that defines how functions are called to be compatible with the ABI. Mostly about how to serialise arguments and return values in registers, stack, etc.

Both C and C++, as well as any other language that wants to be compatible with ARM's EABI standard *have* to abide to those terms.

Thumb is not supported yet.

Do you mean not supported in the Sled code, or inserting ARM Sled code into Thumb functions?

Neither is supported. I estimated that Thumb support requires substantial additional effort.

If the former, then you have to check if the architecture/OS/ABI you're supporting allows ARM code. For instance, Windows doesn't.

If the latter, than you need to check if the architecture/OS/ABI you're supporting allows Thumb code. For instance, there could be libraries around, or even inline assembly with ".thumb" in it (yes, this does happen). I can't remember how, but there's a way to know what's the ISA for a specific function, this could help you. OTOH, this could be an assembler things, can't remember.

Yes, this looks like a lot of effort.

Any way, you need to check if the architecture/OS/ISA/ABI you have is compatible with your assumptions before you emit code.

Architectures which do not support BLX are not supported.

Fair enough. But as I said earlier, this has to be clearly encoded (via error messages) on the entry-point of your code.

Any evidence that POP {lr} is deprecated?

Sorry, my bad. I was thinking about a different case. Ignore me.

Any specific examples which more tests should be added for the single supported architecture armhf?

As I said earlier, you need to make sure you only emit your stubs on architectures that you know works. Checking the target for architecture level, ISA support and ABI should be enough, at least on the entry-point.

Adding tests is, then, easily done by having two files: one where everything should fail, RUNning with a "not" before "llc", CHECKing for error messages, and one where everything should pass, CHECKing for the correct sequence of Nops, etc.

It should be fine to add all error messages to one file and all cases that should pass to another.

cheers,
--renato

The amount of change requested in the code review seems too much for the first iteration. Can we limit the scope and plan incremental improvements?

Cheers,
Serge

[rSerge]

Updated with the latest changes from mainline.

[rengolin]

In D23931#531728, @rSerge wrote:

Do you mean not supported in the Sled code, or inserting ARM Sled code into Thumb functions?

Neither is supported. I estimated that Thumb support requires substantial additional effort.

My gut feeling is that this should mostly work already, since you're using BLX instructions.

But I agree, let's not get ahead of ourselves.

Limit support for ARMv7A, non-Windows (which forces Thumb2). Something like:

if (!SubTarget->hasV7Ops() || SubTarget->isWindows())
  return Forgerabarit.

cheers,
--renato

[dberris]

lib/Target/X86/X86AsmPrinter.h

74–94

Definitely a description in the diff.

[rSerge]

In D23931#531853, @rengolin wrote:

In D23931#531728, @rSerge wrote:

Do you mean not supported in the Sled code, or inserting ARM Sled code into Thumb functions?

Neither is supported. I estimated that Thumb support requires substantial additional effort.

My gut feeling is that this should mostly work already, since you're using BLX instructions.

BLX r12 instruction has different machine code for ARM and Thumb. It is 4 byte long on ARM and 2 byte long on Thumb. Furthermore, the rest of machine code in a sled contains 32-bit ARM instructions. Thumb may need different machine code, or even sequence of instructions because not everything is available in Thumb. To avoid changing trampoline assembly code, the trampoline can be called with BLX indicating that the destination is in ARM assembly.

But I agree, let's not get ahead of ourselves.

Limit support for ARMv7A, non-Windows (which forces Thumb2). Something like:

if (!SubTarget->hasV7Ops() || SubTarget->isWindows())
  return Forgerabarit.

Ok.

[rSerge]

Implemented the changes requested in the code review.

[rSerge]

Limit support for ARMv7A, non-Windows (which forces Thumb2). Something like:

if (!SubTarget->hasV7Ops() || SubTarget->isWindows())
  return Forgerabarit.

It seems that ARMv6 is sufficient. Implemented mostly as suggested.

lib/Target/ARM/ARMMCInstLower.cpp

158

Changed.

198

Done.

test/CodeGen/ARM/xray-attribute-instrumentation.ll

7

Done.

[rengolin]

Hi Serge,

The Nop emission is really simple, and the isXRaySupported() is really simple and accurate. Thanks for addressing all the comments, the code is looking really nice.

Now, two points:

1. There are ways to report warnings/errors back to the front-end, but it depends how this is interpreted.

Since the instrumentation is inserted by the front end, than this should be a back-end *error*, and front-ends should fail with a decent error message saying "XRay is not supported for target X".

If you want just a warning, you can avoid inserting the sleds and the run-time code won't do anything, as you're doing it now. But you then have to warn the users that they won't get what they requested. I strongly suggest to make it an error instead.

For error messages, it's best to use "getContext().reportError(Loc, ...)", as this would nicely roll back to the front-end without crashing. But if that doesn't work (it should, really), you can use "report_fatal_error", "llvm_unreachable" or even an "assert()", though these are just last-resort only.

About front-end duplicating the checks, it's up to you and @dberris. The error message in Clang and llc should be the same, though, and reportError() does that well.

2. Tests.

The current test is good, it checks the right number of NOPs and the overall structure. Excellent.

Now we need "negative tests", ie. those that *have* to fail. For that, you add a RUN line that starts with "not llc ..." and CHECK for the error messages. There are plenty of examples in there already.

Since you're restricting x86_64, you should have one for i386. Since you're restricting ARMv6/Unix, you should have one for ARMv5, and one for ARM Windows.

cheers,
--renato

[dberris]

In D23931#533606, @rengolin wrote:

Hi Serge,

The Nop emission is really simple, and the isXRaySupported() is really simple and accurate. Thanks for addressing all the comments, the code is looking really nice.

+1 -- thanks @rSerge!

Now, two points:

1. There are ways to report warnings/errors back to the front-end, but it depends how this is interpreted.

Since the instrumentation is inserted by the front end, than this should be a back-end *error*, and front-ends should fail with a decent error message saying "XRay is not supported for target X".

If you want just a warning, you can avoid inserting the sleds and the run-time code won't do anything, as you're doing it now. But you then have to warn the users that they won't get what they requested. I strongly suggest to make it an error instead.

For error messages, it's best to use "getContext().reportError(Loc, ...)", as this would nicely roll back to the front-end without crashing. But if that doesn't work (it should, really), you can use "report_fatal_error", "llvm_unreachable" or even an "assert()", though these are just last-resort only.

About front-end duplicating the checks, it's up to you and @dberris. The error message in Clang and llc should be the same, though, and reportError() does that well.

I'm happy with an error using the usual error reporting mechanisms here.

2. Tests.

The current test is good, it checks the right number of NOPs and the overall structure. Excellent.

Now we need "negative tests", ie. those that *have* to fail. For that, you add a RUN line that starts with "not llc ..." and CHECK for the error messages. There are plenty of examples in there already.

Since you're restricting x86_64, you should have one for i386. Since you're restricting ARMv6/Unix, you should have one for ARMv5, and one for ARM Windows.

I agree with this. FWIW, I'm happy with getting this in and getting it tested, then locking it down with more negative tests once it's upstream.

Thanks Renato!

lib/Target/ARM/ARMAsmPrinter.h

102–106

Do you already want to support tail call optimisation sleds now? Or did you plan to do something about that later?

[dberris]

LGTM (I think we should be fine with adding more tests later)

Thanks again @rSerge!

[rengolin]

Thanks!

[rSerge]

This comment was removed by rSerge.

[rSerge]

This comment was removed by rSerge.

[rSerge]

So something started to just remove the first instruction of the sled, whether the sled is emitted as binary or using instructions/builders. Clang -S generates the assembly file with correct sleds (all the instructions present), but then disassembly of the object or executable file shows only 6 last instructions, without the first instruction of the sled.
UPDATE: I just confused the compile options, so assembly files were new and object files were old. No problem with this in the code, tested.

[rSerge]

Rebased to the latest revision. I don't have commit access rights. Could someone commit?

[dberris]

I'll do it for all three, thanks again @rSerge!

[dberris]

For some reason the standard arc patch DNNNNN workflow doesn't apply to this patch (I'm not sure if it's generated in a manner not using arcanist). I've had to massage this manually by doing:

curl https://reviews.llvm.org/file/data/spjqzhddatjrbozzbl4u/PHID-FILE-7s4h3zdshadln2e7cgbi/D23931.diff  | git apply - -p0 --ignore-whitespace --whitespace=fix

I may have to do something similar to the other patches, so all landing errors will be mine.

[rSerge]

Thanks all, especially @dberris .

[dberris]

So, unfortunately this got reverted in rL280967 because it fails on thumb (as the checks hadn't been put in to not generate XRay sleds for non-thumb).

@rSerge -- are you able to put in the appropriate checks to warn when using XRay on thumb? @rengolin has offered to help with the testing on the build-bots to make this possible.

[dberris]

I think @rengolin has more details as to how this caused failures and how else to debug on thumb.

[rSerge]

I don't yet understand how these commits could break build-bots. Did someone add -fxray-instrument clang option to bots which generate Thumb code?

[rengolin]

In D23931#538093, @rSerge wrote:

I don't yet understand how these commits could break build-bots. Did someone add -fxray-instrument clang option to bots which generate Thumb code?

Nope. The error was when compiling xray_trampoline_arm.S.

Compiler-RT's patch enables XRay on ARM, which means it'll run all the existing XRay tests on ARM buildbots, which also mean Thumb ones, which also build XRay's sources.

This was the error message:

FAILED: /usr/lib/ccache/cc  -DXRAY_HAS_EXCEPTIONS=1 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iprojects/compiler-rt/lib/xray -I/home/linaro/devel/buildbot/clang-cmake-thumbv7-a15-full-sh/llvm/projects/compiler-rt/lib/xray -Iinclude -I/home/linaro/devel/buildbot/clang-cmake-thumbv7-a15-full-sh/llvm/include -I/home/linaro/devel/buildbot/clang-cmake-thumbv7-a15-full-sh/llvm/projects/compiler-rt/lib/xray/.. -I/home/linaro/devel/buildbot/clang-cmake-thumbv7-a15-full-sh/llvm/projects/compiler-rt/lib/xray/../../include -fPIC -O3 -DNDEBUG   -UNDEBUG  -march=armv7-a -mfloat-abi=hard -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fvisibility=hidden -fvisibility-inlines-hidden -fno-function-sections -fno-lto -O3 -g -Wno-variadic-macros -Wno-non-virtual-dtor -MMD -MT projects/compiler-rt/lib/xray/CMakeFiles/clang_rt.xray-armhf.dir/xray_trampoline_arm.S.o -MF projects/compiler-rt/lib/xray/CMakeFiles/clang_rt.xray-armhf.dir/xray_trampoline_arm.S.o.d -o projects/compiler-rt/lib/xray/CMakeFiles/clang_rt.xray-armhf.dir/xray_trampoline_arm.S.o -c /home/linaro/devel/buildbot/clang-cmake-thumbv7-a15-full-sh/llvm/projects/compiler-rt/lib/xray/xray_trampoline_arm.S

llvm/projects/compiler-rt/lib/xray/xray_trampoline_arm.S: Assembler messages:
llvm/projects/compiler-rt/lib/xray/xray_trampoline_arm.S:17: Error: attempt to use an ARM instruction on a Thumb-only processor -- `push {r1-r3,lr}'

My patch didn't work because this is using the system compiler, and not Clang, and GCC was picky about assembling ARM instructions (from xray_trampoline_arm.S) into an object that will be linked with other Thumb-only objects.

This will require some experimentation with a cross GCC/binutils, to make sure that -mthumb won't generate the NOPs as well as not try to link the code. An #ifndef __thumb__ in xray_trampoline_arm.S to omit everything could work, so if the Clang implementation is wrong, we get a compiler error instead of a run-time error.

cheers,
--renato

[rSerge]

Now I understand, thanks, @rengolin . Thumb is on my list, though I thought it can be done later. Now I need to weigh whether all the work with conditional compilation and error reporting for Thumb is not too much w.r.t. the time to just implement the support for Thumb. I'm looking into this...

[rSerge]

Fixed "Error: attempt to use an ARM instruction on a Thumb-only processor -- `push {r1-r3,lr}' ". The reason was ".arch armv7" directive. This directive for GCC represents the intersection of arm7v-a and armv7-m instruction sets, implying Thumb-only instructions, and this conflicts with ".code 32" directive. Then GCC, instead of articulating the conflict, complains about every instruction in the assembly file.
Tested on cross-compilation with GCC from x86_64-Ubuntu to ARM-Linux.
Tested on cross-compilation with Clang from x86_64-Windows to ARM-Linux.

[rSerge]

Fixed patch file format.

[rengolin]

In D23931#544741, @rSerge wrote:

Fixed "Error: attempt to use an ARM instruction on a Thumb-only processor -- `push {r1-r3,lr}' ". The reason was ".arch armv7" directive. This directive for GCC represents the intersection of arm7v-a and armv7-m instruction sets, implying Thumb-only instructions, and this conflicts with ".code 32" directive. Then GCC, instead of articulating the conflict, complains about every instruction in the assembly file.

Ah, yes! This makes sense.

[dberris]

Thanks @rSerge -- I'll land this and dependent patches again.

Cheers

include/llvm/CodeGen/AsmPrinter.h

public:
   const MCSection *getCurrentSection() const;
 
   void getNameWithPrefix(SmallVectorImpl<char> &Name,
                          const GlobalValue *GV) const;
 
   MCSymbol *getSymbol(const GlobalValue *GV) const;
 
   //===------------------------------------------------------------------===//
+  // XRay instrumentation implementation.
+  //===------------------------------------------------------------------===//
+public:
+  // This describes the kind of sled we're storing in the XRay table.
+  enum class SledKind : uint8_t {
+    FUNCTION_ENTER = 0,
+    FUNCTION_EXIT = 1,
+    TAIL_CALL = 2,
+  };
+
+  // The table will contain these structs that point to the sled, the function
+  // containing the sled, and what kind of sled (and whether they should always
+  // be instrumented).
+  struct XRayFunctionEntry {
+    const MCSymbol *Sled;
+    const MCSymbol *Function;
+    SledKind Kind;
+    bool AlwaysInstrument;
+    const class Function *Fn;
+  };
+
+  // All the sleds to be emitted.
+  std::vector<XRayFunctionEntry> Sleds;
+
+  // Helper function to record a given XRay sled.
+  void recordSled(MCSymbol *Sled, const MachineInstr &MI, SledKind Kind);
+
+  //===------------------------------------------------------------------===//
   // MachineFunctionPass Implementation.
   //===------------------------------------------------------------------===//
 
   /// Record analysis usage.
   ///
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
   /// Set up the AsmPrinter when we are working on a new module. If your pass

include/llvm/Target/Target.td

def PATCHABLE_FUNCTION_ENTER : Instruction {
   let InOperandList = (ins);
   let AsmString = "# XRay Function Enter.";
   let usesCustomInserter = 1;
   let hasSideEffects = 0;
 }
 def PATCHABLE_RET : Instruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins variable_ops);
-  let AsmString = "# XRay Function Exit.";
+  let AsmString = "# XRay Function Patchable RET.";
   let usesCustomInserter = 1;
   let hasSideEffects = 1;
   let isReturn = 1;
 }
+def PATCHABLE_FUNCTION_EXIT : Instruction {
+  let OutOperandList = (outs);
+  let InOperandList = (ins);
+  let AsmString = "# XRay Function Exit.";
+  let usesCustomInserter = 1;
+  let hasSideEffects = 0; // FIXME: is this correct?
+  let isReturn = 0; // Original return instruction will follow
+}
 
 // Generic opcodes used in GlobalISel.
 include "llvm/Target/GenericOpcodes.td"
 
 }
 
 //===----------------------------------------------------------------------===//
 // AsmParser - This class can be implemented by targets that wish to implement

include/llvm/Target/TargetOpcodes.def

 
 /// This is a marker instruction which gets translated into a nop sled, useful
 /// for inserting instrumentation instructions at runtime.
 HANDLE_TARGET_OPCODE(PATCHABLE_FUNCTION_ENTER)
 
 /// Wraps a return instruction and its operands to enable adding nop sleds
 /// either before or after the return. The nop sleds are useful for inserting
 /// instrumentation instructions at runtime.
+/// The patch here replaces the return instruction. 
 HANDLE_TARGET_OPCODE(PATCHABLE_RET)
 
+/// This is a marker instruction which gets translated into a nop sled, useful
+/// for inserting instrumentation instructions at runtime.
+/// The patch here prepends the return instruction.
+/// The same thing as in x86_64 is not possible for ARM because it has multiple
+/// return instructions. Furthermore, CPU allows parametrized and even
+/// conditional return instructions. In the current ARM implementation we are
+/// making use of the fact that currently LLVM doesn't seem to generate
+/// conditional return instructions. 
+/// On ARM, the same instruction can be used for popping multiple registers
+/// from the stack and returning (it just pops pc register too), and LLVM
+/// generates it sometimes. So we can't insert the sled between this stack
+/// adjustment and the return without splitting the original instruction into 2
+/// instructions. So on ARM, rather than jumping into the exit trampoline, we
+/// call it, it does the tracing, preserves the stack and returns.
+HANDLE_TARGET_OPCODE(PATCHABLE_FUNCTION_EXIT)
+
 /// The following generic opcodes are not supposed to appear after ISel.
 /// This is something we might want to relax, but for now, this is convenient
 /// to produce diagnostics.
 
 /// Generic ADD instruction. This is an integer add.
 HANDLE_TARGET_OPCODE(G_ADD)
 HANDLE_TARGET_OPCODE_MARKER(PRE_ISEL_GENERIC_OPCODE_START, G_ADD)
 

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

GCMetadataPrinter *AsmPrinter::GetOrCreateGCPrinter(GCStrategy &S) {
 
   report_fatal_error("no GCMetadataPrinter registered for GC: " + Twine(Name));
 }
 
 /// Pin vtable to this file.
 AsmPrinterHandler::~AsmPrinterHandler() {}
 
 void AsmPrinterHandler::markFunctionEnd() {}
+
+void AsmPrinter::recordSled(MCSymbol *Sled, const MachineInstr &MI,
+  SledKind Kind) {
+  auto Fn = MI.getParent()->getParent()->getFunction();
+  auto Attr = Fn->getFnAttribute("function-instrument");
+  bool AlwaysInstrument =
+    Attr.isStringAttribute() && Attr.getValueAsString() == "xray-always";
+  Sleds.emplace_back(
+    XRayFunctionEntry{ Sled, CurrentFnSym, Kind, AlwaysInstrument, Fn });
+}

lib/CodeGen/XRayInstrumentation.cpp

 struct XRayInstrumentation : public MachineFunctionPass {
   static char ID;
 
   XRayInstrumentation() : MachineFunctionPass(ID) {
     initializeXRayInstrumentationPass(*PassRegistry::getPassRegistry());
   }
 
   bool runOnMachineFunction(MachineFunction &MF) override;
+
+private:
+  // Replace the original RET instruction with the exit sled code ("patchable ret"
+  //   pseudo-instruction), so that at runtime XRay can replace the sled with a
+  //   code jumping to XRay trampoline, which calls the tracing handler and, in
+  //   the end, issues the RET instruction.
+  // This is the approach to go on CPUs which have a single RET instruction,
+  //   like x86/x86_64.
+  void replaceRetWithPatchableRet(MachineFunction &MF, const TargetInstrInfo *TII);
+  // Prepend the original return instruction with the exit sled code ("patchable
+  //   function exit" pseudo-instruction), preserving the original return instruction
+  //   just after the exit sled code.
+  // This is the approach to go on CPUs which have multiple options for the return
+  //   instruction, like ARM. For such CPUs we can't just jump into the XRay trampoline
+  //   and issue a single return instruction there. We rather have to call the
+  //   trampoline and return from it to the original return instruction of the
+  //   function being instrumented.
+  void prependRetWithPatchableExit(MachineFunction &MF, const TargetInstrInfo *TII);
 };
+} // anonymous namespace
+
+void XRayInstrumentation::replaceRetWithPatchableRet(MachineFunction &MF, const TargetInstrInfo *TII)
+{
+  // We look for *all* terminators and returns, then replace those with
+  // PATCHABLE_RET instructions.
+  SmallVector<MachineInstr *, 4> Terminators;
+  for (auto &MBB : MF) {
+    for (auto &T : MBB.terminators()) {
+      // FIXME: Handle tail calls here too?
+      if (T.isReturn() && T.getOpcode() == TII->getReturnOpcode()) {
+        // Replace return instructions with:
+        //   PATCHABLE_RET <Opcode>, <Operand>...
+        auto MIB = BuildMI(MBB, T, T.getDebugLoc(),
+          TII->get(TargetOpcode::PATCHABLE_RET))
+          .addImm(T.getOpcode());
+        for (auto &MO : T.operands())
+          MIB.addOperand(MO);
+        Terminators.push_back(&T);
+        break;
+      }
+    }
+  }
+
+  for (auto &I : Terminators)
+    I->eraseFromParent();
+}
+
+void XRayInstrumentation::prependRetWithPatchableExit(MachineFunction &MF, const TargetInstrInfo *TII)
+{
+  for (auto &MBB : MF) {
+    for (auto &T : MBB.terminators()) {
+      if (T.isReturn()) {
+        // Prepend the return instruction with PATCHABLE_FUNCTION_EXIT
+        auto MIB = BuildMI(MBB, T, T.getDebugLoc(),
+          TII->get(TargetOpcode::PATCHABLE_FUNCTION_EXIT));
+        break; //FIXME: is this correct? Can't a MachineBasicBlock have multiple return instructions?
+      }
+    }
+  }
 }
 
 bool XRayInstrumentation::runOnMachineFunction(MachineFunction &MF) {
   auto &F = *MF.getFunction();
   auto InstrAttr = F.getFnAttribute("function-instrument");
   bool AlwaysInstrument = !InstrAttr.hasAttribute(Attribute::None) &&
                           InstrAttr.isStringAttribute() &&
                           InstrAttr.getValueAsString() == "xray-always";
bool XRayInstrumentation::runOnMachineFunction(MachineFunction &MF) {
   // First, insert an PATCHABLE_FUNCTION_ENTER as the first instruction of the
   // MachineFunction.
   auto &FirstMBB = *MF.begin();
   auto &FirstMI = *FirstMBB.begin();
   auto *TII = MF.getSubtarget().getInstrInfo();
   BuildMI(FirstMBB, FirstMI, FirstMI.getDebugLoc(),
           TII->get(TargetOpcode::PATCHABLE_FUNCTION_ENTER));
 
-  // Then we look for *all* terminators and returns, then replace those with
-  // PATCHABLE_RET instructions.
-  SmallVector<MachineInstr *, 4> Terminators;
-  for (auto &MBB : MF) {
-    for (auto &T : MBB.terminators()) {
-      // FIXME: Handle tail calls here too?
-      if (T.isReturn() && T.getOpcode() == TII->getReturnOpcode()) {
-        // Replace return instructions with:
-        //   PATCHABLE_RET <Opcode>, <Operand>...
-        auto MIB = BuildMI(MBB, T, T.getDebugLoc(),
-                           TII->get(TargetOpcode::PATCHABLE_RET))
-                       .addImm(T.getOpcode());
-        for (auto &MO : T.operands())
-          MIB.addOperand(MO);
-        Terminators.push_back(&T);
-        break;
-      }
-    }
-  }
-
-  for (auto &I : Terminators)
-    I->eraseFromParent();
-
+  switch (MF.getTarget().getTargetTriple().getArch()) {
+  // List here the architectures which don't have a single return instruction
+  case Triple::ArchType::arm:
+    prependRetWithPatchableExit(MF, TII);
+  // Architectures that have a single return instruction (such as RETQ on x86_64)
+  default:
+    replaceRetWithPatchableRet(MF, TII);
   return true;
 }
 
 char XRayInstrumentation::ID = 0;
 char &llvm::XRayInstrumentationID = XRayInstrumentation::ID;
 INITIALIZE_PASS(XRayInstrumentation, "xray-instrumentation", "Insert XRay ops",
                 false, false)

lib/Target/ARM/ARMAsmPrinter.h

public:
   void EmitFunctionEntryLabel() override;
   void EmitStartOfAsmFile(Module &M) override;
   void EmitEndOfAsmFile(Module &M) override;
   void EmitXXStructor(const DataLayout &DL, const Constant *CV) override;
 
   // lowerOperand - Convert a MachineOperand into the equivalent MCOperand.
   bool lowerOperand(const MachineOperand &MO, MCOperand &MCOp);
 
+  //===------------------------------------------------------------------===//
+  // XRay implementation
+  //===------------------------------------------------------------------===//
+public:
+  // XRay-specific lowering for ARM.
+  void LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr &MI);
+  void LowerPATCHABLE_FUNCTION_EXIT(const MachineInstr &MI);
+  // Helper function that emits the XRay sleds we've collected for a particular
+  // function.
+  void EmitXRayTable();
+
 private:
+  void EmitSled(const MachineInstr &MI, SledKind Kind);
 
   // Helpers for EmitStartOfAsmFile() and EmitEndOfAsmFile()
   void emitAttributes();
 
   // Generic helper used to emit e.g. ARMv5 mul pseudos
   void EmitPatchedInstruction(const MachineInstr *MI, unsigned TargetOpc);
 
   void EmitUnwindingInstruction(const MachineInstr *MI);

lib/Target/ARM/ARMAsmPrinter.cpp

if (Subtarget->isTargetCOFF()) {
     OutStreamer->EmitCOFFSymbolStorageClass(Scl);
     OutStreamer->EmitCOFFSymbolType(Type);
     OutStreamer->EndCOFFSymbolDef();
   }
 
   // Emit the rest of the function body.
   EmitFunctionBody();
 
+  // Emit the XRay table for this function.
+  EmitXRayTable();
+
   // If we need V4T thumb mode Register Indirect Jump pads, emit them.
   // These are created per function, rather than per TU, since it's
   // relatively easy to exceed the thumb branch range within a TU.
   if (! ThumbIndirectPads.empty()) {
     OutStreamer->EmitAssemblerFlag(MCAF_Code16);
     EmitAlignment(1);
     for (unsigned i = 0, e = ThumbIndirectPads.size(); i < e; i++) {
       OutStreamer->EmitLabel(ThumbIndirectPads[i].second);
EmitToStreamer(*OutStreamer, MCInstBuilder(ARM::t2LDRi12)
                                      .addReg(ARM::PC)
                                      .addReg(SrcReg)
                                      .addImm(4)
                                      // Predicate
                                      .addImm(ARMCC::AL)
                                      .addReg(0));
     return;
   }
+  case ARM::PATCHABLE_FUNCTION_ENTER:
+  {
+    LowerPATCHABLE_FUNCTION_ENTER(*MI);
+    return;
+  }
+  case ARM::PATCHABLE_FUNCTION_EXIT:
+  {
+    LowerPATCHABLE_FUNCTION_EXIT(*MI);
+    return;
+  }
   }
 
   MCInst TmpInst;
   LowerARMMachineInstrToMCInst(MI, TmpInst, *this);
 
   EmitToStreamer(*OutStreamer, TmpInst);
 }
 

lib/Target/ARM/ARMMCInstLower.cpp

 #include "ARMAsmPrinter.h"
 #include "MCTargetDesc/ARMBaseInfo.h"
 #include "MCTargetDesc/ARMMCExpr.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCSymbolELF.h"
+#include "llvm/MC/MCSectionELF.h"
+#include "llvm/MC/MCInstBuilder.h"
 using namespace llvm;
 
 
 MCOperand ARMAsmPrinter::GetSymbolRef(const MachineOperand &MO,
                                       const MCSymbol *Symbol) {
   const MCExpr *Expr =
       MCSymbolRefExpr::create(Symbol, MCSymbolRefExpr::VK_None, OutContext);
   switch (MO.getTargetFlags() & ARMII::MO_OPTION_MASK) {
if (AP.lowerOperand(MO, MCOp)) {
         int32_t Enc = ARM_AM::getSOImmVal(MCOp.getImm());
         if (Enc != -1)
           MCOp.setImm(Enc);
       }
       OutMI.addOperand(MCOp);
     }
   }
 }
+
+// FIXME: Is there a ready way to emit NOPs on ARM?
+static void Emit4ByteNops(MCStreamer& OS, int NumInstructions, const MCSubtargetInfo &STI)
+{
+  STI; // would be useful for OS.EmitInstruction(MCInstBuilder(ARM::NOP), STI);
+  for (int I = 1; I <= NumInstructions; I++)
+  {
+    OS.EmitBytes(StringRef( /*Little-endian!*/ "\x00\xF0\x20\xE3", 4));
+  }
+}
+
+void ARMAsmPrinter::EmitSled(const MachineInstr &MI, SledKind Kind)
+{
+  // We want to emit the following pattern:
+  //
+  // .Lxray_sled_N:
+  //   ALIGN
+  //   B #20
+  //   ; 6 NOP instructions (24 bytes)
+  // .tmpN
+  //
+  // We need the 24 bytes (6 instructions) because at runtime, we'd be patching
+  // over the full 28 bytes (7 instructions) with the following pattern:
+  //
+  //   PUSH{ r0, lr }
+  //   MOVW r0, #<lower 16 bits of function ID>
+  //   MOVT r0, #<higher 16 bits of function ID>
+  //   MOVW ip, #<lower 16 bits of address of __xray_FunctionEntry/Exit>
+  //   MOVT ip, #<higher 16 bits of address of __xray_FunctionEntry/Exit>
+  //   BLX ip
+  //   POP{ r0, lr }
+  //
+  OutStreamer->EmitCodeAlignment(4);
+  auto CurSled = OutContext.createTempSymbol("xray_sled_", true);
+  OutStreamer->EmitLabel(CurSled);
+  auto Target = OutContext.createTempSymbol();
+
+  // Emit "B #20" instruction, which jumps over the next 24 bytes (because
+  // register pc is 8 bytes ahead of the jump instruction by the moment CPU
+  // is executing it).
+  // FIXME: Find another less hacky way do force the relative jump.
+  OutStreamer->EmitBytes(StringRef( /*Little-endian!*/ "\x05\x00\x00\xEA", 4));
+  Emit4ByteNops(*OutStreamer, 6, getSubtargetInfo());
+  OutStreamer->EmitLabel(Target);
+  recordSled(CurSled, MI, Kind);
+}
+
+void ARMAsmPrinter::LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr &MI)
+{
+  EmitSled(MI, SledKind::FUNCTION_ENTER);
+}
+
+void ARMAsmPrinter::LowerPATCHABLE_FUNCTION_EXIT(const MachineInstr &MI)
+{
+  EmitSled(MI, SledKind::FUNCTION_EXIT);
+}
+
+void ARMAsmPrinter::EmitXRayTable()
+{
+  if (Sleds.empty())
+    return;
+  if (Subtarget->isTargetELF()) {
+    auto *Section = OutContext.getELFSection(
+      "xray_instr_map", ELF::SHT_PROGBITS,
+      ELF::SHF_ALLOC | ELF::SHF_GROUP | ELF::SHF_MERGE, 0,
+      CurrentFnSym->getName());
+    auto PrevSection = OutStreamer->getCurrentSectionOnly();
+    OutStreamer->SwitchSection(Section);
+    for (const auto &Sled : Sleds) {
+      OutStreamer->EmitSymbolValue(Sled.Sled, 4);
+      OutStreamer->EmitSymbolValue(CurrentFnSym, 4);
+      auto Kind = static_cast<uint8_t>(Sled.Kind);
+      OutStreamer->EmitBytes(
+        StringRef(reinterpret_cast<const char *>(&Kind), 1));
+      OutStreamer->EmitBytes(
+        StringRef(reinterpret_cast<const char *>(&Sled.AlwaysInstrument), 1));
+      OutStreamer->EmitZeros(6);
+    }
+    OutStreamer->SwitchSection(PrevSection);
+  }
+  Sleds.clear();
+}

lib/Target/X86/X86AsmPrinter.h

private:
     // CurrentShadowSize counts the number of bytes encoded since the most
     // recently encountered STACKMAP, stopping when that number is greater than
     // or equal to RequiredShadowSize.
     unsigned RequiredShadowSize = 0, CurrentShadowSize = 0;
   };
 
   StackMapShadowTracker SMShadowTracker;
 
-  // This describes the kind of sled we're storing in the XRay table.
-  enum class SledKind : uint8_t {
-    FUNCTION_ENTER = 0,
-    FUNCTION_EXIT = 1,
-    TAIL_CALL = 2,
-  };
-
-  // The table will contain these structs that point to the sled, the function
-  // containing the sled, and what kind of sled (and whether they should always
-  // be instrumented).
-  struct XRayFunctionEntry {
-    const MCSymbol *Sled;
-    const MCSymbol *Function;
-    SledKind Kind;
-    bool AlwaysInstrument;
-    const class Function *Fn;
-  };
-
-  // All the sleds to be emitted.
-  std::vector<XRayFunctionEntry> Sleds;
-
   // All instructions emitted by the X86AsmPrinter should use this helper
   // method.
   //
   // This helper function invokes the SMShadowTracker on each instruction before
   // outputting it to the OutStream. This allows the shadow tracker to minimise
   // the number of NOPs used for stackmap padding.
   void EmitAndCountInstruction(MCInst &Inst);
   void LowerSTACKMAP(const MachineInstr &MI);
void LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr &MI,
                                      X86MCInstLower &MCIL);
   void LowerPATCHABLE_RET(const MachineInstr &MI, X86MCInstLower &MCIL);
   void LowerPATCHABLE_TAIL_CALL(const MachineInstr &MI, X86MCInstLower &MCIL);
 
   // Helper function that emits the XRay sleds we've collected for a particular
   // function.
   void EmitXRayTable();
 
-  // Helper function to record a given XRay sled.
-  void recordSled(MCSymbol *Sled, const MachineInstr &MI, SledKind Kind);
 public:
   explicit X86AsmPrinter(TargetMachine &TM,
                          std::unique_ptr<MCStreamer> Streamer)
       : AsmPrinter(TM, std::move(Streamer)), SM(*this), FM(*this) {}
 
   const char *getPassName() const override {
     return "X86 Assembly / Object Emitter";
   }

lib/Target/X86/X86MCInstLower.cpp

void X86AsmPrinter::LowerPATCHPOINT(const MachineInstr &MI,
   unsigned NumBytes = opers.getNumPatchBytes();
   assert(NumBytes >= EncodedBytes &&
          "Patchpoint can't request size less than the length of a call.");
 
   EmitNops(*OutStreamer, NumBytes - EncodedBytes, Subtarget->is64Bit(),
            getSubtargetInfo());
 }
 
-void X86AsmPrinter::recordSled(MCSymbol *Sled, const MachineInstr &MI,
-                               SledKind Kind) {
-  auto Fn = MI.getParent()->getParent()->getFunction();
-  auto Attr = Fn->getFnAttribute("function-instrument");
-  bool AlwaysInstrument =
-      Attr.isStringAttribute() && Attr.getValueAsString() == "xray-always";
-  Sleds.emplace_back(
-      XRayFunctionEntry{Sled, CurrentFnSym, Kind, AlwaysInstrument, Fn});
-}
-
 void X86AsmPrinter::LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr &MI,
                                                   X86MCInstLower &MCIL) {
   // We want to emit the following pattern:
   //
   //   .p2align 1, ...
   // .Lxray_sled_N:
   //   jmp .tmpN
   //   # 9 bytes worth of noops

test/CodeGen/ARM/xray-attribute-instrumentation.ll

+; RUN: llc -filetype=asm -o - -mtriple=arm-unknown-linux-gnu < %s | FileCheck %s
+
+define i32 @foo() nounwind noinline uwtable "function-instrument"="xray-always" {
+; CHECK-LABEL: Lxray_sled_0:
+; CHECK-NEXT:  .ascii "\005\000\000\352"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-LABEL: Ltmp0:
+  ret i32 0
+; CHECK-LABEL: Lxray_sled_1:
+; CHECK-NEXT:  .ascii "\005\000\000\352"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-NEXT:  .ascii  "\000\360 \343"
+; CHECK-LABEL: Ltmp1:
+; CHECK-NEXT:  mov pc, lr
+}