Page MenuHomePhabricator

[libc] Add sigaction
ClosedPublic

Authored by abrachet on Mar 7 2020, 2:15 AM.

Details

Summary

This patch adds sigaction and the sa_restorer signal trampoline function __restore_rt

Diff Detail

Event Timeline

abrachet created this revision.Mar 7 2020, 2:15 AM
abrachet marked 3 inline comments as done.Mar 7 2020, 2:24 AM
abrachet added inline comments.
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.

abrachet updated this revision to Diff 248969.Mar 7 2020, 3:44 PM
abrachet edited the summary of this revision. (Show Details)
  • Moved inline asm out of c file into arch specific assembly file.
  • Use add_entrypoint_object for __restore target.
  • Added test for SIG_IGN.
abrachet updated this revision to Diff 248982.Mar 7 2020, 8:30 PM

Rename file to use capital S suffix to not manually need to specify that the C preprocessor should be run.

MaskRay added inline comments.Mar 7 2020, 11:24 PM
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.

abrachet updated this revision to Diff 249018.Mar 8 2020, 2:35 PM
  • 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
abrachet marked 3 inline comments as done.Mar 8 2020, 2:36 PM
abrachet updated this revision to Diff 249033.Mar 8 2020, 11:45 PM

Rename restore_rt_${arch}.S to restore_${arch}

sivachandra added inline comments.Mar 9 2020, 11:38 AM
libc/src/signal/linux/arch/__restore_x86_64.S
1

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.

abrachet updated this revision to Diff 249204.Mar 9 2020, 1:35 PM

Stop using assembly file all together

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

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.

abrachet updated this revision to Diff 249267.Mar 9 2020, 7:12 PM
abrachet edited the summary of this revision. (Show Details)

Implement __restore_rt in .cpp file. Compile with specific options.

abrachet marked an inline comment as done.Mar 9 2020, 7:23 PM

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;
#endif

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

abrachet updated this revision to Diff 250479.Mar 15 2020, 10:55 PM
abrachet marked an inline comment as done.

Implement Siva's suggestion to internally refer to our struct sigaction as struct __sigaction

abrachet marked 4 inline comments as done.Mar 15 2020, 10:57 PM
abrachet added inline comments.
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.

LGTM. I will write back before the end of today about the single object rule.

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.

abrachet marked 3 inline comments as done.Mar 17 2020, 12:22 AM
abrachet added inline comments.
libc/test/src/signal/sigaction_test.cpp
37

Does it crash with D76271 or just as is? My guess is its a problem with how __restore_rt is being compiled. Also, does it crash outside of GDB or just in GDB?

sivachandra added inline comments.Mar 17 2020, 9:31 AM
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 sigaction

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

abrachet updated this revision to Diff 250946.Mar 17 2020, 5:40 PM
  • Use add_object
  • Add errno_h dependency
sivachandra accepted this revision.Mar 17 2020, 9:55 PM
This revision is now accepted and ready to land.Mar 17 2020, 9:55 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 10:10 PM