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

[Dean Michael Berris via llvm-commits]

dberris requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/sanitizer_common/scripts/gen_dynamic_list.py:54
@@ -52,3 +53,3 @@
   # On PowerPC, nm prints function descriptors from .data section.
-  if os.uname()[4] in ["powerpc", "ppc64"]:
+  if platform.uname()[4] in ["powerpc", "ppc64"]:
     func_symbols += ['D']
----------------
Is this required to make this change work? Or should this really happen as an isolated change?

================
Comment at: lib/xray/xray_arm.cc:28-31
@@ +27,6 @@
+
+static const uint32_t cOpcPush_r0_lr = 0xE92D4001;
+static const uint32_t cOpcBlx_ip = 0xE12FFF3C;
+static const uint32_t cOpcPop_r0_lr = 0xE8BD4001;
+static const uint32_t cOpcB20 = 0xEA000005;
+
----------------
The Coding Standards seem to require that variables be camel case starting with a capital letter.

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

================
Comment at: lib/xray/xray_arm.cc:109
@@ +108,3 @@
+        reinterpret_cast<std::atomic<uint32_t> *>(FirstAddress), cOpcPush_r0_lr,
+        std::memory_order_release);
+  } else {
----------------
On ARM, does `std::memory_order_release` turn into writes that have fences after to ensure they're visible? Or am I confusing ARM for an architecture that only has relaxed memory order semantics?

================
Comment at: lib/xray/xray_interface.cc:30
@@ +29,3 @@
+#if defined(__x86_64__)
+  // FIXME: The actual length is 11 bytes. Why was length 12 passed to mprotect() ?
+  static const int16_t cSledLength = 12;
----------------
Good question. I may have miscounted. We can fix that later, once this lands (or if you can change and test to make sure it doesn't break, I'm fine with it).


https://reviews.llvm.org/D23933