Merge pull request #204 from trailofbits/another_atomic_check

Adds possibly missing check to mark an instruction as atomic

remill/Arch/Runtime/Intrinsics.cpp

@@ -63,6 +63,37 @@ extern "C" void __remill_intrinsics(void) {
   USED(__remill_atomic_begin);
   USED(__remill_atomic_end);
 
+  // Atomic intrinsics
+  USED(__remill_compare_exchange_memory_8);
+  USED(__remill_compare_exchange_memory_16);
+  USED(__remill_compare_exchange_memory_32);
+  USED(__remill_compare_exchange_memory_64);
+
+  USED(__remill_fetch_and_add_8);
+  USED(__remill_fetch_and_add_16);
+  USED(__remill_fetch_and_add_32);
+  USED(__remill_fetch_and_add_64);
+
+  USED(__remill_fetch_and_sub_8);
+  USED(__remill_fetch_and_sub_16);
+  USED(__remill_fetch_and_sub_32);
+  USED(__remill_fetch_and_sub_64);
+
+  USED(__remill_fetch_and_or_8);
+  USED(__remill_fetch_and_or_16);
+  USED(__remill_fetch_and_or_32);
+  USED(__remill_fetch_and_or_64);
+
+  USED(__remill_fetch_and_and_8);
+  USED(__remill_fetch_and_and_16);
+  USED(__remill_fetch_and_and_32);
+  USED(__remill_fetch_and_and_64);
+
+  USED(__remill_fetch_and_xor_8);
+  USED(__remill_fetch_and_xor_16);
+  USED(__remill_fetch_and_xor_32);
+  USED(__remill_fetch_and_xor_64);
+
   USED(__remill_fpu_exception_test_and_clear);
 
 //  USED(__remill_defer_inlining);

remill/Arch/Runtime/Intrinsics.h

@@ -135,6 +135,107 @@ extern Memory *__remill_atomic_begin(Memory *);
 [[gnu::used, gnu::const]]
 extern Memory *__remill_atomic_end(Memory *);
 
+/* Most memory intrinsics are marked as `[[gnu::const]]` which tells the compiler that they
+ * do not read/write to memory. This permits LLVM to optimize around the intrinsic, without thinking
+ * about it's internals.
+ * The meaning of `[[gnu::const]]` is the function will neither read nor write to the memory. In
+ * This case the previous value of memory at `addr` needs to be communicated back to the program
+ * which will happen by writing back to the refernecs of `expected`. If the function were declared
+ * with `[[gnu::const]]`, the compiler is free to assume that the value of `expected` is not changed
+ * and it will cause the unwanted behavior.
+ *
+ * The `gnu::pure` attribute causes the unwanted behaviour and the argument references updated inside
+ * the function are not visible in the caller functions
+ */
+
+
+[[gnu::used]]
+extern Memory *__remill_compare_exchange_memory_8(Memory *, addr_t addr, uint8_t &expected, uint8_t desired);
+
+[[gnu::used]]
+extern Memory *__remill_compare_exchange_memory_16(Memory *, addr_t addr, uint16_t &expected, uint16_t desired);
+
+[[gnu::used]]
+extern Memory *__remill_compare_exchange_memory_32(Memory *, addr_t addr, uint32_t &expected, uint32_t desired);
+
+[[gnu::used]]
+extern Memory *__remill_compare_exchange_memory_64(Memory *, addr_t addr, uint64_t &expected, uint64_t desired);
+
+[[gnu::used]]
+extern Memory *__remill_compare_exchange_memory_128(Memory *, addr_t addr, uint128_t &expected, uint128_t &desired);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_add_8(Memory *, addr_t addr, uint8_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_add_16(Memory *, addr_t addr, uint16_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_add_32(Memory *, addr_t addr, uint32_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_add_64(Memory *, addr_t addr, uint64_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_sub_8(Memory *, addr_t addr, uint8_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_sub_16(Memory *, addr_t addr, uint16_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_sub_32(Memory *, addr_t addr, uint32_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_sub_64(Memory *, addr_t addr, uint64_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_and_8(Memory *, addr_t addr, uint8_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_and_16(Memory *, addr_t addr, uint16_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_and_32(Memory *, addr_t addr, uint32_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_and_64(Memory *, addr_t addr, uint64_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_or_8(Memory *, addr_t addr, uint8_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_or_16(Memory *, addr_t addr, uint16_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_or_32(Memory *, addr_t addr, uint32_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_or_64(Memory *, addr_t addr, uint64_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_xor_8(Memory *, addr_t addr, uint8_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_xor_16(Memory *, addr_t addr, uint16_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_xor_32(Memory *, addr_t addr, uint32_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_xor_64(Memory *, addr_t addr, uint64_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_nand_8(Memory *, addr_t addr, uint8_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_nand_16(Memory *, addr_t addr, uint16_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_nand_32(Memory *, addr_t addr, uint32_t &value);
+
+[[gnu::used]]
+extern Memory *__remill_fetch_and_nand_64(Memory *, addr_t addr, uint64_t &value);
+
 // Read and modify the floating point exception state of the (virtual) machine
 // that is executing the actual floating point operations.
 //

remill/Arch/Runtime/Operators.h

@@ -465,6 +465,77 @@ MAKE_WRITE_REF(float64_t)
 
 #undef MAKE_WRITE_REF
 
+#define MAKE_CMPXCHG(size, type_prefix, access_suffix) \
+    template <typename T> \
+    ALWAYS_INLINE static bool _CmpXchg( \
+        Memory *&memory, RnW<T> op, type_prefix ## size ## _t &expected, \
+      type_prefix ## size ## _t desired) { \
+      if (*op.val_ref == expected) {\
+        *op.val_ref = desired; \
+        return true; \
+      } else { \
+        expected = *reinterpret_cast<type_prefix ## size ## _t *>(op.val_ref); \
+        return false; \
+      } \
+    } \
+    \
+    template <typename T> \
+    ALWAYS_INLINE static bool _CmpXchg( \
+        Memory *&memory, MnW<T> op, type_prefix ## size ## _t &expected, \
+      type_prefix ## size ## _t desired) { \
+      auto prev_val = expected; \
+      memory = __remill_compare_exchange_memory_ ## access_suffix \
+          (memory, op.addr, expected, desired);\
+      return prev_val == expected; \
+    }
+
+MAKE_CMPXCHG(8, uint, 8)
+MAKE_CMPXCHG(16, uint, 16)
+MAKE_CMPXCHG(32, uint, 32)
+MAKE_CMPXCHG(64, uint, 64)
+MAKE_CMPXCHG(128, uint, 128)
+
+#undef MAKE_CMPXCHG
+#define UCmpXchg(op, oldval, newval) _CmpXchg(memory, op, oldval, newval)
+
+#define MAKE_ATOMIC_INTRINSIC(name, intrinsic_name, size, type_prefix, op)\
+  template<typename T> \
+  ALWAYS_INLINE type_prefix ## size ## _t _U ## name( \
+      Memory *&memory, MnW<T> addr, type_prefix ## size ## _t value) { \
+    memory = __remill_ ## intrinsic_name ## _ ## size(memory, addr.addr, value); \
+    return value; \
+  } \
+  \
+  template<typename T> \
+  ALWAYS_INLINE type_prefix ## size ## _t _U ## name ( \
+      Memory *&memory, RnW<T> addr, type_prefix ## size ## _t value) { \
+    auto prev_value = *reinterpret_cast<type_prefix ## size ## _t *>(addr.val_ref); \
+    *addr.val_ref = prev_value op value; \
+    return prev_value; \
+  }
+
+#define MAKE_ATOMIC(name, intrinsic_name, op)  \
+    MAKE_ATOMIC_INTRINSIC(name, intrinsic_name, 8, uint, op) \
+    MAKE_ATOMIC_INTRINSIC(name, intrinsic_name, 16, uint, op) \
+    MAKE_ATOMIC_INTRINSIC(name, intrinsic_name, 32, uint, op) \
+    MAKE_ATOMIC_INTRINSIC(name, intrinsic_name, 64, uint, op) \
+
+MAKE_ATOMIC(FetchAdd, fetch_and_add, +)
+MAKE_ATOMIC(FetchSub, fetch_and_sub, -)
+MAKE_ATOMIC(FetchOr, fetch_and_or, |)
+MAKE_ATOMIC(FetchAnd, fetch_and_and, &)
+MAKE_ATOMIC(FetchXor, fetch_and_xor, ^)
+
+#undef MAKE_ATOMIC
+#undef MAKE_ATOMIC_INTRINSIC
+
+#define UFetchAdd(op1, op2) _UFetchAdd(memory, op1, op2)
+#define UFetchSub(op1, op2) _UFetchSub(memory, op1, op2)
+#define UFetchOr(op1, op2)  _UFetchOr(memory, op1, op2)
+#define UFetchAnd(op1, op2) _UFetchAnd(memory, op1, op2)
+#define UFetchXor(op1, op2) _UFetchXor(memory, op1, op2)
+
+
 // For the sake of esthetics and hiding the small-step semantics of memory
 // operands, we use this macros to implicitly pass in the `memory` operand,
 // which we know will be defined in semantics functions.

remill/Arch/X86/Arch.cpp

@@ -936,7 +936,8 @@ bool X86Arch::DecodeInstruction(
   // Wrap an instruction in atomic begin/end if it accesses memory with RMW
   // semantics or with a LOCK prefix.
   if (xed_operand_values_get_atomic(xedd) ||
-      xed_operand_values_has_lock_prefix(xedd)) {
+      xed_operand_values_has_lock_prefix(xedd) ||
+      XED_CATEGORY_SEMAPHORE == xed_decoded_inst_get_category(xedd)) {
     inst.is_atomic_read_modify_write = true;
   }
 

remill/Arch/X86/Semantics/SEMAPHORE.cpp

@@ -19,14 +19,14 @@ namespace {
 #define MAKE_CMPXCHG_XAX(xax) \
     template <typename D, typename S1, typename S2> \
     DEF_SEM(CMPXCHG_ ## xax, D dst, S1 src1, S2 src2) { \
-      auto curr_val = Read(src1); \
       auto desired_val = Read(src2); \
       auto check_val = Read(REG_ ## xax); \
-      auto cmp_res = USub(check_val, curr_val); \
-      auto replace = UCmpEq(cmp_res, 0); \
-      WriteFlagsAddSub<tag_sub>(state, check_val, curr_val, cmp_res); \
-      WriteZExt(dst, Select(replace, desired_val, curr_val)); \
-      WriteZExt(REG_ ## xax, Select(replace, check_val, curr_val)); \
+      auto prev_value = check_val; \
+      auto swap_flag = UCmpXchg(dst, check_val, desired_val); \
+      auto sub_res = USub(prev_value, check_val); \
+      WriteFlagsAddSub<tag_sub>(state, prev_value, check_val, sub_res); \
+      Write(FLAG_ZF, swap_flag);\
+      WriteZExt(REG_ ## xax, check_val); \
       return memory; \
     }
 
@@ -36,37 +36,31 @@ MAKE_CMPXCHG_XAX(EAX)
 IF_64BIT(MAKE_CMPXCHG_XAX(RAX))
 
 DEF_SEM(DoCMPXCHG8B_MEMq, M64W dst, M64 src1) {
-  auto curr_val = Read(src1);
   auto xdx = Read(REG_EDX);
   auto xax = Read(REG_EAX);
   auto xcx = Read(REG_ECX);
   auto xbx = Read(REG_EBX);
   auto desired_val = UOr(UShl(ZExt(xcx), 32), ZExt(xbx));
   auto check_val = UOr(UShl(ZExt(xdx), 32), ZExt(xax));
-  auto cmp_res = USub(check_val, curr_val);
-  auto replace = UCmpEq(cmp_res, 0);
-  Write(FLAG_ZF, replace);
-  Write(dst, Select(replace, desired_val, curr_val));
-  Write(REG_EDX, Select(replace, xdx, Trunc(UShr(curr_val, 32))));
-  Write(REG_EAX, Select(replace, xax, Trunc(curr_val)));
+  auto swap_flag = UCmpXchg(dst, check_val, desired_val);
+  Write(FLAG_ZF, swap_flag);
+  Write(REG_EDX, Trunc(UShr(check_val, 32)));
+  Write(REG_EAX, Trunc(check_val));
   return memory;
 }
 
 #if 64 == ADDRESS_SIZE_BITS
 DEF_SEM(DoCMPXCHG16B_MEMdq, M128W dst, M128 src1) {
-  auto curr_val = Read(src1);
   auto xdx = Read(REG_RDX);
   auto xax = Read(REG_RAX);
   auto xcx = Read(REG_RCX);
   auto xbx = Read(REG_RBX);
   auto desired_val = UOr(UShl(ZExt(xcx), 64), ZExt(xbx));
   auto check_val = UOr(UShl(ZExt(xdx), 64), ZExt(xax));
-  auto cmp_res = USub(check_val, curr_val);
-  auto replace = UCmpEq(cmp_res, 0);
-  Write(FLAG_ZF, replace);
-  Write(dst, Select(replace, desired_val, curr_val));
-  Write(REG_RDX, Select(replace, xdx, Trunc(UShr(curr_val, 64))));
-  Write(REG_RAX, Select(replace, xax, Trunc(curr_val)));
+  auto swap_flag = UCmpXchg(dst, check_val, desired_val);
+  Write(FLAG_ZF, swap_flag);
+  Write(REG_RDX, Trunc(UShr(check_val, 64)));
+  Write(REG_RAX, Trunc(check_val));
   return memory;
 }
 #endif  // 64 == ADDRESS_SIZE_BITS
@@ -106,12 +100,11 @@ DEF_SEM(XADD, D1 dst1, S1 src1, D2 dst2, S2 src2) {
     BarrierStoreLoad();
   }
 
-  auto lhs = Read(src1);
   auto rhs = Read(src2);
+  auto lhs = UFetchAdd(dst1, rhs);
   auto sum = UAdd(lhs, rhs);
-  WriteZExt(dst1, sum);
+  WriteFlagsAddSub<tag_add>(state, rhs, lhs, sum);
   WriteZExt(dst2, lhs);
-  WriteFlagsAddSub<tag_add>(state, lhs, rhs, sum);
   return memory;
 }
 

tests/AArch64/Run.cpp

@@ -152,6 +152,71 @@ NEVER_INLINE Memory *__remill_write_memory_f80(Memory *, addr_t, float64_t) {
   __builtin_unreachable();
 }
 
+Memory *__remill_compare_exchange_memory_8(
+    Memory *memory, addr_t addr, uint8_t &expected, uint8_t desired) {
+  expected = __sync_val_compare_and_swap(
+      reinterpret_cast<uint8_t *>(addr), expected, desired);
+  return memory;
+}
+
+Memory *__remill_compare_exchange_memory_16(
+    Memory *memory, addr_t addr, uint16_t &expected, uint16_t desired) {
+  expected =  __sync_val_compare_and_swap(
+      reinterpret_cast<uint16_t *>(addr), expected, desired);
+  return memory;
+}
+
+Memory *__remill_compare_exchange_memory_32(
+    Memory *memory, addr_t addr, uint32_t &expected, uint32_t desired) {
+  expected = __sync_val_compare_and_swap(
+      reinterpret_cast<uint32_t *>(addr), expected, desired);
+  return memory;
+}
+
+Memory *__remill_compare_exchange_memory_64(
+    Memory *memory, addr_t addr, uint64_t &expected, uint64_t desired) {
+  expected = __sync_val_compare_and_swap(
+      reinterpret_cast<uint64_t *>(addr), expected, desired);
+  return memory;
+}
+
+Memory *__remill_compare_exchange_memory_128(
+    Memory *memory, addr_t addr, uint128_t &expected, uint128_t &desired) {
+#ifdef _GXX_EXPERIMENTAL_CXX0X__
+  expected = __sync_val_compare_and_swap(
+      reinterpret_cast<uint128_t *>(addr), expected, desired);
+#endif
+  return memory;
+}
+
+#define MAKE_ATOMIC_INTRINSIC(intrinsic_name, type_prefix, size) \
+  Memory *__remill_ ## intrinsic_name ## _ ## size( \
+      Memory *memory, addr_t addr, type_prefix ## size ## _t &value) { \
+    value = __sync_ ## intrinsic_name(reinterpret_cast<type_prefix ## size ## _t *>(addr), value); \
+    return memory; \
+  } \
+
+MAKE_ATOMIC_INTRINSIC(fetch_and_add, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_add, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_add, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_add, uint, 64)
+MAKE_ATOMIC_INTRINSIC(fetch_and_sub, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_sub, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_sub, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_sub, uint, 64)
+MAKE_ATOMIC_INTRINSIC(fetch_and_or, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_or, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_or, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_or, uint, 64)
+MAKE_ATOMIC_INTRINSIC(fetch_and_and, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_and, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_and, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_and, uint, 64)
+MAKE_ATOMIC_INTRINSIC(fetch_and_xor, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_xor, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_xor, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_xor, uint, 64)
+
 int __remill_fpu_exception_test_and_clear(int read_mask, int clear_mask) {
   auto except = std::fetestexcept(read_mask);
   std::feclearexcept(clear_mask);

tests/X86/Run.cpp

@@ -183,6 +183,91 @@ NEVER_INLINE Memory *__remill_write_memory_f80(
   return memory;
 }
 
+Memory *__remill_compare_exchange_memory_8(
+    Memory *memory, addr_t addr, uint8_t &expected, uint8_t desired) {
+  expected = __sync_val_compare_and_swap(
+      reinterpret_cast<uint8_t *>(addr), expected, desired);
+  return memory;
+}
+
+Memory *__remill_compare_exchange_memory_16(
+    Memory *memory, addr_t addr, uint16_t &expected, uint16_t desired) {
+  expected =  __sync_val_compare_and_swap(
+      reinterpret_cast<uint16_t *>(addr), expected, desired);
+  return memory;
+}
+
+Memory *__remill_compare_exchange_memory_32(
+    Memory *memory, addr_t addr, uint32_t &expected, uint32_t desired) {
+  expected = __sync_val_compare_and_swap(
+      reinterpret_cast<uint32_t *>(addr), expected, desired);
+  return memory;
+}
+
+Memory *__remill_compare_exchange_memory_64(
+    Memory *memory, addr_t addr, uint64_t &expected, uint64_t desired) {
+  expected = __sync_val_compare_and_swap(
+      reinterpret_cast<uint64_t *>(addr), expected, desired);
+  return memory;
+}
+
+Memory *__remill_compare_exchange_memory_128(
+    Memory *memory, addr_t addr, uint128_t &expected, uint128_t &desired) {
+#if !(defined(__x86_64__) || defined(__i386__) || defined(_M_X86))
+  expected = __sync_val_compare_and_swap(
+      reinterpret_cast<uint128_t *>(addr), expected, desired);
+#else
+  bool result;
+  struct alignas(16) uint128 {
+    uint64_t lo;
+    uint64_t hi;
+  };
+
+  uint128 *oldval = reinterpret_cast<uint128 *>(&expected);
+  uint128 *newval = reinterpret_cast<uint128 *>(&desired);
+
+  __asm__ __volatile__(
+      "lock; cmpxchg16b %0; setz %1"
+      : "=m"(*reinterpret_cast<uint128_t *>(addr)), "=q"(result)
+      : "m"(*reinterpret_cast<uint128_t *>(addr)), "d" (oldval->hi), "a" (oldval->lo),
+        "c" (newval->hi), "b" (newval->lo)
+      : "memory");
+
+  if(!result) {
+    expected = *reinterpret_cast<uint128_t*>(addr);
+  }
+#endif
+  return memory;
+}
+
+#define MAKE_ATOMIC_INTRINSIC(intrinsic_name, type_prefix, size) \
+  Memory *__remill_ ## intrinsic_name ## _ ## size( \
+      Memory *memory, addr_t addr, type_prefix ## size ## _t &value) { \
+    value = __sync_ ## intrinsic_name(reinterpret_cast<type_prefix ## size ## _t *>(addr), value); \
+    return memory; \
+  } \
+
+MAKE_ATOMIC_INTRINSIC(fetch_and_add, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_add, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_add, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_add, uint, 64)
+MAKE_ATOMIC_INTRINSIC(fetch_and_sub, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_sub, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_sub, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_sub, uint, 64)
+MAKE_ATOMIC_INTRINSIC(fetch_and_or, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_or, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_or, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_or, uint, 64)
+MAKE_ATOMIC_INTRINSIC(fetch_and_and, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_and, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_and, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_and, uint, 64)
+MAKE_ATOMIC_INTRINSIC(fetch_and_xor, uint, 8)
+MAKE_ATOMIC_INTRINSIC(fetch_and_xor, uint, 16)
+MAKE_ATOMIC_INTRINSIC(fetch_and_xor, uint, 32)
+MAKE_ATOMIC_INTRINSIC(fetch_and_xor, uint, 64)
+
 int __remill_fpu_exception_test_and_clear(int read_mask, int clear_mask) {
   auto except = std::fetestexcept(read_mask);
   std::feclearexcept(clear_mask);