[asan][win][msvc] override new and delete and seperate TUs #68754
Conversation
|
@llvm/pr-subscribers-compiler-rt-sanitizer @llvm/pr-subscribers-platform-windows Author: Charlie Barto (barcharcraz) ChangesMigrated from: https://reviews.llvm.org/D155879, with suggestions applied. PR Description copied from above: Currently asan simply exports each overridden new/delete function from the DLL, this works fine normally, but fails if the user is overriding some, but not all, of these functions. In this case the non-overridden functions still come from the asan DLL, but they can't correctly call the user provided override (for example sized op delete should fall back to scalar op delete, if a scalar op delete is provided). Things were also broken in the static build because all the asan overrides were exported from the same TU, and so if you overrode one but not all of them then you'd get ODR violations. This PR should fix both of these cases, but the static case isn't really tested (and indeed one such test does fail) because linking asan statically basically doesn't work on windows right now with LLVM's version of asan. In fact, while we did fix this in our fork, it was a huge mess and we've now made the dynamic version work in all situations (/MD, /MT, /MDd, /MTd, etc) instead. The following is the description from the internal PR that implemented most of this feature. > Previously, operator new/delete were provided as DLL exports when linking dynamically and wholearchived when linked statically. Both scenarios were broken. When linking statically, the user could not define their own op new/delete, because they were already brought into the link by ASAN. When dynamically linking, if the user provided some but not all of the overloads, new and delete would be partially hooked. For example, if the user defined scalar op delete, but the program then called sized op delete, the sized op delete would still be the version provided by ASAN instead of falling back to the user-defined scalar op delete, like the standard requires. > The change <internal PR number>: ASAN operator new/delete fallbacks in the ASAN libraries fixes this moving all operator new/delete definitions to be statically linked. However, this still won't work if /InferAsanLibs still whole-archives everything since then all the op new/deletes would always be provided by ASAN, which is why these changes are necessary. > With these changes, we will no longer wholearchive all of ASAN and will leave the c++ parts (the op new/delete definitions) to be included as a default library. However, it is also necessary to ensure that the asan library with op new/delete will be searched before the corresponding CRT library with the same op new/delete definitions. To accomplish this, we make sure to add the asan library to the beginning of the default lib list, or move it explicitly to the front if it's already in the list. If the C runtime library is explicitly provided, we make sure to warn the user if the current linker line will result in operator new/delete not being provided by ASAN. Note that the rearrangement of defaultlibs is not in this diff. Patch is 92.19 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/68754.diff 37 Files Affected:
diff --git a/compiler-rt/lib/asan/CMakeLists.txt b/compiler-rt/lib/asan/CMakeLists.txt
index 1bfc6f0c5e37d84..a6052fdda9475a2 100644
--- a/compiler-rt/lib/asan/CMakeLists.txt
+++ b/compiler-rt/lib/asan/CMakeLists.txt
@@ -38,9 +38,40 @@ if (NOT WIN32 AND NOT APPLE)
)
endif()
-set(ASAN_CXX_SOURCES
- asan_new_delete.cpp
- )
+if (WIN32)
+ set(ASAN_CXX_SOURCES asan_win_new_delete.cpp)
+else()
+ set(ASAN_CXX_SOURCES asan_new_delete.cpp)
+endif()
+
+if (APPLE)
+ set(ASAN_SOURCES ASAN_CXX_SOURCES)
+endif()
+
+if (WIN32)
+ set(ASAN_STATIC_IMPLIB_SOURCES
+ asan_win_delete_array_thunk.cpp
+ asan_win_delete_array_align_thunk.cpp
+ asan_win_delete_array_align_nothrow_thunk.cpp
+ asan_win_delete_array_nothrow_thunk.cpp
+ asan_win_delete_array_size_thunk.cpp
+ asan_win_delete_array_size_align_thunk.cpp
+ asan_win_delete_scalar_thunk.cpp
+ asan_win_delete_scalar_align_thunk.cpp
+ asan_win_delete_scalar_align_nothrow_thunk.cpp
+ asan_win_delete_scalar_nothrow_thunk.cpp
+ asan_win_delete_scalar_size_thunk.cpp
+ asan_win_delete_scalar_size_align_thunk.cpp
+ asan_win_new_array_thunk.cpp
+ asan_win_new_array_align_thunk.cpp
+ asan_win_new_array_align_nothrow_thunk.cpp
+ asan_win_new_array_nothrow_thunk.cpp
+ asan_win_new_scalar_thunk.cpp
+ asan_win_new_scalar_align_thunk.cpp
+ asan_win_new_scalar_align_nothrow_thunk.cpp
+ asan_win_new_scalar_nothrow_thunk.cpp
+ )
+endif()
set(ASAN_STATIC_SOURCES
asan_rtl_static.cpp
@@ -83,6 +114,13 @@ SET(ASAN_HEADERS
asan_thread.h
)
+if (WIN32)
+ list(APPEND ASAN_HEADERS
+ asan_win_new_delete_thunk_common.h
+ asan_win_thunk_common.h
+ )
+endif()
+
include_directories(..)
set(ASAN_CFLAGS ${SANITIZER_COMMON_CFLAGS})
@@ -139,6 +177,15 @@ add_compiler_rt_object_libraries(RTAsan_dynamic
CFLAGS ${ASAN_DYNAMIC_CFLAGS}
DEFS ${ASAN_DYNAMIC_DEFINITIONS})
+if (WIN32)
+ add_compiler_rt_object_libraries(RTAsan_static_implib
+ ARCHS ${ASAN_SUPPORTED_ARCH}
+ SOURCES ${ASAN_STATIC_IMPLIB_SOURCES}
+ ADDITIONAL_HEADERS ${ASAN_HEADERS}
+ CFLAGS ${ASAN_CFLAGS} ${NO_DEFAULT_LIBS_OPTION}
+ DEFS ${ASAN_COMMON_DEFINITIONS})
+endif()
+
if(NOT APPLE)
add_compiler_rt_object_libraries(RTAsan
ARCHS ${ASAN_SUPPORTED_ARCH}
@@ -238,13 +285,24 @@ else()
DEFS ${ASAN_COMMON_DEFINITIONS}
PARENT_TARGET asan)
+if(WIN32)
add_compiler_rt_runtime(clang_rt.asan_static
STATIC
ARCHS ${ASAN_SUPPORTED_ARCH}
OBJECT_LIBS RTAsan_static
+ RTAsan_static_implib
CFLAGS ${ASAN_CFLAGS}
DEFS ${ASAN_COMMON_DEFINITIONS}
PARENT_TARGET asan)
+else()
+ add_compiler_rt_runtime(clang_rt.asan_static
+ STATIC
+ ARCHS ${ASAN_SUPPORTED_ARCH}
+ OBJECT_LIBS RTAsan_static
+ CFLAGS ${ASAN_CFLAGS}
+ DEFS ${ASAN_COMMON_DEFINITIONS}
+ PARENT_TARGET asan)
+endif()
add_compiler_rt_runtime(clang_rt.asan-preinit
STATIC
@@ -254,6 +312,13 @@ else()
DEFS ${ASAN_COMMON_DEFINITIONS}
PARENT_TARGET asan)
+
+ if (MSVC AND COMPILER_RT_DEBUG)
+ set(MSVC_DBG_SUFFIX _dbg)
+ else()
+ set(MSVC_DBG_SUFFIX )
+ endif()
+
foreach(arch ${ASAN_SUPPORTED_ARCH})
if (COMPILER_RT_HAS_VERSION_SCRIPT)
add_sanitizer_rt_version_list(clang_rt.asan-dynamic-${arch}
@@ -310,6 +375,27 @@ else()
DEFS ${ASAN_DYNAMIC_DEFINITIONS}
PARENT_TARGET asan)
+ if(WIN32)
+ set_target_properties(clang_rt.asan${MSVC_DBG_SUFFIX}-dynamic-${arch}
+ PROPERTIES ARCHIVE_OUTPUT_NAME clang_rt.asan_dynamic-${arch}_implib
+ ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}")
+
+ add_library(clang_rt.asan-${arch}_implib STATIC)
+ target_link_libraries(clang_rt.asan-${arch}_implib RTAsan_static_implib.${arch})
+ add_dependencies(asan clang_rt.asan-${arch}_implib)
+ add_dependencies(clang_rt.asan-${arch}_implib clang_rt.asan${MSVC_DBG_SUFFIX}-dynamic-${arch})
+ get_compiler_rt_output_dir(${arch} IMPLIB_OUTPUT_DIR)
+ set_target_properties(clang_rt.asan-${arch}_implib
+ PROPERTIES ARCHIVE_OUTPUT_NAME clang_rt.asan${MSVC_DBG_SUFFIX}_dynamic-${arch}
+ ARCHIVE_OUTPUT_DIRECTORY ${IMPLIB_OUTPUT_DIR}
+ STATIC_LIBRARY_OPTIONS "$<TARGET_LINKER_FILE:clang_rt.asan${MSVC_DBG_SUFFIX}-dynamic-${arch}>")
+ get_compiler_rt_install_dir(${arch} IMPLIB_INSTALL_DIR)
+ install(TARGETS clang_rt.asan-${arch}_implib
+ ARCHIVE DESTINATION ${IMPLIB_INSTALL_DIR}
+ LIBRARY DESTINATION ${IMPLIB_INSTALL_DIR}
+ RUNTIME DESTINATION ${IMPLIB_INSTALL_DIR})
+ endif()
+
if (SANITIZER_USE_SYMBOLS AND NOT ${arch} STREQUAL "i386")
add_sanitizer_rt_symbols(clang_rt.asan_cxx
ARCHS ${arch})
diff --git a/compiler-rt/lib/asan/asan_interface.inc b/compiler-rt/lib/asan/asan_interface.inc
index bfc44b46196232f..83c0c1cffb85363 100644
--- a/compiler-rt/lib/asan/asan_interface.inc
+++ b/compiler-rt/lib/asan/asan_interface.inc
@@ -187,3 +187,22 @@ INTERFACE_FUNCTION(__asan_update_allocation_context)
INTERFACE_WEAK_FUNCTION(__asan_default_options)
INTERFACE_WEAK_FUNCTION(__asan_default_suppressions)
INTERFACE_WEAK_FUNCTION(__asan_on_error)
+
+#if SANITIZER_WINDOWS
+INTERFACE_FUNCTION(__asan_delete)
+INTERFACE_FUNCTION(__asan_delete_align)
+INTERFACE_FUNCTION(__asan_delete_array)
+INTERFACE_FUNCTION(__asan_delete_array_align)
+INTERFACE_FUNCTION(__asan_delete_array_size)
+INTERFACE_FUNCTION(__asan_delete_array_size_align)
+INTERFACE_FUNCTION(__asan_delete_size)
+INTERFACE_FUNCTION(__asan_delete_size_align)
+INTERFACE_FUNCTION(__asan_new)
+INTERFACE_FUNCTION(__asan_new_align)
+INTERFACE_FUNCTION(__asan_new_align_nothrow)
+INTERFACE_FUNCTION(__asan_new_array)
+INTERFACE_FUNCTION(__asan_new_array_align)
+INTERFACE_FUNCTION(__asan_new_array_align_nothrow)
+INTERFACE_FUNCTION(__asan_new_array_nothrow)
+INTERFACE_FUNCTION(__asan_new_nothrow)
+#endif // SANITIZER_WINDOWS
diff --git a/compiler-rt/lib/asan/asan_stack.h b/compiler-rt/lib/asan/asan_stack.h
index 02a76af847ae6e4..bc1b676e3d45ba6 100644
--- a/compiler-rt/lib/asan/asan_stack.h
+++ b/compiler-rt/lib/asan/asan_stack.h
@@ -47,6 +47,21 @@ u32 GetMallocContextSize();
fast, max_size); \
}
+#define GET_STACK_TRACE_EXPLICIT(max_size, fast, pc, bp, caller_pc, \
+ extra_context) \
+ UNINITIALIZED __sanitizer::BufferedStackTrace stack; \
+ if (max_size <= 2) { \
+ stack.size = max_size; \
+ if (max_size > 0) { \
+ stack.top_frame_bp = bp; \
+ stack.trace_buffer[0] = pc; \
+ if (max_size > 1) \
+ stack.trace_buffer[1] = caller_pc; \
+ } \
+ } else { \
+ stack.Unwind(pc, bp, nullptr, fast, max_size + extra_context); \
+ }
+
#define GET_STACK_TRACE_FATAL(pc, bp) \
UNINITIALIZED BufferedStackTrace stack; \
stack.Unwind(pc, bp, nullptr, common_flags()->fast_unwind_on_fatal)
@@ -60,8 +75,16 @@ u32 GetMallocContextSize();
#define GET_STACK_TRACE_MALLOC \
GET_STACK_TRACE(GetMallocContextSize(), common_flags()->fast_unwind_on_malloc)
+#define GET_STACK_TRACE_MALLOC_WIN(pc, bp, caller_pc, extra_context) \
+ GET_STACK_TRACE_EXPLICIT(__asan::GetMallocContextSize(), \
+ common_flags()->fast_unwind_on_malloc, pc, bp, \
+ caller_pc, extra_context)
+
#define GET_STACK_TRACE_FREE GET_STACK_TRACE_MALLOC
+#define GET_STACK_TRACE_FREE_WIN(pc, bp, caller_pc, extra_context) \
+ GET_STACK_TRACE_MALLOC_WIN(pc, bp, caller_pc, extra_context)
+
#define PRINT_CURRENT_STACK() \
{ \
GET_STACK_TRACE_FATAL_HERE; \
diff --git a/compiler-rt/lib/asan/asan_win_delete_array_align_nothrow_thunk.cpp b/compiler-rt/lib/asan/asan_win_delete_array_align_nothrow_thunk.cpp
new file mode 100644
index 000000000000000..dbc258749446b98
--- /dev/null
+++ b/compiler-rt/lib/asan/asan_win_delete_array_align_nothrow_thunk.cpp
@@ -0,0 +1,40 @@
+//===-- asan_win_delete_array_align_nothrow_thunk.cc ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// Windows-specific user-provided new/delete operator detection and fallback.
+//===----------------------------------------------------------------------===//
+#include "asan_win_new_delete_thunk_common.h"
+
+//////////////////////////////////////////////////////////////////////////////////
+// clang-format off
+// Aligned delete() Fallback Ordering
+//
+// +-------------------+
+// |delete_scalar_align<----+---------------------------+
+// +--^----------------+ | |
+// | | |
+// +--+---------------+ +--+---------------------+ +--+------------------------+
+// |delete_array_align| |delete_scalar_size_align| |delete_scalar_align_nothrow|
+// +--^-----^---------+ +------------------------+ +---------------------------+
+// | |
+// | +------------------------+
+// | |
+// +--+--------------------+ +------+-------------------+
+// |delete_array_size_align| |DELETE_ARRAY_ALIGN_NOTHROW|
+// +-----------------------+ +--------------------------+
+// clang-format on
+
+// Avoid tailcall optimization to preserve stack frame.
+#pragma optimize("", off)
+void operator delete[](void* ptr, std::align_val_t align,
+ std::nothrow_t const&) noexcept {
+ // nothrow version is identical to throwing version
+ operator delete[](ptr, align);
+}
diff --git a/compiler-rt/lib/asan/asan_win_delete_array_align_thunk.cpp b/compiler-rt/lib/asan/asan_win_delete_array_align_thunk.cpp
new file mode 100644
index 000000000000000..eb7351a3e9d5f50
--- /dev/null
+++ b/compiler-rt/lib/asan/asan_win_delete_array_align_thunk.cpp
@@ -0,0 +1,48 @@
+//===-- asan_win_delete_array_align_thunk.cc ------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// Windows-specific user-provided new/delete operator detection and fallback.
+//===----------------------------------------------------------------------===//
+#include "asan_win_new_delete_thunk_common.h"
+
+//////////////////////////////////////////////////////////////////////////////////
+// clang-format off
+// Aligned delete() Fallback Ordering
+//
+// +-------------------+
+// |delete_scalar_align<----+---------------------------+
+// +--^----------------+ | |
+// | | |
+// +--+---------------+ +--+---------------------+ +--+------------------------+
+// |DELETE_ARRAY_ALIGN| |delete_scalar_size_align| |delete_scalar_align_nothrow|
+// +--^-----^---------+ +------------------------+ +---------------------------+
+// | |
+// | +------------------------+
+// | |
+// +--+--------------------+ +------+-------------------+
+// |delete_array_size_align| |delete_array_align_nothrow|
+// +-----------------------+ +--------------------------+
+// clang-format on
+
+__asan_InitDefine<op_delete_array_align> init_delete_array_align;
+
+extern "C" void __cdecl __asan_delete_array_align(
+ __asan_win_new_delete_data* data, void* ptr, std::align_val_t align);
+
+// Avoid tailcall optimization to preserve stack frame.
+#pragma optimize("", off)
+void operator delete[](void* ptr, std::align_val_t align) noexcept {
+ if (__asan_InitDefine<op_delete_scalar_align>::defined) {
+ __asan_win_new_delete_data data{};
+ __asan_delete_array_align(&data, ptr, align);
+ } else {
+ operator delete(ptr, align);
+ }
+}
diff --git a/compiler-rt/lib/asan/asan_win_delete_array_nothrow_thunk.cpp b/compiler-rt/lib/asan/asan_win_delete_array_nothrow_thunk.cpp
new file mode 100644
index 000000000000000..2e44eaf4cc4eade
--- /dev/null
+++ b/compiler-rt/lib/asan/asan_win_delete_array_nothrow_thunk.cpp
@@ -0,0 +1,39 @@
+//===-- asan_win_delete_array_nothrow_thunk.cc ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// Windows-specific user-provided new/delete operator detection and fallback.
+//===----------------------------------------------------------------------===//
+#include "asan_win_new_delete_thunk_common.h"
+
+////////////////////////////////////////////////////////////////
+// clang-format off
+// delete() Fallback Ordering
+//
+// +-------------+
+// |delete_scalar<----+-----------------------+
+// +--^----------+ | |
+// | | |
+// +--+---------+ +--+---------------+ +----+----------------+
+// |delete_array| |delete_scalar_size| |delete_scalar_nothrow|
+// +--^----^----+ +------------------+ +---------------------+
+// | |
+// | +-------------------+
+// | |
+// +--+--------------+ +------+-------------+
+// |delete_array_size| |DELETE_ARRAY_NOTHROW|
+// +-----------------+ +--------------------+
+// clang-format on
+
+// Avoid tailcall optimization to preserve stack frame.
+#pragma optimize("", off)
+void operator delete[](void* ptr, std::nothrow_t const&) noexcept {
+ // nothrow version is identical to throwing version
+ operator delete[](ptr);
+}
diff --git a/compiler-rt/lib/asan/asan_win_delete_array_size_align_thunk.cpp b/compiler-rt/lib/asan/asan_win_delete_array_size_align_thunk.cpp
new file mode 100644
index 000000000000000..d27e2ebb83fd37d
--- /dev/null
+++ b/compiler-rt/lib/asan/asan_win_delete_array_size_align_thunk.cpp
@@ -0,0 +1,49 @@
+//===-- asan_win_delete_array_size_align_thunk.cc -------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// Windows-specific user-provided new/delete operator detection and fallback.
+//===----------------------------------------------------------------------===//
+#include "asan_win_new_delete_thunk_common.h"
+
+//////////////////////////////////////////////////////////////////////////////////
+// clang-format off
+// Aligned delete() Fallback Ordering
+//
+// +-------------------+
+// |delete_scalar_align<----+---------------------------+
+// +--^----------------+ | |
+// | | |
+// +--+---------------+ +--+---------------------+ +--+------------------------+
+// |delete_array_align| |delete_scalar_size_align| |delete_scalar_align_nothrow|
+// +--^-----^---------+ +------------------------+ +---------------------------+
+// | |
+// | +------------------------+
+// | |
+// +--+--------------------+ +------+-------------------+
+// |DELETE_ARRAY_SIZE_ALIGN| |delete_array_align_nothrow|
+// +-----------------------+ +--------------------------+
+// clang-format on
+
+extern "C" void __cdecl __asan_delete_array_size_align(
+ __asan_win_new_delete_data* data, void* ptr, size_t size,
+ std::align_val_t align);
+
+// Avoid tailcall optimization to preserve stack frame.
+#pragma optimize("", off)
+void operator delete[](void* ptr, size_t size,
+ std::align_val_t align) noexcept {
+ if (__asan_InitDefine<op_delete_scalar_align>::defined &&
+ __asan_InitDefine<op_delete_array_align>::defined) {
+ __asan_win_new_delete_data data{};
+ __asan_delete_array_size_align(&data, ptr, size, align);
+ } else {
+ operator delete[](ptr, align);
+ }
+}
diff --git a/compiler-rt/lib/asan/asan_win_delete_array_size_thunk.cpp b/compiler-rt/lib/asan/asan_win_delete_array_size_thunk.cpp
new file mode 100644
index 000000000000000..0720b6337bc7a67
--- /dev/null
+++ b/compiler-rt/lib/asan/asan_win_delete_array_size_thunk.cpp
@@ -0,0 +1,47 @@
+//===-- asan_win_delete_array_size_thunk.cc -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// Windows-specific user-provided new/delete operator detection and fallback.
+//===----------------------------------------------------------------------===//
+#include "asan_win_new_delete_thunk_common.h"
+
+////////////////////////////////////////////////////////////////
+// clang-format off
+// delete() Fallback Ordering
+//
+// +-------------+
+// |delete_scalar<----+-----------------------+
+// +--^----------+ | |
+// | | |
+// +--+---------+ +--+---------------+ +----+----------------+
+// |delete_array| |delete_scalar_size| |delete_scalar_nothrow|
+// +--^----^----+ +------------------+ +---------------------+
+// | |
+// | +-------------------+
+// | |
+// +--+--------------+ +------+-------------+
+// |DELETE_ARRAY_SIZE| |delete_array_nothrow|
+// +-----------------+ +--------------------+
+// clang-format on
+
+extern "C" void __cdecl __asan_delete_array_size(
+ __asan_win_new_delete_data* data, void* ptr, size_t size);
+
+// Avoid tailcall optimization to preserve stack frame.
+#pragma optimize("", off)
+void operator delete[](void* ptr, size_t size) noexcept {
+ if (__asan_InitDefine<op_delete_scalar>::defined &&
+ __asan_InitDefine<op_delete_array>::defined) {
+ __asan_win_new_delete_data data{};
+ __asan_delete_array_size(&data, ptr, size);
+ } else {
+ operator delete[](ptr);
+ }
+}
diff --git a/compiler-rt/lib/asan/asan_win_delete_array_thunk.cpp b/compiler-rt/lib/asan/asan_win_delete_array_thunk.cpp
new file mode 100644
index 000000000000000..02e8b7a5af80b64
--- /dev/null
+++ b/compiler-rt/lib/asan/asan_win_delete_array_thunk.cpp
@@ -0,0 +1,48 @@
+//===-- asan_win_delete_array_thunk.cc ------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is a part of AddressSanitizer, an address sanity checker.
+//
+// Windows-specific user-provided new/delete operator detection and fallback.
+//===----------------------------------------------------------------------===//
+#include "asan_win_new_delete_thunk_common.h"
+
+////////////////////////////////////////////////////////////////
+// clang-format off
+// delete() Fallback Ordering
+//
+// +-------------+
+// |delete_scalar<----+------------...
[truncated]
|
| return GET_CALLER_PC(); | ||
| } | ||
|
|
||
| struct __asan_win_stack_data { |
There was a problem hiding this comment.
I am not sure how this related to the goal of the patch.
Can you just continue to use GET_STACK_TRACE_MALLOC
There was a problem hiding this comment.
From my understanding it's a usability feature, to try and eliminate the asan interception code from the reported callstack. Yeah, I'm not sure this is a good idea either.
There was a problem hiding this comment.
If I understand correctly, the question is "why not just call GET_STACK_TRACE_MALLOC where you are constructing an __asan_win_stack_data"?
The reason is to reduce the file size and complexity of the ASan .lib. The operator new/delete overrides need to be outside the ASan DLL and are included statically. GET_STACK_TRACE_MALLOC calls internal functions that would bloat the otherwise lightweight static library that is only new/delete overrides. We construct __asan_win_stack_data outside the DLL here to collect the stack trace and then pass that into the relevant ASan export where we have all the internals available. Potentially there is a better way to do this by collecting the stack trace inside the DLL and filtering to where we want to show, but at the time we went with this to avoid any "tweaking the numbers" games with respect to how optimization or internal changes affect how our stack traces are displayed.
There was a problem hiding this comment.
Seems reasonable.
Still this patch seperate TUs, so if you can focus only on this here, it would be nice, and anything else in other patches.
There was a problem hiding this comment.
The trouble is that without the stack trace changes the top of the captured stack trace won't be operator new anymore, thus breaking tests. Alternatively we could just eliminate asan_win_new_delete and put the operator bodies in the import library, but that would be a bunch of new code that we'd have to undo in order to move them back to the dll later.
I've fixed most of the tests, I'll fix the last few and then push it and you can see what you think
Currently asan simply exports each overridden new/delete function from the DLL, this works fine normally, but fails if the user is overriding some, but not all, of those functions. In this case the ones they aren't overriding will pull in all the functions from the dll, and you'll get multiple definition errors. In particular this happened with MFC, since MFC contains an op new overload in the same TU as a bunch of other stuff. This fixes that issue and adds tests. I verified that all the tests that were passing before in the dynamic asan build still pass, the dll_heap_allocation test still fails, likely because it's trying to link with the static asan runtime which, as mentioned below, doesn't really work. Additionally this messes around with the build system to add the new/delete overrides to the import library of the DLL, not the dll itself. This is a bit messy in cmake but it does work. The new functions are also added to the static library, but the static library currently doesn't work well on windows, in the branch of asan that we (Microsoft) use i Visual Studio we've actually completely removed support for the static asan runtime, concluding that because of the windows dynamic linkage model it's basically unworkable without really horrible to maintain hacks. add GET_STACK_TRACE_EXPLICIT for windows add static implib to asan static runtime char* -> char** clang-format Differential Revision: https://reviews.llvm.org/D155879
Previously I was using generator expressions which fails because the implib object library doesn't exist at all on non-windows platforms
barcharcraz
force-pushed
the
dev/chbarto/win_new_delete_override_seperate_tu
branch
from
90746c5
to
d5668b2
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I could move the Normally I'd just remove the filtering and do it later, but given that requires modifying all the tests it would be easier to just move the macros to a windows specific header (possibly asan_win_thunk_common) and call it a day. (Oh, and sorry for dropping this on the floor, I got pulled away by a high priority internal task) |
|
Allright, I've removed the stack trace modification stuff. This results in the top of the stack traces being in functions like |
|
Other than comments above it's LGTM |
OK I'll merge it on wednesday then (I'm moving house and I really don't want to have to go and revert this in the middle of my move) |
Yeah, I noticed I forgot to remove those macros, fixed that. For the declarations of the dllexported memory functions I don't understand how moving them would reduce duplicated code at all. Each fallback works through the operator, not the associated |
Declaration is not about duplication. It's to make sure that all involved *cpp files, implementation and users, see the same signature. Deduplication is about diagrams, they look very similar to me. e.g. compiler-rt/lib/asan/asan_win_delete_array_align_nothrow_thunk.cpp if I look at It's not very obvious why the first half of the file is there and how edit the file to preserve this patch intention. Also There are more compact and easier to maintain tree visualizations. |
| } | ||
|
|
||
| try { | ||
| return operator new[](size); |
There was a problem hiding this comment.
FYI (somewhat offtopic) I just checked and saw that this implementation here does fall back on the right operator - vcruntime seems to be getting this bit wrong, see https://developercommunity.visualstudio.com/t/vcruntime-nothrow-array-operator-new-fal/10373274 - where it seems that vcruntime's implementation of nothrowing array new falls back on nothrowing scalar new, instead of throwing array new like it's supposed to.
OK, There's already a full tree in asan_win_new_delete_thunk_common.h, so I'll change each comment in the individual cpp files to reference that |
…fer to the one in asan_win_new_delete_thunk_common.h.
| // see diagram in asan_win_new_delete_thunk_common.h for the ordering of the | ||
| // new/delete fallbacks. | ||
|
|
||
| extern "C" void* __cdecl __asan_new_align_nothrow(size_t size, |
There was a problem hiding this comment.
Why you don't want to move this into header?
There was a problem hiding this comment.
Maybe you can declare all of them in header using?
#include "asan_interface.inc"
There was a problem hiding this comment.
Just because each individual TU is their only callsite, so there's no need to have all of their declarations in a header. If someone needs to call them from someplace else they can move them then.
If you feel strongly about it I can move them though. I thought you just wanted the big diagrams centralized.
There was a problem hiding this comment.
Yes, I think it would be better.
And I think there is high prob. that it would be moved by some random refactoring later.
So I don't see good reasons no to do that right now.
Other than that the patch is LGTM.
compiler-rt/test/asan/TestCases/Windows/new_delete_mfc_already_defined.cpp
Outdated
Show resolved
Hide resolved
|
Clang format is asking |
|
You may need to click "re-request" review. |
|
It fails on these https://lab.llvm.org/buildbot/#/builders/127/builds/59071 |
|
Hi, this patch breaks runtime build on mac. We are seeing following build errors on our Mac bots: I believe the error is caused by 481e9b3#diff-e07727916fd4838a70b92fe6a62b5c6b52b8db246579951b16498628091e05c1R48 |
…68754)" This reverts commit 481e9b3. Breaks sanitizer bots: https://lab.llvm.org/buildbot/#/builders/127/builds/59071
|
@barcharcraz, I reverted the patch because of bot failures: https://lab.llvm.org/buildbot/#/builders/127/builds/59071/steps/8/logs/stdio |
Migrated from: https://reviews.llvm.org/D155879, with some of the suggestions applied.
PR Description copied from above:
Currently asan simply exports each overridden new/delete function from the DLL, this works fine normally, but fails if the user is overriding some, but not all, of these functions. In this case the non-overridden functions still come from the asan DLL, but they can't correctly call the user provided override (for example sized op delete should fall back to scalar op delete, if a scalar op delete is provided). Things were also broken in the static build because all the asan overrides were exported from the same TU, and so if you overrode one but not all of them then you'd get ODR violations. This PR should fix both of these cases, but the static case isn't really tested (and indeed one such test does fail) because linking asan statically basically doesn't work on windows right now with LLVM's version of asan. In fact, while we did fix this in our fork, it was a huge mess and we've now made the dynamic version work in all situations (/MD, /MT, /MDd, /MTd, etc) instead.
The following is the description from the internal PR that implemented most of this feature.
Note that the rearrangement of defaultlibs is not in this diff.