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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM, but please take the suggested edit to update a documentation URL.



================
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)];
----------------
URL update in case the redirect goes bad in the future.


================
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;
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91853