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

[Alvin Wong via Phabricator via llvm-commits]

alvinhochun 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"
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149002