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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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})
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106919