BUILD: Adjusted BRK/SBRK conditionals to review feedback.

Author: GenericRikka

config/m4/ucm.m4

@@ -8,28 +8,30 @@
 #
 # Memory allocator selection
 #
+AC_CHECK_FUNCS([brk sbrk])
+
 AC_ARG_WITH([allocator],
     [AS_HELP_STRING([--with-allocator=NAME],
         [Build UCX with predefined memory allocator. The supported values are:
          ptmalloc286. Default: ptmalloc286])],
         [],
         [with_allocator=ptmalloc286])
 
-HAVE_UCM_PTMALLOC286=no
-
 case ${with_allocator} in
     ptmalloc286)
         AC_MSG_NOTICE(Memory allocator is ptmalloc-2.8.6 version)
-        AS_IF([test "x$have_brk_sbrk" = xyes],
-              [AC_DEFINE([HAVE_UCM_PTMALLOC286], 1, [Use ptmalloc-2.8.6 version])
-               HAVE_UCM_PTMALLOC286=yes],
-              [AC_MSG_WARN([brk()/sbrk() not available; disabling ptmalloc286 allocator])
-               HAVE_UCM_PTMALLOC286=no])
+        AC_DEFINE([HAVE_UCM_PTMALLOC286], [1], [Use ptmalloc-2.8.6 version])
+        HAVE_UCM_PTMALLOC286=yes
+        AS_IF([test "x$ac_cv_func_sbrk" != xyes], [
+            AC_DEFINE([HAVE_MORECORE], [0],
+                      [Disable MORECORE in ptmalloc (no sbrk available)])
+        ])
         ;;
     *)
         AC_MSG_ERROR(Cannot continue. Unsupported memory allocator name
                      in --with-allocator=[$with_allocator])
         ;;
+
 esac
 
 AM_CONDITIONAL([HAVE_UCM_PTMALLOC286],[test "x$HAVE_UCM_PTMALLOC286" = "xyes"])
@@ -89,9 +91,6 @@ AS_IF([test "x$mmap_hooks_happy" = "xyes"],
       AS_IF([test "x$ipc_hooks_happy" = "xyes" -o "x$shm_hooks_happy" = "xyes"],
             [bistro_hooks_happy=yes]))
 
