[PATCH] D149002: [compiler-rt][interception][win] Improve error reporting

Fri Apr 28 11:25:47 PDT 2023

vitalybuka added inline comments.

================
Comment at: compiler-rt/lib/interception/interception_win.cpp:130
 #if SANITIZER_WINDOWS
+#include "sanitizer_common/sanitizer_common.h"
+#include "sanitizer_common/sanitizer_libc.h"
----------------
alvinhochun wrote:
> vitalybuka wrote:
> > We want to avoid dependency of interceptors on most of sanitizer_common
> > For most users these messages is just a spam, they unlikely will rebuild and fix sanitizers.
> > So it's rather helpful but unnecessary debug leftovers for folks like us.
> > 
> > I prefer we don't land this one and dependent patch.
> The behaviour of interception_win when it encounters an unknown instruction, before this patch, is to terminate the process without printing any messages, which is very unhelpful and requires more effort than it should to diagnose. One of the changes in this patch is to make it not terminate the process, but to fail the interception like it would for any other reasons, so that if the functions it fails to intercept are not critical to the operation of asan it can still run with somewhat reduced coverage.
> 
> For most users actually these messages should not show up at all. If they show up for any users running on Windows then it means we do have a real problem that needs to be fixed. The point of these messages is, if the users file an issue with the output included it would be much easier for someone to come up with a patch because they can just check the printed instruction bytes, instead of having to debug on the affected systems.
> 
> Note that I wrote "running on Windows" -- IIRC asan-instrumented programs currently doesn't run on Wine because the functions have different instructions at the start that interception_win currently doesn't support (@mstorsjo may be able to confirm this). With this patch users can get a better idea what is wrong (or file a better bug report), and we can again fix the issues more easily with the printed bytes. Others may also start running tests on CIs under Wine and be able to report or help fix these issues in case new unsupported instructions comes up.
> 
> Also, I would really like this patch to land together with D148991. In that patch we start hooking `libc++.dll` and `libunwind.dll` for i686 MinGW. There is a chance Clang may generate different instructions and I would hate for asan to break because of it. We do have nightly checks in llvm-mingw to help catch this, and again the additional output messages can help us fix the issue more easily.
> 
> At least that's what I think. The dependency on sanitizer_common is unfortunate but I don't know how it can be avoided. Though it is only limited to Windows, can this be acceptable?
> The behavior of interception_win when it encounters an unknown instruction, before this patch, is to terminate the process without printing any messages, which is very unhelpful and requires more effort than it should to diagnose. One of the changes in this patch is to make it not terminate the process, but to fail the interception like it would for any other reasons, so that if the functions it fails to intercept are not critical to the operation of asan it can still run with somewhat reduced coverage.

This part of change LGTM. 
BTW would be nice to keep Report and "make non-fatal" in separate patches.

> 
> For most users actually these messages should not show up at all. If they show up for any users running on Windows then it means we do have a real problem that needs to be fixed. The point of these messages is, if the users file an issue with the output included it would be much easier for someone to come up with a patch because they can just check the printed instruction bytes, instead of having to debug on the affected systems.

So even less reasons to introduce this dependency?

> 
> Note that I wrote "running on Windows" -- IIRC asan-instrumented programs currently doesn't run on Wine because the functions have different instructions at the start that interception_win currently doesn't support (@mstorsjo may be able to confirm this). With this patch users can get a better idea what is wrong (or file a better bug report), and we can again fix the issues more easily with the printed bytes. Others may also start running tests on CIs under Wine and be able to report or help fix these issues in case new unsupported instructions comes up.

We kind of have this functionality in llvm-project/compiler-rt/lib/asan/asan_interceptors.h:140
maybe we can extend to:

```
VReport(1, "AddressSanitizer: failed to intercept '%s, details: %s'\n", #name, getLastInterceptionError());
```

Another way to break dependency is to call from asan:
```
void __interception::SetOnErrorCallback(void(*)(const char*, uptr addr));
``

> 
> Also, I would really like this patch to land together with D148991. In that patch we start hooking `libc++.dll` and `libunwind.dll` for i686 MinGW. There is a chance Clang may generate different instructions and I would hate for asan to break because of it. We do have nightly checks in llvm-mingw to help catch this, and again the additional output messages can help us fix the issue more easily.
> 
> At least that's what I think. The dependency on sanitizer_common is unfortunate but I don't know how it can be avoided. Though it is only limited to Windows, can this be acceptable?

This is windows only, however there is stuff like e.g. safestack which depends on interception but not on common. I don't think we have that on Windows, but you are making this a problem for future committers who decide to support that.

================
Comment at: compiler-rt/lib/interception/tests/CMakeLists.txt:109
     RUNTIME ${INTERCEPTION_COMMON_LIB}
-    SOURCES ${INTERCEPTION_UNITTESTS} ${COMPILER_RT_GTEST_SOURCE}
+    SOURCES ${INTERCEPTION_UNITTESTS} ${COMPILER_RT_GTEST_SOURCE} ${INTERCEPTION_TEST_GMOCK_SOURCE}
     COMPILE_DEPS ${INTERCEPTION_TEST_HEADERS}
----------------
maybe add gmock on all platforms to have less conditions in CMake?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149002