[compiler-rt] Windows Debug build fix

[mcgov]

Summary

Define a function that asserts if called for MemalignFromLocalPool. This function is never called on Windows release builds because the 'if' condition that contains it is optimized away. It's presence breaks debug builds for asan_new_delete.cc, so something must define it to compile a debug build.

[zturner]

lib/asan/asan_new_delete.cc

73

I don't have an opinion on whether this fix is ok, but is the #ifdef correct here? Shouldn't it be #if defined(SANITIZER_WINDOWS) && defined(DEBUG) or #if SANITIZER_WINDOWS && DEBUG

[mcgov]

fixed nonportable #ifdef condition

[mcgov]

Fixing nonportable #ifdef condition identified by @zturner

[vitalybuka]

lib/asan/asan_new_delete.cc

73

Looks like we check windoes as #if SANITIZER_WINDOWS and debug as #ifdef DEBUG
so I believe it should be
#if SANITIZER_WINDOWS && defined(DEBUG)

[vitalybuka]

And I don't see this code in the r346999

Could you please explain why you need to patch code which is not in the repo?
https://github.com/llvm-project/llvm-project-20170507/blob/master/compiler-rt/lib/asan/asan_new_delete.cc#L72

Does this depend on other patches?

[mcgov]

Updating to follow convention for SANITIZER_WINDOWS and DEBUG checks.

[mcgov]

Fixing to account for Windows and Debug checks.

[mcgov]

In D54581#1300511, @vitalybuka wrote:

And I don't see this code in the r346999

Could you please explain why you need to patch code which is not in the repo?

Ah I think I see the issue. I uploaded a diff from my first version of the patch, rather than a clean diff from the master version. I'll fix this now.

Building debug will not succeed because of the undefined symbol, MemalignFromLocalPool. The idea was to define the symbol as nullptr, since the code path is never executed on Windows and is optimized away in Release builds. It occurs to me that a function that just asserts if it's called may be a better idea , rather than the dumb macro.

[mcgov]

Replaced #define with a function that asserts if called.

[mcgov]

Fix debug builds by defining a Windows section of the OPERATOR_NEW_BODY macro with the MemalignFromLocalPool call and if condition removed. This section is dead code on Windows, anyway.

[mcgov]

This comment was removed by mcgov.

[mcgov]

This needs some more work, I'll ping you all when I have a real fix.

[vitalybuka]

In D54581#1303325, @mcgov wrote:

This needs some more work, I'll ping you all when I have a real fix.

you can switch this patch into "Plan Changes" state

lib/asan/asan_new_delete.cc

76

ALLOCATE_FROM_LOCAL_POOL is used for early allocations when asan is not initialized, so asan_memalign will not work.

[mcgov]

In D54581#1303419, @vitalybuka wrote:

In D54581#1303325, @mcgov wrote:

you can switch this patch into "Plan Changes" state

I don't see an option to do this, is there some permission needed to change the revision state from "Needs Review"?

lib/asan/asan_new_delete.cc

76

It looks like ALLOCATE_FROM_LOCAL_POOL is statically defined and only really important on RTEMS, so this wouldn't matter here. The original definition is still below in the #else condition. This section is only for Windows.

When running the test suite for this though, the debug build will complete with this fix. It will not successfully run the asan-check-dynamic suite though, so I need to figure out what's going wrong there.

[vitalybuka]

In D54581#1303486, @mcgov wrote:

In D54581#1303419, @vitalybuka wrote:

In D54581#1303325, @mcgov wrote:

you can switch this patch into "Plan Changes" state

I don't see an option to do this, is there some permission needed to change the revision state from "Needs Review"?

Should be in the "Add Action..." listbox in the bottom of the review page.

lib/asan/asan_new_delete.cc

76

sgtm

lib/asan/asan_new_delete.cc

 enum class align_val_t: size_t {};
 }  // namespace std
 
+//Windows builds do not define MemalignFromLocalPool
+#if SANITIZER_WINDOWS
+#define OPERATOR_NEW_BODY(type, nothrow) \
+  GET_STACK_TRACE_MALLOC;\
+  void *res = asan_memalign(0, size, &stack, type);\
+  if (!nothrow && UNLIKELY(!res)) ReportOutOfMemory(size, &stack);\
+  return res;
+#define OPERATOR_NEW_BODY_ALIGN(type, nothrow) \
+  GET_STACK_TRACE_MALLOC;\
+  void *res = asan_memalign((uptr)align, size, &stack, type);\
+  if (!nothrow && UNLIKELY(!res)) ReportOutOfMemory(size, &stack);\
+  return res;
+#else
 // TODO(alekseyshl): throw std::bad_alloc instead of dying on OOM.
 // For local pool allocation, align to SHADOW_GRANULARITY to match asan
 // allocator behavior.

   void *res = asan_memalign((uptr)align, size, &stack, type);\
   if (!nothrow && UNLIKELY(!res)) ReportOutOfMemory(size, &stack);\
   return res;
-
+#endif
 // On OS X it's not enough to just provide our own 'operator new' and
 // 'operator delete' implementations, because they're going to be in the
 // runtime dylib, and the main executable will depend on both the runtime