This patch adds sigaction and the sa_restorer signal trampoline function __restore_rt
Details
Diff Detail
Event Timeline
| libc/config/linux/signal.h.in | ||
|---|---|---|
| 11 | This is very not ideal, but the Linux headers expose a different struct sigaction than POSIX expects, it has no sa_action member. | |
| libc/spec/stdc.td | ||
| 217 | I moved this in this patch because I realized sigset_t doesn't belong here but rather posix.td, I can do this in a separate patch if reviewers want. | |
| libc/src/signal/linux/sigaction.cpp | ||
| 21 | To elaborate more on why we can't use a function written in C, we need a way to guarantee that no stack allocations can be made, to my knowledge there is no attribute which satisfies this. The closest is optimize("O1") which we can assume wouldn't bother pushing anything but clang doesn't have this. Also see __restore_rt.s, to make this useable with gdb there are a lot of prerequisites which must be met that we cannot do in C (at least easily) unfortunately. | |
- Moved inline asm out of c file into arch specific assembly file.
- Use add_entrypoint_object for __restore target.
- Added test for SIG_IGN.
Rename file to use capital S suffix to not manually need to specify that the C preprocessor should be run.
| libc/src/signal/linux/arch/__restore_rt_x86_64.S | ||
|---|---|---|
| 21 ↗ | (On Diff #248982) | Delete them. |
| 24 ↗ | (On Diff #248982) | There needs to be a reference to musl/src/signal/x86_64/restore.s commit 54991729 unless I am mistaken. Can you figure out whether the gdb bug has been fixed? |
| 33 ↗ | (On Diff #248982) | Use . instead of an end symbol. The end symbol wastes a symbol table entry. |
- Move comment explaining why __restore_rt is handwritten to the c file so it doesn't need to be repeated in every assembly implementation
- Stop using Intel syntax
- Remove nop
| libc/src/signal/linux/arch/__restore_x86_64.S | ||
|---|---|---|
| 1 ↗ | (On Diff #249033) | While I understand (thanks to your comments and other references) why this is required, adding an assembly file in LLVM libc is a complete no go at least at this point in time. For example, options like inline assembly are OK but I do think it helps here. Sorry, I am not trying to discourage here. In fact, I totally appreciate the effort you put in to do the investigation because it hastened my own learning. I want to make sure we have exhausted all other options. Will do my own research and get back to you. Feel free to propose other options if you can think of any. |
Going to quote this here because the file it was commented on has since been deleted
While I understand (thanks to your comments and other references) why this is required, adding an assembly file in LLVM libc is a complete no go at least at this point in time. For example, options like inline assembly are OK but I do think it helps here.
Sorry, I am not trying to discourage here. In fact, I totally appreciate the effort you put in to do the investigation because it hastened my own learning. I want to make sure we have exhausted all other options. Will do my own research and get back to you. Feel free to propose other options if you can think of any.
I can't think of any way to do this without handwriting this function. If we don't want an assembly file thats fine but it would need to be inline asm. My personal opinion is that inline asm will get uglier in the future when more architectures are supported but thats not a huge concern right now.
There's just no portable way that I know of to ensure the compile will never make any stack allocations. It's not like like the memcpy intrinsic that @gchatelet added either where it is nice to have, so even if we add a never_stack_alloc attribute to clang (which sounds non trivial), it wouldn't just be slower on other compilers but not work at all. Any stack allocation and the kernel cannot return from the signal, I've gotten either SIGSEGV or SIGBUS when implementing this in C.
Those errors were with ASan and UBSan turned on, and compiled at O1. When sanitizers are turned off and compiling at O1 this function works when written in C (breaks at O0. One idea, is put this in a separate C file, implement it in C and then use add_custom_command so we always know how it is being compiled, ie no sanitizers no instrumentation and optimizations always turned on. This is hacky though because if someone has an alias for their compiler like alias clang="clang -fsanitize=address" then that solution breaks too. Not to mention there is no guarantee that with optimizations turned on the compiler will not emit any push instructions (although its a safe bet that none would).
Firstly, I should have mentioned something more in my previous message. The only inline assembly we want to restrict to for now is the syscall layer. If such a restriction makes it impossible to do certain things, then we will consider relaxing it case-by-case.
Coming back to this patch, if we require a certain optimization level, I think that's reasonable. FWIW, glibc has such requirements at many places.
If I have a function like this:
__attribute__((no_sanitize("address"))) // Add the full list of sanitizers which can affect us
void my_syscall(void) {
__llvm_libc::syscall(5);
__builtin_unreachable();
}And, I compile with -O3 -fomit-frame-pointer, it produces:
0000000000000000 <_Z10my_syscallv>: 0: b8 05 00 00 00 mov $0x5,%eax 5: 0f 05 syscall
So, it seems to me like the only additional thing we need is to teach the compiler is to use rax instead of eax. That too, we need it so that it works with gdb? Other way around, can we teach gdb/lldb to work LLVM libc? I am OK to wait on "fixing" this aspect.
So, in conclusion, if we can put this new function in a .cpp file as you suggest, and set up the right compiler options, we are good for now? Our build rules are probably not setup to handle all this correctly/conveniently, so they might need some extension.
Done I like this solution a lot more :). Also I found a great flag -Wframe-larger-than=N so we can get an error for stack frames bigger than N, which in this case I have as 0.
With regards to using the 64 bit registers, I'm not bothered by this. There are no situations I can imagine anyone would ever actually need a backtrace from inside of __restore_rt so wether gdb recognizes it or not is not a big deal from my point of view. Not mangling __restore_rt's name helps a bit (basically only for internal development though). I have made it hidden but I could keep it as global if this is a problem and users (of a llvmlibc.so) really need gdb to recognize it.
| libc/src/signal/linux/__restore.cpp | ||
|---|---|---|
| 17 ↗ | (On Diff #249267) | These are the sanitizers I found which will affect the stack. https://godbolt.org/z/8ExGxT Probably this isn't needed since we are compiling with separate flags than the rest of the project, but I suppose it doesn't hurt. |
Sorry for the long delay here. I am feeling much better now and should be more responsive.
| libc/config/linux/signal.h.in | ||
|---|---|---|
| 11 | What do you think of using the other way round scheme. As in, in our public signal.h header, we will define sigaction like this: struct __sigaction {
...
};
#ifndef __LLVM_LIBC_INTERNAL_SIGACTION
typedef __sigaction sigaction;
#endifThis will allow us to include our public header in internal code as: #define __LLVM_LIBC_INTERNAL_SIGACTION #include "include/signal.h" Throughout the internal implementation code, we will use struct __sigaction instead of struct sigaction. This way, we don't have to mess with the kernel headers. Does this make sense? | |
| libc/src/signal/linux/CMakeLists.txt | ||
| 15 | I have a similar requirement for a change I have been working on. I will share more on this soon. I am OK with this setup as is, but I have a feeling we can make use of similar items elsewhere as well. | |
Implement Siva's suggestion to internally refer to our struct sigaction as struct __sigaction
| libc/config/linux/signal.h.in | ||
|---|---|---|
| 11 | How does this look? | |
| libc/src/signal/linux/CMakeLists.txt | ||
| 15 | I don't quite love how this is currently so I'd be happy to wait on that patch. Especially because I also end up touching cmake/modules/LLVMLibcRules.cmake in this patch which is not ideal. The only thing I can think of which depends on this patch is implementing signal and fixing abort, which I'm fine waiting on. | |
I just uploaded https://reviews.llvm.org/D76271. I tested it with this patch to make sure it is working as expected.
| libc/test/src/signal/CMakeLists.txt | ||
|---|---|---|
| 20 | Add errno_h as dependency. | |
| libc/test/src/signal/sigaction_test.cpp | ||
| 37 | This test is crashing for me. I have not investigated why, but GDB is telling me that SIGUSR1 is not getting caught after line 49. | |
| libc/test/src/signal/sigaction_test.cpp | ||
|---|---|---|
| 37 | Ah, looks like a bad interaction between the NOHANG race and this test. I used Paula's patch and the test passes. So, the way I use the build rules is this: add_object(
__restore
SRC
__restore.cpp
COMPILE_OPTIONS
-fomit-frame-pointer
-O3
-Wframe-larger-than=0
DEPENDS
linux_syscall_h
sys_syscall_h
)
add_entrypoint_object(
sigaction
SRCS
sigaction.cpp
HDRS
signal.h
../sigaction.h
DEPENDS
sys_syscall_h
linux_syscall_h
signal_h
SPECIAL_OBJECTS
__restore
)We need the construct like SPECIAL_OBJECTS because it cannot be a normal dep: we need to get the object file out of it. With the above rules, the symtab in sigaction.o is like this: Symbol table '.symtab' contains 21 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 0000000000000000 0 SECTION LOCAL DEFAULT 1
2: 0000000000000000 0 SECTION LOCAL DEFAULT 2
3: 0000000000000000 0 SECTION LOCAL DEFAULT 3
4: 0000000000000000 0 SECTION LOCAL DEFAULT 4
5: 0000000000000000 0 SECTION LOCAL DEFAULT 5
6: 0000000000000000 0 SECTION LOCAL DEFAULT 6
7: 0000000000000000 0 SECTION LOCAL DEFAULT 7
8: 0000000000000000 0 SECTION LOCAL DEFAULT 8
9: 0000000000000000 0 SECTION LOCAL DEFAULT 9
10: 0000000000000000 0 SECTION LOCAL DEFAULT 10
11: 0000000000000000 0 SECTION LOCAL DEFAULT 11
12: 0000000000000000 0 SECTION LOCAL DEFAULT 12
13: 0000000000000000 0 FILE LOCAL DEFAULT ABS sigaction.cpp
14: 0000000000000050 77 FUNC LOCAL DEFAULT 1 _ZN11__llvm_libcL13copySigactionI11__sigaction9sigactionEEvRT_RKT0_
15: 0000000000000000 76 FUNC LOCAL DEFAULT 1 _ZN11__llvm_libcL13copySigactionI9sigaction11__sigactionEEvRT_RKT0_
16: 0000000000000000 0 FILE LOCAL DEFAULT ABS __restore.cpp
17: 00000000000000a0 7 FUNC GLOBAL DEFAULT 1 __restore_rt
18: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND _ZN11__llvm_libc16__errno_locationEv
19: 0000000000000000 461 FUNC GLOBAL DEFAULT 2 _ZN11__llvm_libc9sigactionEiPK11__sigactionPS0_
20: 0000000000000000 0 FUNC GLOBAL DEFAULT 2 sigactionThat confirms __restore.o is getting packed into sigaction.o. Next, objdump shows this: 00000000000000a0 <__restore_rt>: a0: b8 0f 00 00 00 mov $0xf,%eax a5: 0f 05 syscall This confirms that the compile options are being picked up alright. | |
This is very not ideal, but the Linux headers expose a different struct sigaction than POSIX expects, it has no sa_action member.