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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka requested changes to this revision.
vitalybuka added inline comments.
This revision now requires changes to proceed.


================
Comment at: compiler-rt/lib/asan/asan_win.cpp:162
 void InitializePlatformInterceptors() {
+  __interception::SetErrorReportCallback(Report);
+
----------------
Maybe VReport?


================
Comment at: compiler-rt/lib/interception/interception_win.cpp:687-688
+  ReportError(
+      "interception_win: unhandled instruction at %p: %02X %02X %02X %02X %02X "
+      "%02X %02X %02X\n",
+      (void *)address, bytes[0], bytes[1], bytes[2], bytes[3], bytes[4],
----------------
alvinhochun wrote:
> vitalybuka wrote:
> > we use lowercase for hex in sanitizers
> Sure, I can change it before committing if you are fine with that.
Yes, please change.


================
Comment at: compiler-rt/lib/interception/interception_win.cpp:157-161
+#  define Report(...)                     \
+    do {                                  \
+      if (ErrorReportCallback)            \
+        ErrorReportCallback(__VA_ARGS__); \
+    } while (0)
----------------
alvinhochun wrote:
> vitalybuka wrote:
> > alvinhochun wrote:
> > > vitalybuka wrote:
> > > > alvinhochun wrote:
> > > > > vitalybuka wrote:
> > > > > > Can we avoid a macro?
> > > > > > static inline should work?
> > > > > > 
> > > > > > Can you call it ReportError, to avoid confusion for Report in common
> > > > > The problem is calling a variadic function, there is no way that I know of to just forward the arguments without using `va_list`. There is currently not a version of `__sanitizer::Report` that takes `va_list`, so I would have to add one, which is more work than just using a macro here.
> > > > > 
> > > > > I can rename the macro to `ReportError`, yes.
> > > > This should work https://en.cppreference.com/w/cpp/language/parameter_pack
> > > > 
> > > > ```
> > > > template<class... Ts>
> > > > void ReportError(Ts... args) {
> > > >       if (ErrorReportCallback)            
> > > >         ErrorReportCallback(args...); 
> > > > }
> > > > ```
> > > Ah , didn't know we can do this. However, if I take this approach then we will lose the printf format check warnings provided by `__attribute__((format))` (doesn't work when applied to the template function with parameter pack). I think these checks are quite worth having.
> > I don's see why is that? When template instantiated all types are known to call ErrorReportCallback
> > 
> > If it's broken, my preference is to rather abandon format check, as we have just a few calls. but avoid macro.
> > 
> > However, because it's just 2 calls, I don't see a problem 
> > removing ReportError and inlining calls
> > 
> > 
> Variadic arguments are converted using default argument promotions, but template arguments don't get this promotion which confuses the format checks. There are also a couple of GCC-compat warnings.
> 
> ```
> warning: GCC does not allow 'format' attribute in this position on a function definition [-Wgcc-compat]
> warning: GCC requires a function with the 'format' attribute to be variadic [-Wgcc-compat]
> warning: format specifies type 'unsigned int' but the argument has type 'u8' (aka 'unsigned char') [-Wformat]
> ```
> 
> I'm curious, why should we avoid this macro? `VReport` and `VPrintf` in `sanitizer_common.h` are similar macros, and they make use of `Printf` and `Report` which also have the format check attributes. In my opinion the alternatives to having this macro are worse. Do note that the child patch adds two more usages of `ReportError`.
> Variadic arguments are converted using default argument promotions, but template arguments don't get this promotion which confuses the format checks. There are also a couple of GCC-compat warnings.
> 
> ```
> warning: GCC does not allow 'format' attribute in this position on a function definition [-Wgcc-compat]
> warning: GCC requires a function with the 'format' attribute to be variadic [-Wgcc-compat]
> warning: format specifies type 'unsigned int' but the argument has type 'u8' (aka 'unsigned char') [-Wformat]
> ```
> 
> I'm curious, why should we avoid this macro? `VReport` and `VPrintf` in `sanitizer_common.h` are similar macros, and they make use of `Printf` and `Report` which also have the format check attributes. In my opinion the alternatives to having this macro are worse. Do note that the child patch adds two more usages of `ReportError`.

Reasons are harder to debug, bad stack traces, weird compilation error.
I don't see reasons to have them there as well.

Either way I don't think the issue is important, with or without macro is OK to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149002