Merge pull request #84 from microsoft/bug-fix

Bug fix

CMakeLists.txt

@@ -2,6 +2,7 @@ cmake_minimum_required(VERSION 3.8)
 project(snmalloc C CXX)
 
 option(USE_SNMALLOC_STATS "Track allocation stats" OFF)
+option(SNMALLOC_CI_BUILD "Disable features not sensible for CI" OFF)
 option(USE_MEASURE "Measure performance with histograms" OFF)
 option(EXPOSE_EXTERNAL_PAGEMAP "Expose the global pagemap" OFF)
 option(EXPOSE_EXTERNAL_RESERVE "Expose an interface to reserve memory using the default memory provider" OFF)
@@ -95,6 +96,10 @@ if(USE_MEASURE)
   target_compile_definitions(snmalloc_lib INTERFACE -DUSE_MEASURE)
 endif()
 
+if(SNMALLOC_CI_BUILD)
+  target_compile_definitions(snmalloc_lib INTERFACE -DSNMALLOC_CI_BUILD)
+endif()
+
 if(CACHE_FRIENDLY_OFFSET)
   target_compile_definitions(snmalloc_lib INTERFACE -DCACHE_FRIENDLY_OFFSET=${CACHE_FRIENDLY_OFFSET})
 endif()
@@ -158,7 +163,7 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY)
         aux_source_directory(${TESTDIR}/${TEST_CATEGORY}/${TEST} SRC)
         set(TESTNAME "${TEST_CATEGORY}-${TEST}-${SUPER_SLAB_SIZE}")
 
-        add_executable(${TESTNAME} ${SRC} src/override/new.cc)
+        add_executable(${TESTNAME} ${SRC})
         if (${SUPER_SLAB_SIZE} EQUAL 1)
           target_compile_definitions(${TESTNAME} PRIVATE IS_ADDRESS_SPACE_CONSTRAINED)
         endif()
@@ -174,6 +179,9 @@ if(NOT DEFINED SNMALLOC_ONLY_HEADER_LIBRARY)
         if (${TEST_CATEGORY} MATCHES "perf")
           set_tests_properties(${TESTNAME} PROPERTIES PROCESSORS 4)
         endif()
+        if (${TEST_CATEGORY} MATCHES "func")
+          target_compile_definitions(${TESTNAME} PRIVATE -DUSE_SNMALLOC_STATS)
+        endif ()
       endforeach()
     endforeach()
   endforeach()

azure-pipelines.yml

@@ -72,9 +72,9 @@ jobs:
     displayName: 'Install Build Dependencies'
 
   - task: CMake@1
-    displayName: 'CMake .. $(CMakeArgs) -GNinja -DCMAKE_BUILD_TYPE=$(BuildType) -DCMAKE_CXX_FLAGS="$CMAKE_CXX_FLAGS"'
+    displayName: 'CMake .. $(CMakeArgs) -GNinja -DCMAKE_BUILD_TYPE=$(BuildType) -DCMAKE_CXX_FLAGS="$CMAKE_CXX_FLAGS" -DSNMALLOC_CI_BUILD=On'
     inputs:
-      cmakeArgs: '.. $(CMakeArgs) -GNinja -DCMAKE_BUILD_TYPE=$(BuildType) -DCMAKE_CXX_FLAGS="$CMAKE_CXX_FLAGS"'
+      cmakeArgs: '.. $(CMakeArgs) -GNinja -DCMAKE_BUILD_TYPE=$(BuildType) -DCMAKE_CXX_FLAGS="$CMAKE_CXX_FLAGS" -DSNMALLOC_CI_BUILD=On'
 
   - script: |
       ninja
@@ -131,9 +131,9 @@ jobs:
 
   steps:
   - task: CMake@1
-    displayName: 'CMake .. $(CMakeArgs) -DCMAKE_BUILD_TYPE=$(BuildType)'
+    displayName: 'CMake .. $(CMakeArgs) -DCMAKE_BUILD_TYPE=$(BuildType) -DSNMALLOC_CI_BUILD=On'
     inputs:
-      cmakeArgs: '.. $(CMakeArgs) -DCMAKE_BUILD_TYPE=$(BuildType)'
+      cmakeArgs: '.. $(CMakeArgs) -DCMAKE_BUILD_TYPE=$(BuildType) -DSNMALLOC_CI_BUILD=On'
 
   - task: MSBuild@1
     displayName: 'Build solution build/snmalloc.sln'
@@ -158,9 +158,9 @@ jobs:
 
   steps:
   - task: CMake@1
-    displayName: 'CMake .. -DCMAKE_BUILD_TYPE=$(BuildType)'
+    displayName: 'CMake .. -DCMAKE_BUILD_TYPE=$(BuildType) -DSNMALLOC_CI_BUILD=On'
     inputs:
-      cmakeArgs: '.. -DCMAKE_BUILD_TYPE=$(BuildType)'
+      cmakeArgs: '.. -DCMAKE_BUILD_TYPE=$(BuildType) -DSNMALLOC_CI_BUILD=On'
 
   - script: |
       make -j 4

