-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cpu/native: fix build with musl #18942
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me in a docker run --rm -it --network=host --platform linux/i386 alpine:edge
container.
On master:
/tmp/RIOT # make -C examples/hello-world
make: Entering directory '/tmp/RIOT/examples/hello-world'
musl libc (i386)
Version 1.2.3
Dynamic Program Loader
Usage: /lib/ld-musl-i386.so.1 [options] [--] pathname
Building application "hello-world" for "native" with MCU "native".
"make" -C /tmp/RIOT/boards/common/init
"make" -C /tmp/RIOT/boards/native
"make" -C /tmp/RIOT/boards/native/drivers
"make" -C /tmp/RIOT/core
"make" -C /tmp/RIOT/core/lib
"make" -C /tmp/RIOT/cpu/native
/tmp/RIOT/cpu/native/irq_cpu.c: In function 'native_isr_entry':
/tmp/RIOT/cpu/native/irq_cpu.c:365:68: error: 'REG_EIP' undeclared (first use in this function)
365 | _native_saved_eip = ((ucontext_t *)context)->uc_mcontext.gregs[REG_EIP];
| ^~~~~~~
/tmp/RIOT/cpu/native/irq_cpu.c:365:68: note: each undeclared identifier is reported only once for each function it appears in
make[2]: *** [/tmp/RIOT/Makefile.base:146: /tmp/RIOT/examples/hello-world/bin/native/cpu/irq_cpu.o] Error 1
make[1]: *** [/tmp/RIOT/Makefile.base:31: ALL--/tmp/RIOT/cpu/native] Error 2
make: *** [/tmp/RIOT/examples/hello-world/../../Makefile.include:739: application_hello-world.module] Error 2
make: Leaving directory '/tmp/RIOT/examples/hello-world'
On this branch but without libucontext-dev (because maybe there is a cheap way to make pkgconfig err fatally in this case -- otherwise just so that it can be found through full-text search in the issue tracker):
/tmp/RIOT # make -C examples/hello-world
make: Entering directory '/tmp/RIOT/examples/hello-world'
musl libc (i386)
Version 1.2.3
Dynamic Program Loader
Usage: /lib/ld-musl-i386.so.1 [options] [--] pathname
Building application "hello-world" for "native" with MCU "native".
"make" -C /tmp/RIOT/boards/common/init
"make" -C /tmp/RIOT/boards/native
"make" -C /tmp/RIOT/boards/native/drivers
"make" -C /tmp/RIOT/core
"make" -C /tmp/RIOT/core/lib
"make" -C /tmp/RIOT/cpu/native
"make" -C /tmp/RIOT/cpu/native/periph
"make" -C /tmp/RIOT/cpu/native/stdio_native
"make" -C /tmp/RIOT/drivers
"make" -C /tmp/RIOT/drivers/periph_common
"make" -C /tmp/RIOT/sys
"make" -C /tmp/RIOT/sys/auto_init
"make" -C /tmp/RIOT/sys/libc
"make" -C /tmp/RIOT/sys/preprocessor
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: warning: /tmp/RIOT/examples/hello-world/bin/native/hello-world.elf has a LOAD segment with RWX permissions
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/irq_cpu.o: in function `native_isr_entry':
/tmp/RIOT/cpu/native/irq_cpu.c:340: undefined reference to `makecontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/irq_cpu.o: in function `native_interrupt_init':
/tmp/RIOT/cpu/native/irq_cpu.c:520: undefined reference to `getcontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/cpu/native/irq_cpu.c:538: undefined reference to `makecontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/native_cpu.o: in function `isr_cpu_switch_context_exit':
/tmp/RIOT/cpu/native/native_cpu.c:175: undefined reference to `setcontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/native_cpu.o: in function `isr_thread_yield':
/tmp/RIOT/cpu/native/native_cpu.c:227: undefined reference to `setcontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/native_cpu.o: in function `thread_stack_init':
/tmp/RIOT/cpu/native/native_cpu.c:140: undefined reference to `getcontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/cpu/native/native_cpu.c:153: undefined reference to `makecontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/native_cpu.o: in function `cpu_switch_context_exit':
/tmp/RIOT/cpu/native/native_cpu.c:196: undefined reference to `makecontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/cpu/native/native_cpu.c:197: undefined reference to `setcontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/native_cpu.o: in function `thread_yield_higher':
/tmp/RIOT/cpu/native/native_cpu.c:245: undefined reference to `makecontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/cpu/native/native_cpu.c:246: undefined reference to `swapcontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/native_cpu.o: in function `native_cpu_init':
/tmp/RIOT/cpu/native/native_cpu.c:255: undefined reference to `getcontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/cpu/native/native_cpu.c:262: undefined reference to `makecontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/syscalls.o: in function `_native_syscall_leave':
/tmp/RIOT/cpu/native/syscalls.c:142: undefined reference to `makecontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/cpu/native/syscalls.c:143: undefined reference to `swapcontext'
/usr/lib/gcc/i586-alpine-linux-musl/12.2.1/../../../../i586-alpine-linux-musl/bin/ld: /tmp/RIOT/examples/hello-world/bin/native/cpu/tramp.o: in function `_native_sig_leave_tramp':
/tmp/RIOT/cpu/native/tramp.S:105: undefined reference to `swapcontext'
collect2: error: ld returned 1 exit status
make: *** [/tmp/RIOT/examples/hello-world/../../Makefile.include:730: /tmp/RIOT/examples/hello-world/bin/native/hello-world.elf] Error 1
make: Leaving directory '/tmp/RIOT/examples/hello-world'
On this branch with libucontext-dev installed: hello-world builds and runs (it terminates after the hello-world message; that may just be due to the container's terminal settings). Other programs still don't work (eg. filesystem and gcoap have trouble around struct timeval), but hey it's a start to get it working on more platforms.
@@ -13,10 +13,16 @@ | |||
* @author Ludwig Knüpfer <[email protected]> | |||
*/ | |||
|
|||
/* __USE_GNU for gregs[REG_EIP] access under glibc | |||
* _GNU_SOURCE for REG_EIP and strsignal() under musl */ | |||
#define __USE_GNU |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling these to the top is a good call anyway (and when done in a .c file it can even work, distinct from what I tried in #18681).
Merge queue setting changed
What do you need to get this merged @maribu? 🙂 |
#if __GLIBC__ | ||
static const bool _is_glibc = true; | ||
#else | ||
static const bool _is_glibc = false; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may look like a good use case for IS_ACTIVE()
. However, IS_ACTIVE
only works if the macro is either 1
, 0
, or undefined. __GLIBC__
however is 2
on current glibc based systems, so it just won't work.
@@ -262,7 +267,8 @@ void native_cpu_init(void) | |||
err(EXIT_FAILURE, "native_cpu_init: getcontext"); | |||
} | |||
|
|||
end_context.uc_stack.ss_sp = __end_stack; | |||
end_context.uc_stack.ss_sp = malloc(SIGSTKSZ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no longer statically allocating? Because
# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
which is not a constant number, but function call issued at runtime. There still is a legacy compat mode in glibc that just defines this as a large constant, but moving the _GNU_SOURCE
up disabled this compat.
Looks like |
5321059
to
9aa9df8
Compare
This changes a bunch of things that allows building with the musl C lib, provided that `libucontext-dev` and `pkg-config` are installed. Note that installing libucontext makes absolutely zero sense on C libs that do natively provide this deprecated System V API, such as glibc. Hence, it no sane glibc setup is expected to ever have libucontext installed. A main pain point was that argv and argc are expected to be passed to init_fini handlers, but that is actually a glibc extension. This just parses `/proc/self/cmdline` by hand to populate argv and argc during startup, unless running on glibc.
core/include/thread.h
Outdated
*/ | ||
uintptr_t thread_measure_stack_free(const char *stack); | ||
uintptr_t thread_measure_stack_free(const char *stack, size_t size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just pass a thread struct pointer here instead? That'd make it a typed pointer that's easier to document and harder to mix up. (Either works for RIOT-OS/rust-riot-wrappers#96 which is where I'm coming from)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was also my first take. But ESP is using this function to measure the stack usage of the ISR stack, for which no thread_t
is existing.
It is also a bit cumbersome for the idle thread statistics, as no thread_t
pointer is available there. It would be possible to use thread_get(1)
, but that would bake in an assumption that no threads are created before kernel_init (e.g. not in periph_init()
), which currently would work provided the "don't yield" flag is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks for the explanation. If this were a higher level API I'd suggest having an internal low-level tool and high-level functions for "of thread *thread", "of the ISR thread" and "of the idle pseudothread", but as this is mostly used internally, fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should rename it to measure_stack_free()
and provide a thread_measure_stack_free()
as static inline wrapper for convenience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, did just that. The last commit points rust_riot_wrappers
to the PR so that I can see if this now still segfaults on non-musl systems.
I intend to drop that commit, as pointing to some github.com/maribu/*
is IMO not ideal. Who is that maribu, anyway, and how could we trust his personal repos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful :) With the rust-riot-wrappers
working with both variants, I reordered the commits to bump that first so that git biset
is more fun :)
829cf29
to
dc79930
Compare
3164824
to
8b6192a
Compare
Can I haz ACKs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK reaffirmed. I didn't test things again, trusting your testing. The rust-riot-wrappers update just pulls in the one fix that is needed for the API change. The API change is a pretty one now (programs will not accidentally compile because pointer content doesn't match).
core/include/thread.h
Outdated
*/ | ||
uintptr_t thread_measure_stack_free(const char *stack); | ||
uintptr_t internal_measure_stack_free(const char *stack, size_t size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably have gone with thread_measure_stack_free_internal
so that the (publicly visible) symbol is still easily attributed – but either works, and I don't think we have a precise policy on internal-but-visible symbols.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comprise: Let's go for measure_stack_free_internal()
.
The thread_
prefix would indicate it is not suitable for ISR stacks, but that is point of having this second flavour.
`thread_measure_stack_free()` previously assumed that reading past the stack is safe. When the stack was indeed part of a thread, the `thread_t` structure is put after the stack, increasing the odds of this assumption to hold. However, `thread_measure_stack_free()` could also be used on the ISR stack, which may be allocated at the end of SRAM. A second parameter had to be added to indicate the stack size, so that reading past the stack can now be prevented. This also makes valgrind happy on `native`/`native64`.
Hooray 🎉 Thx a bunch! |
This breaks the stack free measurement :(
|
Contribution description
This changes a bunch of things that allows building with
BOARD=native
on 32-bit arches with the musl C lib, provided thatlibucontext-dev
andpkg-config
is installed.Note that installing libucontext makes absolutely zero sense on C libs that do natively provide this deprecated System V API, such as glibc. Hence, it no sane glibc setup is expected to ever have libucontext installed.
Testing procedure
On a 32 bit system with musl as C lib and
pkg-config
as well aslibucontext-dev
installed should now succeed. The generated binary on glibc based 32 bit or multiarch systems should not change compared tomaster
.Issues/PRs references
Fixes #18937