BUILD: Adjusted BRK/SBRK conditionals to review feedback.

Author: GenericRikka

config/m4/ucm.m4

@@ -8,28 +8,29 @@
 #
 # 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"])
@@ -63,14 +64,8 @@ AC_CHECK_DECLS([getauxval], [], [],
 # SYS_xxx macro
 #
 mmap_hooks_happy=yes
-AC_CHECK_DECLS([SYS_mmap,
-                SYS_munmap,
-                SYS_mremap,
-                SYS_brk,
-                SYS_madvise],
-               [],
-               [mmap_hooks_happy=no], dnl mmap syscalls are not defined
-               [#include <sys/syscall.h>])
+AC_CHECK_DECLS([SYS_mmap, SYS_munmap, SYS_mremap, SYS_madvise, SYS_brk],
+               [], [mmap_hooks_happy=no], [#include <sys/syscall.h>])
 
 shm_hooks_happy=yes
 AC_CHECK_DECLS([SYS_shmat,
@@ -89,9 +84,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,7 @@ 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
+#ifdef HAVE_SBRK
 static void ucm_malloc_sbrk(ucm_event_type_t event_type,
                             ucm_event_t *event, void *arg)
 {
@@ -616,9 +606,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,11 +811,7 @@ 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
+#ifdef HAVE_SBRK
     static ucm_event_handler_t sbrk_handler = {
         .events   = UCM_EVENT_SBRK,
         .priority = 1000,
@@ -857,7 +840,21 @@ 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);

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
+#if defined(HAVE_BRK) && defined(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,7 +226,7 @@ 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
+#ifdef HAVE_BRK
         if (events & UCM_EVENT_BRK) {
             UCM_FIRE_EVENT(events, UCM_EVENT_BRK, data, ucm_brk_checked(NULL));
         }

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,7 +126,7 @@ 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*)
 
 int ucm_orig_brk(void *addr)
@@ -143,7 +145,21 @@ int ucm_orig_brk(void *addr)
         return 0;
     }
 }
+#else
+int ucm_orig_brk(void *addr)
+{
+    (void)addr;
+    errno = ENOSYS;
+    return -1;
+}
+
+int ucm_override_brk(void *addr)
+{
+    return ucm_orig_brk(addr);
+}
+#endif
 
+#if defined(HAVE_SBRK) && defined(HAVE_BRK)
 _UCM_DEFINE_DLSYM_FUNC(sbrk, ucm_orig_dlsym_sbrk, ucm_override_sbrk, void*,
                        intptr_t)
 
@@ -160,25 +176,13 @@ void *ucm_orig_sbrk(intptr_t increment)
     }
 }
 #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);
@@ -196,13 +200,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*));