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

[Alvin Wong via Phabricator via llvm-commits]

alvinhochun added inline comments.


================
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],
----------------
vitalybuka wrote:
> we use lowercase for hex in sanitizers
Sure, I can change it before committing if you are fine with that.


================
Comment at: compiler-rt/lib/interception/interception_win.cpp:157-161
+#  define Report(...)                     \
+    do {                                  \
+      if (ErrorReportCallback)            \
+        ErrorReportCallback(__VA_ARGS__); \
+    } while (0)
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149002