[PATCH] D22111: [compiler-rt] Refactor the interception code on windows.

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
Comment at: lib/interception/interception_win.cc:202
@@ +201,3 @@
+static void WriteJumpInstruction(uptr from, uptr target) {
+  ptrdiff_t offset = target - from - kJumpInstructionLength;
+  *(u8*)from = 0xE9;
----------------
Maybe CHECK(offset >= INT_MIN && offset <= INT_MAX) or an equivalent?

================
Comment at: lib/interception/interception_win.cc:210
@@ +209,3 @@
+  *(u8*)from = 0xEB;
+  *(u8*)(from + 1) = (u8)offset;
+}
----------------
Check overflow

================
Comment at: lib/interception/interception_win.cc:222
@@ -48,1 +221,3 @@
+  *(u16*)from = 0x25FF;
+  *(u32*)(from + 2) = offset;
 }
----------------
check overflow

================
Comment at: lib/interception/interception_win.cc:256
@@ +255,3 @@
+static const int kMaxTrampolineRegion = 1024;
+static TrampolineMemoryRegion TrampolineRegions[kMaxTrampolineRegion];
+
----------------
We should protect this with a critical section or some other mutex. The sanitizer_common mutex is probably not accessible from here, so I guess we should use a windows critical section.

================
Comment at: lib/interception/interception_win.cc:369
@@ +368,3 @@
+
+    case 0xE8:  // E8 XX XX XX XX : call <func>
+    case 0xC3:  // C3 : ret
----------------
A comment before this case block that these are all control flow instructions that we can't overwrite, and we indicate failure by returning 0.

================
Comment at: lib/interception/tests/CMakeLists.txt:124-125
@@ -123,4 +123,2 @@
                                   ${TARGET_FLAGS})
-
-
 endmacro()
----------------
This file is otherwise unchanged, so I'd revert this whitespace-only change.

================
Comment at: lib/interception/tests/interception_win_test.cc:43-44
@@ -29,1 +42,4 @@
 #if !SANITIZER_WINDOWS64
+struct TestOverrideFunctionWithDetour {
+  uptr Override(
+      uptr old_func, uptr new_func, uptr *orig_old_func) const {
----------------
I don't think these need to be functors, you can just pass function pointers to TestIdentityFunctionPatching and the other guy.

================
Comment at: lib/interception/tests/interception_win_test.cc:350-352
@@ +349,5 @@
+TEST(Interception, OverrideFunctionWithDetour) {
+  typedef TestOverrideFunctionWithDetour T;
+  FunctionPrefixKind prefix = FunctionPrefixDetour;
+  TestIdentityFunctionPatching<T>(kIdentityCodeWithPrologue, prefix);
+  TestIdentityFunctionPatching<T>(kIdentityCodeWithPushPop, prefix);
----------------
Do you think this is cleaner without the functor? Do you think we'll need to put other stuff in the overrider class?

I think you can use plain function pointers like this:
  typedef void (*Overrider)(uptr, uptr, uptr*);
  Overrider o = &OverrideFunctionWithDetour;
  FunctionPrefixKind prefix = FunctionPrefixDetour;
  TestIdentityFunctionPatching(kIdenti..., prefix, o);
  TestIdentityFunctionPatching(kIdenti..., prefix, o);
  TestIdentityFunctionPatching(kIdenti..., prefix, o);


http://reviews.llvm.org/D22111