-AS_IF([test "x$have_brk_sbrk" != xyes],
-      [bistro_hooks_happy=no])
-
 AS_IF([test "x$bistro_hooks_happy" = "xyes"],
       [AC_DEFINE([UCM_BISTRO_HOOKS], [1], [Enable BISTRO hooks])],
       [AC_DEFINE([UCM_BISTRO_HOOKS], [0], [Enable BISTRO hooks])

configure.ac

@@ -87,16 +87,6 @@ AC_FUNC_STRERROR_R
 
 AC_PATH_TOOL([PKG_CONFIG], [pkg-config], [pkg-config])
 
-AC_CHECK_FUNCS([brk sbrk])
-
-AS_IF([test "x$ac_cv_func_brk" = "xyes" && test "x$ac_cv_func_sbrk" = "xyes"],
-      [have_brk_sbrk=yes],
-      [have_brk_sbrk=no])
-
-AS_IF([test "x$have_brk_sbrk" = "xyes"],
-      [AC_DEFINE([HAVE_BRK_SBRK], [1],
-                 [Define if both brk() and sbrk() are available])])
-
 
 #
 # Define SHARED_LIB preprocessor macro when building a shared library

src/ucm/malloc/malloc_hook.c

@@ -556,17 +556,6 @@ static int ucm_setenv(const char *name, const char *value, int overwrite)
     return ret;
 }
 
-static inline int ucm_malloc_sanitize_events(int events)
-{
-#ifdef HAVE_BRK_SBRK
-    return events;
-#else
-    /* If brk/sbrk are not available, don't wait for or install sbrk/brk hooks */
-    return events & ~(UCM_EVENT_SBRK | UCM_EVENT_BRK);
-#endif
-}
-
-#ifdef HAVE_BRK_SBRK
 static void ucm_malloc_sbrk(ucm_event_type_t event_type,
                             ucm_event_t *event, void *arg)
 {
@@ -584,7 +573,6 @@ static void ucm_malloc_sbrk(ucm_event_type_t event_type,
 
     ucs_recursive_spin_unlock(&ucm_malloc_hook_state.lock);
 }
-#endif
 
 static int ucs_malloc_is_ready(int events, const char *title)
 {
@@ -616,9 +604,6 @@ static void ucm_malloc_event_test_callback(ucm_event_type_t event_type,
 /* Has to be called with install_mutex held */
 static void ucm_malloc_test(int events)
 {
-#if !defined(HAVE_BRK_SBRK)
-    events = ucm_malloc_sanitize_events(events);
-#endif
     static const size_t small_alloc_count = 128;
     static const size_t small_alloc_size  = 4096;
     static const size_t large_alloc_size  = 4 * UCS_MBYTE;
@@ -824,17 +809,11 @@ static void ucm_malloc_init_orig_funcs()
 
 ucs_status_t ucm_malloc_install(int events)
 {
-#if !defined(HAVE_BRK_SBRK)
-    events = ucm_malloc_sanitize_events(events);
-#endif
-
-#ifdef HAVE_BRK_SBRK
     static ucm_event_handler_t sbrk_handler = {
         .events   = UCM_EVENT_SBRK,
         .priority = 1000,
         .cb       = ucm_malloc_sbrk
     };
-#endif
     ucs_status_t status;
 
     pthread_mutex_lock(&ucm_malloc_hook_state.install_mutex);
@@ -857,12 +836,28 @@ ucs_status_t ucm_malloc_install(int events)
 #endif
     }
 
-#ifdef HAVE_BRK_SBRK
+#ifndef HAVE_BRK
+    if (events & UCM_EVENT_BRK) {
+        ucm_debug("brk event requested, but brk() is not available");
+        events &= ~UCM_EVENT_BRK;
+    }
+#endif
+
+#ifndef HAVE_SBRK
+    if (events & UCM_EVENT_SBRK) {
+        ucm_debug("sbrk event requested, but sbrk() is not available");
+        events &= ~UCM_EVENT_SBRK;
+    }
+#endif
+
+#ifdef HAVE_SBRK
     if (!(ucm_malloc_hook_state.install_state & UCM_MALLOC_INSTALLED_SBRK_EVH)) {
         ucm_debug("installing malloc-sbrk event handler");
         ucm_event_handler_add(&sbrk_handler);
         ucm_malloc_hook_state.install_state |= UCM_MALLOC_INSTALLED_SBRK_EVH;
     }
+#else
+    (void)sbrk_handler;
 #endif
 
     /* When running on valgrind, don't even try malloc hooks.

src/ucm/mmap/install.c

@@ -86,8 +86,10 @@ static ucm_mmap_func_t ucm_mmap_funcs[] = {
 #endif
     { UCM_MMAP_RELOC_ENTRY(shmat),   UCM_EVENT_SHMAT,   UCM_EVENT_NONE},
     { UCM_MMAP_RELOC_ENTRY(shmdt),   UCM_EVENT_SHMDT,   UCM_EVENT_SHMAT},
-#ifdef HAVE_BRK_SBRK
+#ifdef HAVE_SBRK
     { UCM_MMAP_RELOC_ENTRY(sbrk),    UCM_EVENT_SBRK,    UCM_EVENT_NONE},
+#endif
+#ifdef HAVE_BRK
     { UCM_MMAP_RELOC_ENTRY(brk),     UCM_EVENT_BRK,     UCM_EVENT_NONE},
 #endif
     { UCM_MMAP_RELOC_ENTRY(madvise), UCM_EVENT_MADVISE, UCM_EVENT_NONE},
@@ -137,16 +139,16 @@ static void ucm_mmap_event_test_callback(ucm_event_type_t event_type,
     }
 }
 
-#ifdef HAVE_BRK_SBRK
 /* Call brk() and check return value, to avoid compile error of unused result */
 static void ucm_brk_checked(void *addr)
 {
+#ifdef HAVE_BRK
     int ret = brk(addr);
     if ((ret != 0) && (addr != NULL)) {
         ucm_diag("brk(addr=%p) failed: %m", addr);
     }
-}
 #endif
+}
 
 /* Fire events with pre/post action. The problem is in call sequence: we
  * can't just fire single event - most of the system calls require set of
@@ -203,7 +205,7 @@ ucm_fire_mmap_events_internal(int events, ucm_mmap_test_events_data_t *data,
     }
 
     if (exclusive && !RUNNING_ON_VALGRIND) {
-#ifdef HAVE_BRK_SBRK
+#ifdef HAVE_SBRK
         sbrk_size = ucm_get_page_size();
         if (events & (UCM_EVENT_BRK|UCM_EVENT_VM_MAPPED|UCM_EVENT_VM_UNMAPPED)) {
             p = ucm_get_current_brk();
@@ -224,11 +226,9 @@ ucm_fire_mmap_events_internal(int events, ucm_mmap_test_events_data_t *data,
          * pass invalid parameters. We assume that if the natives events are
          * delivered, it means VM_MAPPED/UNMAPPED would be delivered as well.
          */
-#ifdef HAVE_BRK_SBRK
         if (events & UCM_EVENT_BRK) {
             UCM_FIRE_EVENT(events, UCM_EVENT_BRK, data, ucm_brk_checked(NULL));
         }
-#endif
     }
 
     if (events & (UCM_EVENT_MADVISE|UCM_EVENT_VM_UNMAPPED)) {

src/ucm/util/replace.c

@@ -55,8 +55,10 @@ UCM_DEFINE_REPLACE_FUNC(mremap, void*, MAP_FAILED, void*, size_t, size_t, int,
 #endif
 UCM_DEFINE_REPLACE_FUNC(shmat,   void*, MAP_FAILED, int, const void*, int)
 UCM_DEFINE_REPLACE_FUNC(shmdt,   int,   -1,         const void*)
-#ifdef HAVE_BRK_SBRK
+#ifdef HAVE_SBRK
 UCM_DEFINE_REPLACE_FUNC(sbrk,    void*, MAP_FAILED, intptr_t)
+#endif
+#ifdef HAVE_BRK
 UCM_DEFINE_REPLACE_FUNC(brk,     int,   -1,         void*)
 #endif
 UCM_DEFINE_REPLACE_FUNC(madvise, int,   -1,         void*, size_t, int)
@@ -124,8 +126,9 @@ int ucm_orig_shmdt(const void *shmaddr)
 
 #endif
 
-#ifdef HAVE_BRK_SBRK
+#ifdef HAVE_BRK
 _UCM_DEFINE_DLSYM_FUNC(brk, ucm_orig_dlsym_brk, ucm_override_brk, int, void*)
+#endif
 
 int ucm_orig_brk(void *addr)
 {
@@ -144,8 +147,10 @@ int ucm_orig_brk(void *addr)
     }
 }
 
+#ifdef HAVE_SBRK
 _UCM_DEFINE_DLSYM_FUNC(sbrk, ucm_orig_dlsym_sbrk, ucm_override_sbrk, void*,
                        intptr_t)
+#endif
 
 void *ucm_orig_sbrk(intptr_t increment)
 {
@@ -159,31 +164,6 @@ void *ucm_orig_sbrk(intptr_t increment)
                (void*)-1 : prev;
     }
 }
-#else
-int ucm_orig_brk(void *addr)
-{
-    (void)addr;
-    errno = ENOSYS;
-    return -1;
-}
-
-void *ucm_orig_sbrk(intptr_t increment)
-{
-    (void)increment;
-    errno = ENOSYS;
-    return MAP_FAILED;
-}
-
-int ucm_override_brk(void *addr)
-{
-    return ucm_orig_brk(addr);
-}
-
-void *ucm_override_sbrk(intptr_t increment)
-{
-    return ucm_orig_sbrk(increment);
-}
-#endif
 
 #else /* UCM_BISTRO_HOOKS */
 
@@ -196,13 +176,9 @@ UCM_DEFINE_DLSYM_FUNC(shmdt, int, const void*)
 
 void *ucm_get_current_brk()
 {
-#ifdef HAVE_BRK_SBRK
 #if HAVE___CURBRK
     return __curbrk;
 #else
     return ucm_brk_syscall(0);
 #endif
-#else
-    return NULL;
-#endif
 }

src/ucm/util/sys.c

@@ -377,7 +377,7 @@ char *ucm_concat_path(char *buffer, size_t max, const char *dir, const char *fil
 
 void *ucm_brk_syscall(void *addr)
 {
-#ifdef HAVE_BRK_SBRK
+#ifdef HAVE_DECL_SYS_BRK
     /* Return type is equivalent to full pointer size */
     UCS_STATIC_ASSERT(sizeof(syscall(0)) == sizeof(void*));