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

[Etienne Bergeron via llvm-commits]

etienneb added inline comments.

================
Comment at: lib/interception/interception_win.cc:256
@@ +255,3 @@
+static const int kMaxTrampolineRegion = 1024;
+static TrampolineMemoryRegion TrampolineRegions[kMaxTrampolineRegion];
+
----------------
rnk wrote:
> 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.
I do not see why.
I was thinking the patching must occur before threads creation (or after a StopTheWorld).

Otherwise, dynamic rewriting the code will also need critical section.

================
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);
----------------
rnk wrote:
> 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);
Both are fine to me.
I do not think we need to put more into the functor.
Moved to function pointer.


http://reviews.llvm.org/D22111