[PATCH] D118159: [CMake][MSVC] Add include path to crt headers when enabling sanitizers.

[pierre gousseau via Phabricator via llvm-commits]

pgousseau added inline comments.


================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:818
+      endif()
+      include_directories(SYSTEM ${MSVC_CRT_SRC_DIR})
+    endif()
----------------
cbezault wrote:
> rnk wrote:
> > I sent the previous comment too early, I see this now.
> > 
> > @cbezault , is there a reason MSVC doesn't ship asan_interface.h with the other includes? I imagine there could be some bad interaction if clang-cl finds the wrong asan_interface.h (MSVC's instead its own from compiler-rt), but clang puts its resource directory onto the search path first, so this should work.
> > 
> > If the usage if asan_interface.h is to call the LSan APIs, I think it would be reasonable to still use `__has_include`, since LSan support is new and somewhat optional. I would rather not add all internal crt sources to the header search paths used for all compiles in LLVM.
> This is not something that I've looked at. @stwish_msft if you want to comment.
Thank you for reviewing!
We seem to include `asan_interface.h` to use asan manual poisoning in `Allocator.h` 

So, it seems we would lose some Asan coverage if not including `asan_interface.h`

With this change I mean to only affect `cl.exe` builds and not `clang-cl`, let me know if this is not the case.

I have a separate patch for an lsan api msvc issue at https://reviews.llvm.org/D118162



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118159