[PATCH] D91853: [compiler-rt] [sanitizer] Silence -Wframe-larger-than= for a few windows functions with large stack buffers

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp:143
 
   // See http://msdn.microsoft.com/en-us/library/ms680578(VS.85).aspx
   char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(CHAR)];
----------------
amccarth wrote:
> URL update in case the redirect goes bad in the future.
Sure, will add that change.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_win.cpp:144
   // See http://msdn.microsoft.com/en-us/library/ms680578(VS.85).aspx
   char buffer[sizeof(SYMBOL_INFO) + MAX_SYM_NAME * sizeof(CHAR)];
   PSYMBOL_INFO symbol = (PSYMBOL_INFO)buffer;
----------------
amccarth wrote:
> mstorsjo wrote:
> > amccarth wrote:
> > > 1.  The sample code used `sizeof(TCHAR)` not `CHAR`.  So I'm guessing this is a potential stack buffer overrun bug for "Unicode" builds.  (Windows defines `CHAR` to `char` as indicated here:  https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types)
> > > 
> > > 2.  Does symbolizing happen on multiple threads?  If not, the buffer could be static, which would keep the frame size small.
> > 1. In https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/ns-dbghelp-symbol_info it looks like the SYMBOL_INFO struct explicitly uses CHAR and isn't available in an unicode form, so this code seems to be correct in that aspect.
> > 
> > 2. I guess it can happen on multiple threads, for sanitizer error reporting at runtime in cases where error don't terminate the process.
> 1.  Well, there is a wide version called `SYMBOL_INFOW` for use with `SymFromAddrW`, see https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/ns-dbghelp-symbol_infow
> 
> Unlike other Win32 APIs, the "wide" versions seem to be enabled by `DBGHELP_TRANSLATE_TCHAR` rather than `UNICODE`.  LLVM doesn't seem to use that, so I guess this works for now.  Using `sizeof(TCHAR)` now might ease a future transition, but I'll leave it up to you.
> 
> 
I'd hold off of such further refactorings as part of this commit, and just stick to silencing warnings, but thanks for the pointers!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91853