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

[Renato Golin via llvm-commits]

rengolin requested changes to this revision.
rengolin added a comment.
This revision now requires changes to proceed.

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.


================
Comment at: lib/CodeGen/XRayInstrumentation.cpp:46-53
@@ +45,10 @@
+  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);
----------------
dberris wrote:
> 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?
Agreed. Probably move the separate comments to their implementations?

================
Comment at: lib/CodeGen/XRayInstrumentation.cpp:92
@@ +91,3 @@
+          TII->get(TargetOpcode::PATCHABLE_FUNCTION_EXIT));
+        break; //FIXME: is this correct? Can't a MachineBasicBlock have multiple return instructions?
+      }
----------------
Good point. Probably not correct.

================
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:
----------------
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.

================
Comment at: lib/Target/ARM/ARMAsmPrinter.cpp:1983
@@ +1982,3 @@
+  case ARM::PATCHABLE_FUNCTION_ENTER:
+  {
+    LowerPATCHABLE_FUNCTION_ENTER(*MI);
----------------
No need for braces if you're not declaring variables.

================
Comment at: lib/Target/ARM/ARMMCInstLower.cpp:158
@@ +157,3 @@
+
+// FIXME: Is there a ready way to emit NOPs on ARM?
+static void Emit4ByteNops(MCStreamer& OS, int NumInstructions, const MCSubtargetInfo &STI)
----------------
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.

================
Comment at: lib/Target/ARM/ARMMCInstLower.cpp:181
@@ +180,3 @@
+  //
+  //   PUSH{ r0, lr }
+  //   MOVW r0, #<lower 16 bits of function ID>
----------------
Why just save r0? AAPCS can use all four r0-r3 for return results.

================
Comment at: lib/Target/ARM/ARMMCInstLower.cpp:187
@@ +186,3 @@
+  //   BLX ip
+  //   POP{ r0, lr }
+  //
----------------
BLX is unconditional, POP will never be executed. Is that intended?

================
Comment at: lib/Target/ARM/ARMMCInstLower.cpp:198
@@ +197,3 @@
+  // 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());
----------------
Please, don't emit binary directly. Use the builders.


https://reviews.llvm.org/D23931