Skip to content
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

Merged
merged 3 commits into from
May 31, 2024
Merged

cpu/native: fix build with musl #18942

merged 3 commits into from
May 31, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 21, 2022

Contribution description

This changes a bunch of things that allows building with BOARD=native on 32-bit arches with the musl C lib, provided that libucontext-dev and pkg-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

make BOARD=native -C examples/default

On a 32 bit system with musl as C lib and pkg-config as well as libucontext-dev installed should now succeed. The generated binary on glibc based 32 bit or multiarch systems should not change compared to master.

Issues/PRs references

Fixes #18937

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: native Platform: This PR/issue effects the native platform labels Nov 21, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 21, 2022
Copy link
Member

@chrysn chrysn left a 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
Copy link
Member

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).

@riot-ci
Copy link

riot-ci commented Nov 21, 2022

Murdock results

✔️ PASSED

e93b5e4 core/thread: fix thread_measure_stack_free()

Success Failures Total Runtime
10160 0 10161 17m:22s

Artifacts

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 23, 2023
auto-merge was automatically disabled November 8, 2023 12:48

Merge queue setting changed

@Teufelchen1
Copy link
Contributor

What do you need to get this merged @maribu? 🙂

Comment on lines +122 to +126
#if __GLIBC__
static const bool _is_glibc = true;
#else
static const bool _is_glibc = false;
#endif
Copy link
Member Author

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);
Copy link
Member Author

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.

@benpicco
Copy link
Contributor

benpicco commented Feb 7, 2024

Looks like native64 is crashing in CI now 😕

@maribu maribu force-pushed the cpu/native branch 2 times, most recently from 5321059 to 9aa9df8 Compare May 31, 2024 07:42
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.
@github-actions github-actions bot added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: sys Area: System labels May 31, 2024
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label May 31, 2024
@github-actions github-actions bot added the Area: examples Area: Example Applications label May 31, 2024
*/
uintptr_t thread_measure_stack_free(const char *stack);
uintptr_t thread_measure_stack_free(const char *stack, size_t size);
Copy link
Member

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)

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member Author

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?

Copy link
Member Author

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 :)

@maribu maribu force-pushed the cpu/native branch 2 times, most recently from 829cf29 to dc79930 Compare May 31, 2024 14:37
chrysn added a commit to RIOT-OS/rust-riot-wrappers that referenced this pull request May 31, 2024
@maribu maribu force-pushed the cpu/native branch 2 times, most recently from 3164824 to 8b6192a Compare May 31, 2024 16:35
@maribu maribu added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels May 31, 2024
@github-actions github-actions bot added the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label May 31, 2024
@maribu
Copy link
Member Author

maribu commented May 31, 2024

Can I haz ACKs?

Copy link
Member

@chrysn chrysn left a 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).

*/
uintptr_t thread_measure_stack_free(const char *stack);
uintptr_t internal_measure_stack_free(const char *stack, size_t size);
Copy link
Member

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.

Copy link
Member Author

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`.
@github-actions github-actions bot removed the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label May 31, 2024
@maribu maribu added this pull request to the merge queue May 31, 2024
Merged via the queue into RIOT-OS:master with commit 886c6a2 May 31, 2024
25 checks passed
@maribu
Copy link
Member Author

maribu commented Jun 1, 2024

Hooray 🎉 Thx a bunch!

@maribu maribu deleted the cpu/native branch June 1, 2024 06:53
@benpicco
Copy link
Contributor

benpicco commented Jun 4, 2024

This breaks the stack free measurement :(

> ps
2024-06-04 16:27:07,709 # 	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
2024-06-04 16:27:07,718 # 	  - | isr_stack            | -        - |   - |    512 (  172) (  340) | 0x20000000 | 0x200001c8
2024-06-04 16:27:07,726 # 	  1 | main                 | running  Q |   7 |   1536 ( 1536) (    0) | 0x20000340 | 0x20000854 
2024-06-04 16:27:07,736 # 	  2 | pktdump              | bl rx    _ |   6 |   1472 ( 1472) (    0) | 0x20002fb0 | 0x200034c4 
2024-06-04 16:27:07,744 # 	  3 | ipv6                 | bl rx    _ |   4 |    960 (  960) (    0) | 0x20000a5c | 0x20000d54 
2024-06-04 16:27:07,752 # 	  4 | udp                  | bl rx    _ |   5 |    448 (  448) (    0) | 0x20003bfc | 0x20003cfc 
2024-06-04 16:27:07,761 # 	  5 | sam0_eth             | bl anyfl _ |   2 |    896 (  896) (    0) | 0x200013b8 | 0x2000167c 
2024-06-04 16:27:07,770 # 	  6 | RPL                  | bl rx    _ |   5 |   1024 ( 1024) (    0) | 0x200037b8 | 0x20003aec 
2024-06-04 16:27:07,776 # 	    | SUM                  |            |     |   6848 ( 6508) (  340)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Platform: native Platform: This PR/issue effects the native platform Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: needs >1 ACK Integration Process: This PR requires more than one ACK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cpu/native: support for musl
6 participants