[libc-commits] [PATCH] D106919: [libc][wip] add integration tests for scudo in libc

[Mitch Phillips via Phabricator via libc-commits]

hctim added inline comments.


================
Comment at: libc/lib/CMakeLists.txt:7
 
+if(COMPILER_RT_BUILD_GWP_ASAN)
+  list(APPEND SCUDO_DEPS RTGwpAsan.${LIBC_TARGET_ARCHITECTURE} RTGwpAsanBacktraceLibc.${LIBC_TARGET_ARCHITECTURE} RTGwpAsanSegvHandler.${LIBC_TARGET_ARCHITECTURE})
----------------
sivachandra wrote:
> michaelrj wrote:
> > sivachandra wrote:
> > > michaelrj wrote:
> > > > hctim wrote:
> > > > > sivachandra wrote:
> > > > > > michaelrj wrote:
> > > > > > > sivachandra wrote:
> > > > > > > > hctim wrote:
> > > > > > > > > `COMPILER_RT_HAS_GWP_ASAN`
> > > > > > > > Should this be under `if(LLVM_LIBC_INCLUDE_SCUDO)`?
> > > > > > > I can't make this COMPILER_RT_HAS_GWP_ASAN since that variable won't be defined yet because the llvm libc cmake code runs before the compiler-rt cmake code does.
> > > > > > > 
> > > > > > > If this was under `if(LLVM_LIBC_INCLUDE_SCUDO)` then you couldn't include GWP-ASan without also including SCUDO, and I figured it was better to give the user the option.
> > > > > > Is GWP-ASan useful without something like SCUDO calling into it?
> > > > > That seems weird that libc's full cmake runs before compiler-rt's config-ix.
> > > > > 
> > > > > I think you might run into a problem where you want GWP-ASan, but GWP-ASan isn't supported on your architecture (like mips). In these cases `COMPILER_RT_BUILD_GWP_ASAN == True` but `COMPILER_RT_HAS_GWP_ASAN == False`.
> > > > > 
> > > > > > Is GWP-ASan useful without something like SCUDO calling into it?
> > > > > 
> > > > > Nope, GWP-ASan needs be called from malloc().
> > > > based on this I've moved the `if(COMPILER_RT_BUILD_GWP_ASAN)` inside of the `if(LLVM_LIBC_INCLUDE_SCUDO)`. 
> > > > As for the cmake running order, I'm not actually sure how it ends up working, but I do know that `COMPILER_RT_HAS_GWP_ASAN` wasn't defined when I tried to use it in this case. Could it have something to do with scopes? I'm not very experienced with cmake.
> > > Try setting `COMPILER_RT_HAS_GWP_ASAN` in the `PARENT_SCOPE`?
> > I have now tried that, and it didn't seem to work. I'm not sure why though, given that I think the parent scope should be the one above the `compiler-rt` directory. My guess would be that the problem is the execution order, but if you have other things to try feel free to suggest them.
> I have no more straightforward ideas. But, a question for @hctim - Why do we have code like this in `config-ix.cmake`:
> 
> ```
>   list_intersect(GWP_ASAN_SUPPORTED_ARCH
>     ALL_GWP_ASAN_SUPPORTED_ARCH
>     SANITIZER_COMMON_SUPPORTED_ARCH)
> ```
> 
> The reason I am asking this is because, if GWP-ASan is standalone, why should the supported archs be deduced from what sanitizer common supports? If we can decouple GWP-ASan CMake logic from the rest of compiler-rt logic, we can potentially put GWP-ASan related pieces in a `.cmake` file and include that file from within `config-ix.cmake` and libc CMake files.
It doesn't have a dependency on sanitizer-common, but the files are in compiler-rt for licensing/ease-of-setup/historical reasons.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106919