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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka accepted this revision.
vitalybuka added inline comments.
This revision is now accepted and ready to land.


================
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],
----------------
we use lowercase for hex in sanitizers


================
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:
> > > > 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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149002