================ Comment at: CMakeLists.txt:210 @@ -209,2 +209,3 @@ append_list_if(COMPILER_RT_HAS_FNO_STACK_PROTECTOR_FLAG -fno-stack-protector SANITIZER_COMMON_CFLAGS) +append_list_if(COMPILER_RT_HAS_FNO_SAFE_STACK_FLAG -fno-safe-stack SANITIZER_COMMON_CFLAGS) append_list_if(COMPILER_RT_HAS_FVISIBILITY_HIDDEN_FLAG -fvisibility=hidden SANITIZER_COMMON_CFLAGS) ---------------- kcc wrote: > is this still valid? > Don't we use -fsanitize= syntax? Updated.
================ Comment at: lib/safestack/safestack.cc:38 @@ +37,3 @@ +/// FIXME: is this in some header? +#define STACK_ALIGN 16 + ---------------- kcc wrote: > please no macros where constants work Done ================ Comment at: lib/safestack/safestack.cc:45 @@ +44,3 @@ + +// define MAP_STACK if undefined, this flag is used as a hint +#ifndef MAP_STACK ---------------- kcc wrote: > do you really need this? Removed; they are hints and are not required for correctness. We can bring them back if benchmarks show a clear benefit. ================ Comment at: lib/safestack/safestack.cc:57 @@ +56,3 @@ +// all symbols from pthread that we use dynamically +#define __DECLARE_WRAPPER(fn) static __typeof__(fn)* __d_ ## fn = NULL; + ---------------- kcc wrote: > Mmm. I don't like that. > Why can't we just ensure that pthread is linked? My understanding is that on some platforms (FreeBSD?) linking the pthread library has a performance impact. We could probably consider moving pthread stuff to a separate library which is only linked when pthread is linked. I guess for now we can use the pthread symbols unconditionally. Then any later change will mostly be a matter of moving code around. ================ Comment at: lib/safestack/safestack.cc:80 @@ +79,3 @@ + +#define __GET_UNSAFE_STACK_PTR() __safestack_unsafe_stack_ptr +#define __SET_UNSAFE_STACK_PTR(value) __safestack_unsafe_stack_ptr = (value) ---------------- kcc wrote: > no macros, please, unless you have too, in which case describe in comments > why. Removed ================ Comment at: lib/safestack/safestack.cc:101 @@ +100,3 @@ + // (such overrides might crash is they use the unsafe stack themselves) + void *addr = (void *)internal_mmap( + NULL, size + guard, PROT_WRITE | PROT_READ, ---------------- kcc wrote: > I would use MmapOrDie (or create a similar one in > sanitizer_common/sanitizer_posix.cc) > and get rid of mmap includes in this file. Done ================ Comment at: lib/safestack/safestack.cc:114 @@ +113,3 @@ + void* stack_ptr = (char*) start + size; + assert((((size_t)stack_ptr) & (STACK_ALIGN-1)) == 0); + ---------------- kcc wrote: > use CHECK_EQ Done ================ Comment at: lib/safestack/safestack.cc:126 @@ +125,3 @@ + // (such overrides might crash is they use the unsafe stack themselves) + (void)internal_munmap( + (char*) __GET_UNSAFE_STACK_START() - __GET_UNSAFE_STACK_GUARD(), ---------------- kcc wrote: > UnmapOrDie Done ================ Comment at: lib/safestack/safestack.cc:204 @@ +203,3 @@ + + assert(size != 0); + assert((size & (STACK_ALIGN-1)) == 0); ---------------- kcc wrote: > use CHECK Done ================ Comment at: lib/safestack/safestack.cc:276 @@ +275,3 @@ + +#ifdef __ELF__ +// Run safestack initialization before any other constructors. ---------------- kcc wrote: > please make this similar to other sanitizers > (SANITIZER_CAN_USE_PREINIT_ARRAY). Done ================ Comment at: test/safestack/CMakeLists.txt:6 @@ +5,3 @@ +if(NOT COMPILER_RT_STANDALONE_BUILD) + list(APPEND SAFESTACK_TEST_DEPS safestack clang) +endif() ---------------- samsonov wrote: > Looks like clang is already contained in SANITIZER_COMMON_LIT_TEST_DEPS Done http://reviews.llvm.org/D6096 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
