nemuTUI / nemu

Ncurses UI for QEMU
BSD 2-Clause "Simplified" License
317 stars 22 forks source link

musl libc #87

Closed dm9pZCAq closed 3 years ago

dm9pZCAq commented 3 years ago

cmake

i get this error on musl libc:

CMake Error at cmake/Modules/GetGLibcVersion.cmake:13 (message):
  Unknown glibc version:
Call Stack (most recent call first):
  CMakeLists.txt:81 (get_glibc_version)

src

also it seems that musl don't have on_exit function:

nm -D /usr/lib/libc.so | rg exit
0000000000059f60 T __cxa_atexit
00000000000ba0e0 T _exit
0000000000059e00 T at_quick_exit
000000000005a020 T atexit
000000000005a120 T exit
00000000000b2aa0 W pthread_exit
000000000005a140 T quick_exit
00000000000b5d20 T thrd_exit

musl?

you can easily do tests with musl with alpine in docker:

docker run --rm -it alpine sh -c 'apk add binutils && nm -D /lib/libc.* | grep exit'
grafin commented 3 years ago

mostly for lulz (we target only glibc...) i've tryed building nemu in alpine container (i've patched GetGlibcVersion). patch:

diff --git a/cmake/Modules/GetGLibcVersion.cmake b/cmake/Modules/GetGLibcVersion.cmake
index 7a1c4c7..5b524da 100644
--- a/cmake/Modules/GetGLibcVersion.cmake
+++ b/cmake/Modules/GetGLibcVersion.cmake
@@ -11,6 +11,20 @@ macro(get_glibc_version)
     OUTPUT_VARIABLE GLIBC_VERSION
     OUTPUT_STRIP_TRAILING_WHITESPACE)

+  if(NOT GLIBC_VERSION MATCHES "^[0-9.]+$")
+    execute_process(
+      COMMAND ${CMAKE_C_COMPILER} -print-file-name=libc.musl-x86_64.so.1
+      OUTPUT_VARIABLE MUSL
+      OUTPUT_STRIP_TRAILING_WHITESPACE)
+
+    execute_process(
+      COMMAND ${MUSL}
+      ERROR_VARIABLE MUSL_OUT
+    )
+    string(REGEX REPLACE ".*Version ([0-9]*.[0-9]*.[0-9]*).*"  "\\1" MUSL_VERSION ${MUSL_OUT})
+    set(GLIBC_VERSION ${MUSL_VERSION})
+  endif()
+
   if(NOT GLIBC_VERSION MATCHES "^[0-9.]+$")
     message(FATAL_ERROR "Unknown glibc version: ${GLIBC_VERSION}")
     unset(GLIBC_VERSION)

cmake.log make.log

I'm not really interested in fixing all of that just for musl compatability. You are welcome to fix it it send us a PR. I'll close this issue for now, feel free to ask any questions thou, I'll try my best to help you.

dm9pZCAq commented 3 years ago

COMMAND ${CMAKE_C_COMPILER} -print-file-name=libc.musl-x86_64.so.1

on Gentoo it's ld-musl-x86_64.so.1

just for musl

it's used in many distros (alpine, void, gentoo, kiss, openwrt, ...) and qemu supports it too

You are welcome to fix it it send us a PR

i will try to fix on_exit error, but i'm not so familiar with cmake

also i think glibc version should be checked in header

#ifdef __GLIBC__
# if __GLIBC__ >= 2
#  define NM_WITH_SENDFILE
#  if __GLIBC_MINOR__ >= 18
#   define _DEFAULT_SOURCE
#  else
#   define _XOPEN_SOURCE=700
#   define _BSD_SOURCE
#  endif
#else
/* musl don't have macroses to check version */
#endif

or the easiest way: just define all possibly needed macroses

grafin commented 3 years ago

on Gentoo it's ld-musl-x86_64.so.1 :с

:) well, I'll try to figure out a way to make that flexible, that's not the big problem. For now you can just use my ugly patch to experiment with it.

it's used in many distros (alpine, void, gentoo, kiss, openwrt, ...) and qemu supports it too

I have zero experience with it, and it would be too much effort for me right now

or the easiest way: just define all possibly needed macroses

