improvements to code that checks whether Python (obmalloc) allocated an address

benjaminp · benjaminp · commit 3924f93794fd · 2016-09-18T19:12:48.000-07:00
- Rename Py_ADDRESS_IN_RANGE to address_in_range and make it a static
  function instead of macro. Any compiler worth its salt will inline this
  function.
- Remove the duplicated function version of Py_ADDRESS_IN_RANGE used when memory
  analysis was active. Instead, we can simply mark address_in_range as allergic
  to dynamic memory checking. We can now remove the
  __attribute__((no_address_safety_analysis)) from _PyObject_Free and
  _PyObject_Realloc. All the badness is contained in address_in_range now.
- Fix the code that tried to only read pool-&gt;arenaindex once. Putting something
  in a variable is no guarantee that it won't be read multiple times. We must
  use volatile for that.
diff --git a/Objects/obmalloc.c b/Objects/obmalloc.c
@@ -1,5 +1,7 @@
 #include "Python.h"
 
+#include <stdbool.h>
+
 
 /* Defined in tracemalloc.c */
 extern void _PyMem_DumpTraceback(int fd, const void *ptr);
@@ -37,16 +39,14 @@ static void _PyMem_DebugCheckAddress(char api_id, const void *p);
 #if defined(__has_feature)  /* Clang */
  #if __has_feature(address_sanitizer)  /* is ASAN enabled? */
   #define ATTRIBUTE_NO_ADDRESS_SAFETY_ANALYSIS \
-        __attribute__((no_address_safety_analysis)) \
-        __attribute__ ((noinline))
+        __attribute__((no_address_safety_analysis))
  #else
   #define ATTRIBUTE_NO_ADDRESS_SAFETY_ANALYSIS
  #endif
 #else
  #if defined(__SANITIZE_ADDRESS__)  /* GCC 4.8.x, is ASAN enabled? */
   #define ATTRIBUTE_NO_ADDRESS_SAFETY_ANALYSIS \
-        __attribute__((no_address_safety_analysis)) \
-        __attribute__ ((noinline))
+        __attribute__((no_address_safety_analysis))
  #else
   #define ATTRIBUTE_NO_ADDRESS_SAFETY_ANALYSIS
  #endif
@@ -1124,13 +1124,13 @@ new_arena(void)
 }
 
 /*
-Py_ADDRESS_IN_RANGE(P, POOL)
+address_in_range(P, POOL)
 
 Return true if and only if P is an address that was allocated by pymalloc.
 POOL must be the pool address associated with P, i.e., POOL = POOL_ADDR(P)
 (the caller is asked to compute this because the macro expands POOL more than
 once, and for efficiency it's best for the caller to assign POOL_ADDR(P) to a
-variable and pass the latter to the macro; because Py_ADDRESS_IN_RANGE is
+variable and pass the latter to the macro; because address_in_range is
 called on every alloc/realloc/free, micro-efficiency is important here).
 
 Tricky:  Let B be the arena base address associated with the pool, B =
@@ -1155,7 +1155,7 @@ arenas[(POOL)->arenaindex].  Suppose obmalloc controls P.  Then (barring wild
 stores, etc), POOL is the correct address of P's pool, AO.address is the
 correct base address of the pool's arena, and P must be within ARENA_SIZE of
 AO.address.  In addition, AO.address is not 0 (no arena can start at address 0
-(NULL)).  Therefore Py_ADDRESS_IN_RANGE correctly reports that obmalloc
+(NULL)).  Therefore address_in_range correctly reports that obmalloc
 controls P.
 
 Now suppose obmalloc does not control P (e.g., P was obtained via a direct
@@ -1196,51 +1196,21 @@ that this test determines whether an arbitrary address is controlled by
 obmalloc in a small constant time, independent of the number of arenas
 obmalloc controls.  Since this test is needed at every entry point, it's
 extremely desirable that it be this fast.
-
-Since Py_ADDRESS_IN_RANGE may be reading from memory which was not allocated
-by Python, it is important that (POOL)->arenaindex is read only once, as
-another thread may be concurrently modifying the value without holding the
-GIL.  To accomplish this, the arenaindex_temp variable is used to store
-(POOL)->arenaindex for the duration of the Py_ADDRESS_IN_RANGE macro's
-execution.  The caller of the macro is responsible for declaring this
-variable.
 */
