Removing arch_prctl from syscall and creating architecture specific musl calls #2350
Conversation
|
Hey, this is awesome! Really good solid cleanup that also adds real value in its own right by removing a dependency on the syscall instruction which we really don't want to support. My only real concern here, besides making error messages more helpful, is why you would want to move the musl common header with strace into the API - read my comment on that above. We can discuss that on discord if you want. |
torgeiru
force-pushed
the
remove-arch-prctl-call
branch
2 times, most recently
from
897cad1 to
17d5472
Compare
…usl calls Set_thread_area and arch_prctl are functions used for setting the thread pointer. This is used for thread local storage. Currently arch_prctl goes through a system call trap. This patches musl so that it goes through a typical function call instead. Also, this moves the implementation of these function calls to architecture, removing the need for macros.
torgeiru
force-pushed
the
remove-arch-prctl-call
branch
from
17d5472 to
848ae76
Compare
src/arch/i686/arch_musl.cpp
Outdated
|
|
||
| extern "C" | ||
| long sys_arch_prctl(int code, uintptr_t ptr) { | ||
| os::panic("This should not happen!"); |
There was a problem hiding this comment.
Fair, but this message should be more descriptive. Not a deal breaker, but please don't forget to change it, we want error messages to be helpful and self explanatory.
There was a problem hiding this comment.
I'm not sure we want this to expose this. In the API folder you want to have anything you are willing to support long term, and that you actively think is a good idea for service developers to use. That said - if you can provide a good reason for why someone implementing a service would want to use strace to wrap their own stuff, maybe this should be renamed strace.hpp instead. But in general I think our API should get smaller, not bigger.
src/arch/aarch64/arch_musl.cpp
Outdated
|
|
||
| extern "C" | ||
| long sys_arch_prctl(int code, uintptr_t ptr) { | ||
| os::panic("This should not happen!"); |
There was a problem hiding this comment.
Please explain why. Like because multithreading is not supported on this arch or something. As helpful as you can.
| break; | ||
| default: | ||
| kprintf("<syscall entry> no %lu (a1=%#lx a2=%#lx a3=%#lx a4=%#lx a5=%#lx) \n", | ||
| kprintf("<syscall entry> no %lu (a1=%#lx a2=%#lx a3=%#lx a4=%#lx a5=%#lx) \n", |
There was a problem hiding this comment.
Should we panic here? I don't think we ever want to use the syscall instruction so if some library does (like if libc decides to introduce it in more places) this will loop pretty mysterious. Not a dealbreaker though - this is what the kernel is doing today, so it's fine. But if you agree maybe use the opportunity to fail early.
2 participants
Set_thread_area and arch_prctl are functions
used for setting the thread pointer. This is used
for thread local storage.
Currently arch_prctl goes through a system call trap. This patches musl so that it goes through a typical function call instead.
Also, this moves the implementation of these
function calls to architecture, removing the need for macros.