the cleanest way would be to find out that we build against musl during cmake step, and pass something like -DMUSL=1.2.2, find out what _SOURCE type we should set for musl and set it when MUSL is defined.

dm9pZCAq commented 3 years ago

I have zero experience with it

it's not so different from glibc, just missing some "unneeded" functions

on_exit.patch:

diff --git a/src/nm_mon_daemon.c b/src/nm_mon_daemon.c
index a2bce32..4c52dd6 100644
--- a/src/nm_mon_daemon.c
+++ b/src/nm_mon_daemon.c
@@ -13,6 +13,7 @@
 #include <time.h> /* nanosleep(2) */
 #include <pthread.h>
 #include <mqueue.h>
+#include <assert.h>

 #include <json.h>

@@ -44,12 +45,27 @@ typedef struct nm_clean_data {
     { NM_MON_VMS_INIT, NULL, NULL, NULL, NM_THR_CTRL_INIT, NM_THR_CTRL_INIT }

 #if defined (NM_OS_LINUX)
-static void nm_mon_cleanup(int rc, void *arg)
+
+#if !defined (__GLIBC__)
+static nm_clean_data_t *clean_ptr = NULL;
+#endif
+
+static void nm_mon_cleanup(int
+#if !defined(__GLIBC__)
+        __attribute__((__unused__))
+#endif
+        rc, void *arg)
 {
     nm_clean_data_t *data = arg;
     const nm_cfg_t *cfg = nm_cfg_get();

-    nm_debug("mon daemon exited: %d\n", rc);
+    nm_debug("mon daemon exited"
+#if defined (__GLIBC__)
+            ": %d\n", rc
+#else
+            "\n"
+#endif
+       );

     nm_vect_free(data->vms.list, NULL);
     nm_vect_free(data->vm_list, nm_str_vect_free_cb);
@@ -64,10 +80,25 @@ static void nm_mon_cleanup(int rc, void *arg)
 #endif

     if (unlink(cfg->daemon_pid.data) != 0) {
-        nm_debug("error delete mon daemon pidfile: %s\n", strerror(rc));
+        nm_debug("error delete mon daemon pidfile"
+#if defined (__GLIBC__)
+                 ": %s\n", strerror(rc)
+#else
+                 "\n"
+#endif
+       );
     }
     nm_exit_core();
 }
+
+#if !defined (__GLIBC__)
+static inline void nm_atexit_mon_cleanup(void)
+{
+    assert(!!clean_ptr);
+    nm_mon_cleanup(-1, clean_ptr);
+}
+#endif
+
 #endif /* NM_OS_LINUX */

 void *nm_qmp_worker(void *data)
@@ -429,8 +460,15 @@ void nm_mon_loop(void)
     clean.vm_list = &vm_list;
     clean.qmp_worker = &qmp_thr;
     clean.api_server = &api_srv;
+    clean_ptr = &clean;

-    if (on_exit(nm_mon_cleanup, &clean) != 0) {
+    if (
+#if defined (__GLIBC__)
+        on_exit(nm_mon_cleanup, &clean)
+#else
+        atexit(nm_atexit_mon_cleanup)
+#endif
+        != 0) {
         fprintf(stderr, "%s: on_exit(3) failed\n", __func__);
         nm_exit(EXIT_FAILURE);
     }

if it's ok for you, then i add PR with this

grafin commented 3 years ago

As far as I understood: the idea is to store nm_mon_cleanup argument as static variable, and use atexit instead of on_exit. In my opinion the glibc specific code is not needed at all, we can always use atexit. It would be cleaner and still portable. So my suggestions are: 1) just remove all the if defined(_GLIBC) etc, just always use atexit version 2) make nm_mon_cleanup just return if arg==NULL, and remove assert call (we dont use it)

grafin commented 3 years ago

Thanks @dm9pZCAq for contribution. I'll check that nemu can be built with musl, write cmake recipes for it and close this issue.

grafin commented 3 years ago
  1. reworked our cmake routines to actually run compiler tests, and don't rely on libc version.
  2. fixed several bugs (including 1 UB), which I've found during musl builds and tests

In the end supporting musl was great idea! Thanks @dm9pZCAq again.