src/mem/alloc.h

@@ -1084,13 +1084,13 @@ namespace snmalloc
 
       SNMALLOC_ASSUME(size <= SLAB_SIZE);
       sizeclass_t sizeclass = size_to_sizeclass(size);
-      stats().sizeclass_alloc(sizeclass);
 
       assert(sizeclass < NUM_SMALL_CLASSES);
       auto& fl = small_fast_free_lists[sizeclass];
       void* head = fl.value;
       if (likely(head != nullptr))
       {
+        stats().sizeclass_alloc(sizeclass);
         // Read the next slot from the memory that's about to be allocated.
         fl.value = Metaslab::follow_next(head);
 
@@ -1113,6 +1113,9 @@ namespace snmalloc
         return reinterpret_cast<Allocator*>(replacement)
           ->template small_alloc_slow<zero_mem, allow_reserve>(sizeclass);
       }
+
+      stats().sizeclass_alloc(sizeclass);
+
       handle_message_queue();
       size_t rsize = sizeclass_to_size(sizeclass);
       auto& sl = small_classes[sizeclass];

src/mem/allocstats.h

@@ -141,7 +141,9 @@ namespace snmalloc
       UNUSED(size);
 
 #ifdef USE_SNMALLOC_STATS
-      bucketed_requests[bits::to_exp_mant<BUCKETS_BITS>(size)]++;
+      auto index = (size == 0) ? 0 : bits::to_exp_mant<BUCKETS_BITS>(size);
+      assert(index < TOTAL_BUCKETS);
+      bucketed_requests[index]++;
 #endif
     }
 

src/mem/globalalloc.h

@@ -107,28 +107,26 @@ namespace snmalloc
 #endif
     }
 
-    void debug_check_empty()
+    /**
+      If you pass a pointer to a bool, then it returns whether all the
+      allocators are empty. If you don't pass a pointer to a bool, then will
+      raise an error all the allocators are not empty.
+     */
+    void debug_check_empty(bool* result = nullptr)
     {
 #ifndef USE_MALLOC
       // This is a debugging function. It checks that all memory from all
       // allocators has been freed.
-      size_t alloc_count = 0;
-
       auto* alloc = Parent::iterate();
 
-      // Count the linked allocators.
-      while (alloc != nullptr)
-      {
-        alloc = Parent::iterate(alloc);
-        alloc_count++;
-      }
-
       bool done = false;
 
+      size_t non_empty_count = 0;
       while (!done)
       {
         done = true;
         alloc = Parent::iterate();
+        non_empty_count = 0;
 
         while (alloc != nullptr)
         {
@@ -142,6 +140,10 @@ namespace snmalloc
             p = next;
           }
 
+          // Check that the allocator has freed all memory.
+          if (!alloc->stats().is_empty())
+            non_empty_count++;
+
           // Place the static stub message on the queue.
           alloc->init_message_queue();
 
@@ -158,20 +160,16 @@ namespace snmalloc
         }
       }
 
-      alloc = Parent::iterate();
-      size_t empty_count = 0;
-
-      while (alloc != nullptr)
+      if (result != nullptr)
       {
-        // Check that the allocator has freed all memory.
-        if (alloc->stats().is_empty())
-          empty_count++;
-
-        alloc = Parent::iterate(alloc);
+        *result = non_empty_count == 0;
+        return;
       }
 
-      if (alloc_count != empty_count)
-        error("Incorrect number of allocators");
+      if (non_empty_count != 0)
+      {
+        error("debug_check_empty: found non-empty allocators");
+      }
 #endif
     }
   };

src/test/func/malloc/malloc.cc

@@ -1,4 +1,5 @@
 #include <stdio.h>
+#include <test/setup.h>
 
 #define SNMALLOC_NAME_MANGLE(a) our_##a
 #include "../../../override/malloc.cc"
@@ -48,8 +49,11 @@ void test_realloc(void* p, size_t size, int err, bool null)
 {
   fprintf(stderr, "realloc(%p(%d), %d)\n", p, int(size), (int)size);
   errno = 0;
-  p = our_realloc(p, size);
-  check_result(size, 1, p, err, null);
+  auto new_p = our_realloc(p, size);
+  // Realloc failure case, deallocate original block
+  if (new_p == nullptr && size != 0)
+    our_free(p);
+  check_result(size, 1, new_p, err, null);
 }
 
 void test_posix_memalign(size_t size, size_t align, int err, bool null)
@@ -73,6 +77,8 @@ int main(int argc, char** argv)
   UNUSED(argc);
   UNUSED(argv);
 
+  setup();
+
   constexpr int SUCCESS = 0;
 
   test_calloc(0, 0, SUCCESS, false);
@@ -115,5 +121,6 @@ int main(int argc, char** argv)
     test_posix_memalign(0, align + 1, EINVAL, true);
   }
 
