[libc-commits] [PATCH] D75802: [libc] Add sigaction

[Alex Brachet via Phabricator via libc-commits]

abrachet marked an inline comment as done.
abrachet added a comment.

In D75802#1913439 <https://reviews.llvm.org/D75802#1913439>, @sivachandra wrote:

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



================
Comment at: libc/src/signal/linux/__restore.cpp:17
+extern "C" void __restore_rt()
+    __attribute__((no_sanitize("thread", "memory", "undefined", "fuzzer"),
+                   hidden));
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75802/new/

https://reviews.llvm.org/D75802