[PATCH] D115103: Leak Sanitizer port to Windows

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a reviewer: rnk.
vitalybuka added a subscriber: rnk.
vitalybuka added a comment.

LGTM in general, but it can be cut into smaller patches.

+ @rnk for Windows suff



================
Comment at: compiler-rt/lib/lsan/lsan.h:20
+#elif SANITIZER_WINDOWS
+#  include "lsan_win.h"
 #endif
----------------
MyDeveloperDay wrote:
> clang-format?
@clemenswasser  Can you please a separate tiny patch which clang-format nearby lines


================
Comment at: compiler-rt/lib/lsan/lsan_allocator.cpp:30
 #if defined(__i386__) || defined(__arm__)
-static const uptr kMaxAllowedMallocSize = 1UL << 30;
+static const uptr kMaxAllowedMallocSize = static_cast<uptr>(1) << 30;
 #elif defined(__mips64) || defined(__aarch64__)
----------------
This also can go into a separate patch
BTW I would slightly prefer just UL -> ULL 


================
Comment at: compiler-rt/lib/lsan/lsan_common.cpp:411
 
+#  if !SANITIZER_WINDOWS
 static void ProcessRootRegion(Frontier *frontier,
----------------
I guess do needs this as well on windows
The problem is that we have no MemoryMappingLayout implementation for Windows.
I guess it can be done with VirtualQueryEx
for now can you just ifdef only body of the ProcessRootRegion with TODO?


================
Comment at: compiler-rt/lib/lsan/lsan_common.h:48
+#elif SANITIZER_NETBSD || SANITIZER_FUCHSIA || SANITIZER_WINDOWS
+#  define CAN_SANITIZE_LEAKS 1
 #else
----------------
MyDeveloperDay wrote:
> you probably don't want to change this do you?
I am fine like this, fighting clang-format is not useful
however it would be nice if you clang-format relevant block in a separate patch then then rebase this patch on top of it


================
Comment at: compiler-rt/lib/lsan/lsan_common_win.cpp:48
+
+void ProcessGlobalRegions(Frontier *frontier) {}
+void ProcessPlatformSpecificAllocations(Frontier *frontier) {}
----------------
how this can work without ProcessGlobalRegions?


================
Comment at: compiler-rt/lib/lsan/lsan_interceptors.cpp:506
+  INTERCEPT_FUNCTION(malloc);
+  INTERCEPT_FUNCTION(free);
+  INTERCEPT_FUNCTION(calloc);
----------------
Can you please just convert unneeded stuff from INTERCEPT_FUNCTION into LSAN_MAYBE_INTERCEPT
In a separate patch


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_stoptheworld_win.cpp:4
+// 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
----------------
sanitizer_stoptheworld_test.cpp needs to be ported to windows, probably with std::
can you please move this work and sanitizer_stoptheworld_win.cpp into separate patch/patches


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

https://reviews.llvm.org/D115103