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

[Serge Rogatch via llvm-commits]

rSerge added a comment.

In https://reviews.llvm.org/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`?


================
Comment at: lib/CodeGen/XRayInstrumentation.cpp:126
@@ +125,3 @@
+  switch (MF.getTarget().getTargetTriple().getArch()) {
+  // List here the architectures which don't have a single return instruction
+  case Triple::ArchType::arm:
----------------
Moving the comments towards the function calls.

================
Comment at: lib/Target/ARM/ARMAsmPrinter.cpp:1983
@@ +1982,3 @@
+  case ARM::PATCHABLE_FUNCTION_ENTER:
+  {
+    LowerPATCHABLE_FUNCTION_ENTER(*MI);
----------------
Removing.

================
Comment at: lib/Target/ARM/ARMMCInstLower.cpp:181
@@ +180,3 @@
+  //
+  //   PUSH{ r0, lr }
+  //   MOVW r0, #<lower 16 bits of function ID>
----------------
We save the other registers in the trampoline (`__xray_FunctionEntry` and `__xray_FunctionExit` assembly functions).

================
Comment at: lib/Target/ARM/ARMMCInstLower.cpp:187
@@ +186,3 @@
+  //   BLX ip
+  //   POP{ r0, lr }
+  //
----------------
`POP` is intended to execute after return from the subroutine, which `BLX` calls.


https://reviews.llvm.org/D23931