-#define Py_ADDRESS_IN_RANGE(P, POOL)                    \
-    ((arenaindex_temp = (POOL)->arenaindex) < maxarenas &&              \
-     (uptr)(P) - arenas[arenaindex_temp].address < (uptr)ARENA_SIZE && \
-     arenas[arenaindex_temp].address != 0)
-
-
-/* This is only useful when running memory debuggers such as
- * Purify or Valgrind.  Uncomment to use.
- *
-#define Py_USING_MEMORY_DEBUGGER
- */
-
-#ifdef Py_USING_MEMORY_DEBUGGER
-
-/* Py_ADDRESS_IN_RANGE may access uninitialized memory by design
- * This leads to thousands of spurious warnings when using
- * Purify or Valgrind.  By making a function, we can easily
- * suppress the uninitialized memory reads in this one function.
- * So we won't ignore real errors elsewhere.
- *
- * Disable the macro and use a function.
- */
-
-#undef Py_ADDRESS_IN_RANGE
-
-#if defined(__GNUC__) && ((__GNUC__ == 3) && (__GNUC_MINOR__ >= 1) || \
-                          (__GNUC__ >= 4))
-#define Py_NO_INLINE __attribute__((__noinline__))
-#else
-#define Py_NO_INLINE
-#endif
 
-/* Don't make static, to try to ensure this isn't inlined. */
-int Py_ADDRESS_IN_RANGE(void *P, poolp pool) Py_NO_INLINE;
-#undef Py_NO_INLINE
-#endif
+static bool ATTRIBUTE_NO_ADDRESS_SAFETY_ANALYSIS
+address_in_range(void *p, poolp pool)
+{
+    // Since address_in_range may be reading from memory which was not allocated
+    // by Python, it is important that pool->arenaindex is read only once, as
+    // another thread may be concurrently modifying the value without holding
+    // the GIL. The following dance forces the compiler to read pool->arenaindex
+    // only once.
+    uint arenaindex = *((volatile uint *)&pool->arenaindex);
+    return arenaindex < maxarenas &&
+        (uptr)p - arenas[arenaindex].address < ARENA_SIZE &&
+        arenas[arenaindex].address != 0;
+}
 
 /*==========================================================================*/
 
@@ -1485,17 +1455,13 @@ _PyObject_Calloc(void *ctx, size_t nelem, size_t elsize)
 
 /* free */
 
-ATTRIBUTE_NO_ADDRESS_SAFETY_ANALYSIS
 static void
 _PyObject_Free(void *ctx, void *p)
 {
     poolp pool;
     block *lastfree;
     poolp next, prev;
     uint size;
-#ifndef Py_USING_MEMORY_DEBUGGER
-    uint arenaindex_temp;
-#endif
 
     if (p == NULL)      /* free(NULL) has no effect */
         return;
@@ -1508,7 +1474,7 @@ _PyObject_Free(void *ctx, void *p)
 #endif
 
     pool = POOL_ADDR(p);
-    if (Py_ADDRESS_IN_RANGE(p, pool)) {
+    if (address_in_range(p, pool)) {
         /* We allocated this address. */
         LOCK();
         /* Link p to the start of the pool's freeblock list.  Since
@@ -1714,16 +1680,12 @@ _PyObject_Free(void *ctx, void *p)
  * return a non-NULL result.
  */
 
-ATTRIBUTE_NO_ADDRESS_SAFETY_ANALYSIS
 static void *
 _PyObject_Realloc(void *ctx, void *p, size_t nbytes)
 {
     void *bp;
     poolp pool;
     size_t size;
-#ifndef Py_USING_MEMORY_DEBUGGER
-    uint arenaindex_temp;
-#endif
 
     if (p == NULL)
         return _PyObject_Alloc(0, ctx, 1, nbytes);
@@ -1735,7 +1697,7 @@ _PyObject_Realloc(void *ctx, void *p, size_t nbytes)
 #endif
 
     pool = POOL_ADDR(p);
-    if (Py_ADDRESS_IN_RANGE(p, pool)) {
+    if (address_in_range(p, pool)) {
         /* We're in charge of this block */
         size = INDEX2SIZE(pool->szidx);
         if (nbytes <= size) {
@@ -2432,19 +2394,3 @@ _PyObject_DebugMallocStats(FILE *out)
 }
 
 #endif /* #ifdef WITH_PYMALLOC */
-
-
-#ifdef Py_USING_MEMORY_DEBUGGER
-/* Make this function last so gcc won't inline it since the definition is
- * after the reference.
- */
-int
-Py_ADDRESS_IN_RANGE(void *P, poolp pool)
-{
-    uint arenaindex_temp = pool->arenaindex;
-
-    return arenaindex_temp < maxarenas &&
-           (uptr)P - arenas[arenaindex_temp].address < (uptr)ARENA_SIZE &&
-           arenas[arenaindex_temp].address != 0;
-}
-#endif