+  current_alloc_pool()->debug_check_empty();
   return 0;
 }

src/test/func/release-rounding/rounding.cc

@@ -1,4 +1,5 @@
 #include <snmalloc.h>
+#include <test/setup.h>
 
 using namespace snmalloc;
 
@@ -9,6 +10,8 @@ using namespace snmalloc;
 
 int main(int argc, char** argv)
 {
+  setup();
+
   UNUSED(argc);
   UNUSED(argv);
 

src/test/func/sizeclass/sizeclass.cc

@@ -1,5 +1,6 @@
 #include <iostream>
 #include <snmalloc.h>
+#include <test/setup.h>
 
 NOINLINE
 snmalloc::sizeclass_t size_to_sizeclass(size_t size)
@@ -9,6 +10,8 @@ snmalloc::sizeclass_t size_to_sizeclass(size_t size)
 
 int main(int, char**)
 {
+  setup();
+
   bool failed = false;
   size_t size_low = 0;
 

src/test/func/statistics/stats.cc

@@ -0,0 +1,39 @@
+#include <snmalloc.h>
+
+int main()
+{
+  snmalloc::Alloc* a = snmalloc::ThreadAlloc::get();
+  bool result;
+
+  auto r = a->alloc(16);
+
+  snmalloc::current_alloc_pool()->debug_check_empty(&result);
+  if (result != false)
+  {
+    abort();
+  }
+
+  a->dealloc(r);
+
+  snmalloc::current_alloc_pool()->debug_check_empty(&result);
+  if (result != true)
+  {
+    abort();
+  }
+
+  r = a->alloc(16);
+
+  snmalloc::current_alloc_pool()->debug_check_empty(&result);
+  if (result != false)
+  {
+    abort();
+  }
+
+  a->dealloc(r);
+
+  snmalloc::current_alloc_pool()->debug_check_empty(&result);
+  if (result != true)
+  {
+    abort();
+  }
+}

src/test/func/two_alloc_types/main.cc

@@ -3,6 +3,7 @@
 #include <iostream>
 #include <stdlib.h>
 #include <string.h>
+#include <test/setup.h>
 
 void* oe_base;
 void* oe_end;
@@ -40,9 +41,11 @@ host_snmalloc_pagemap_global_get(snmalloc::PagemapConfig const**);
 using namespace snmalloc;
 int main()
 {
+  setup();
+
   MemoryProviderStateMixin<DefaultPal> mp;
 
-  size_t size = 1ULL << 28;
+  size_t size = 1ULL << 26;
   oe_base = mp.reserve<true>(&size, 1);
   oe_end = (uint8_t*)oe_base + size;
   std::cout << "Allocated region " << oe_base << " - " << oe_end << std::endl;

src/test/perf/contention/contention.cc

@@ -1,5 +1,6 @@
 #include "test/measuretime.h"
 #include "test/opt.h"
+#include "test/setup.h"
 #include "test/usage.h"
 #include "test/xoroshiro.h"
 
@@ -148,6 +149,8 @@ void test_tasks(size_t num_tasks, size_t count, size_t size)
 
 int main(int argc, char** argv)
 {
+  setup();
+
   opt::Opt opt(argc, argv);
   size_t cores = opt.is<size_t>("--cores", 8);
 

src/test/perf/external_pointer/externalpointer.cc

@@ -1,5 +1,6 @@
 #include <snmalloc.h>
 #include <test/measuretime.h>
+#include <test/setup.h>
 #include <test/xoroshiro.h>
 #include <unordered_set>
 
@@ -78,6 +79,8 @@ namespace test
 
 int main(int, char**)
 {
+  setup();
+
   xoroshiro::p128r64 r;
 #if NDEBUG
   size_t nn = 30;

src/test/perf/singlethread/singlethread.cc

@@ -1,5 +1,6 @@
 #include <snmalloc.h>
 #include <test/measuretime.h>
+#include <test/setup.h>
 #include <unordered_set>
 
 using namespace snmalloc;
@@ -63,6 +64,8 @@ void test_alloc_dealloc(size_t count, size_t size, bool write)
 
 int main(int, char**)
 {
+  setup();
+
   for (size_t size = 16; size <= 128; size <<= 1)
   {
     test_alloc_dealloc<NoZero>(1 << 15, size, false);

src/test/setup.h

@@ -0,0 +1,20 @@
+#if defined(WIN32) && defined(SNMALLOC_CI_BUILD)
+#  include <ds/bits.h>
+#  include <signal.h>
+#  include <stdlib.h>
+void _cdecl error(int signal)
+{
+  UNUSED(signal);
+  puts("*****ABORT******");
+  _exit(1);
+}
+void setup()
+{
+  // Disable abort dialog box in CI builds.
+  _set_error_mode(_OUT_TO_STDERR);
+  _set_abort_behavior(0, _WRITE_ABORT_MSG);
+  signal(SIGABRT, error);
+}
+#else
+void setup() {}
+#endif