[PATCH] D23933: [XRay] ARM 32-bit no-Thumb support in compiler-rt

[Dean Michael Berris via llvm-commits]

dberris accepted this revision.
dberris added a comment.
This revision is now accepted and ready to land.

I'll defer to someone else who understands the ARM assembly parts.

You might also consider extending the file header type to indicate what platform the trace was generated from, so tools can determine what to do with a trace that comes from a specific CPU. I'm happy to do it later, but wanted to know your thoughts on how we might encode that information appropriately.


================
Comment at: lib/xray/xray_arm.cc:29-32
@@ +28,6 @@
+// The machine codes for some instructions used in runtime patching.
+enum class PatchOpcodes : uint32_t
+{
+  PO_Push_r0_lr = 0xE92D4001,
+  PO_Blx_ip = 0xE12FFF3C,
+  PO_Pop_r0_lr = 0xE8BD4001,
----------------
I think the coding standards say they should look like types, so just CamelCase. I don't make up the rules, but I just try to follow them. :)

================
Comment at: lib/xray/xray_arm.cc:38
@@ +37,3 @@
+// 0xUUUUWXYZ -> 0x000W0XYZ
+inline static uint32_t GetMovwMask(const uint32_t Value) {
+  return (Value & 0xfff) | ((Value & 0xf000) << 4);
----------------
Shouldn't these functions be camelCase? As in `getMoveMask(...)` according to the guide? I understand you're just following the conventions of the files around, and those mistakes are mine -- but do you mind changing them before landing?

================
Comment at: lib/xray/xray_inmemory_log.cc:30
@@ +29,3 @@
+#elif defined(__arm__)
+  static const int64_t cNanosecondsPerSecond = 1000LL*1000*1000;
+#else
----------------
`static constexpr` instead?

Also, please follow the naming conventions for this one too.

Another thing -- couldn't you just use std::chrono for the constant here?

http://en.cppreference.com/w/cpp/chrono/duration


https://reviews.llvm.org/D23933