[PATCH] D137788: [sanitizers] [windows] Correctly override functions with backward jmps

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

Overall, the patch does look reasonable - but it doesn't seem to work as is.



================
Comment at: compiler-rt/lib/interception/interception_win.cpp:741
   if (orig_old_func) {
-    uptr relative_offset = *(u32*)(old_func + 1);
+    ptrdiff_t relative_offset = *(i32 *)(old_func + 1);
     uptr absolute_target = old_func + relative_offset + kJumpInstructionLength;
----------------
There's no `i32` type defined here - there is `s32` though.


================
Comment at: compiler-rt/lib/interception/tests/interception_win_test.cpp:447
+  TestIdentityFunctionPatching(kIdentityCodeWithJumpBackwards, override,
+                               FunctionPrefixNode,
+                               kIdentityCodeWithJumpBackwardsOffset);
----------------
This is a typo here, there's no `FunctionPrefixNode`, it should be `-None` right?

Even with that fixed, the new test doesn't seem to pass for me. Can you verify - that the newly added test does fail before applying the functional fix, and that it passes afterwards?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137788/new/

https://reviews.llvm.org/D137788