[libc] Add __attribute__((always_inline)) to x86_64 syscall functions.

[sivachandra]

Summary

Some syscalls like SYS_clone do not tolerate a return instruction after
the syscall instruction. Marking the syscall functions with the
always_inline attribute accommodates such syscalls as inlining
eliminates the return instruction.

[sivachandra]

Remove an unintentional change that slipped through.

[abrachet]

libc/config/linux/x86_64/syscall.h.inc

17

I guess inline isn't necessary to keep anymore?

81

No space here __arg1,long it looks like. I suggest just running clang-format.

90

Ditto to clang-format this line is too long.

[sivachandra]

Fix format.

[sivachandra]

libc/config/linux/x86_64/syscall.h.inc

17

I do not know if its unnecessary. I have looked at examples in glibc and they seem to be using the inline keyword along with the inline attribute. But, there are also a small number of examples which only use the attribute and no keyword.

If you are confident the keyword is not necessary anymore, I will remove it :)

90

I do run clang-format with every change. However, since the names of these files do not end in .h, clang-format ignores them. I will play with renaming these files just to get clang-format to look at them, and then revert to the old name. Will do so in different change.

[abrachet]

libc/config/linux/x86_64/syscall.h.inc

17

We should just keep it then. It doesn't hurt certainly.

90

Thats a pain. It looks like there is an --assume-filename=<string> flag, I haven't used this yet but maybe that would be easier? https://clang.llvm.org/docs/ClangFormat.html

[MaskRay]

libc/config/linux/x86_64/syscall.h.inc

17

We should keep it. The inline specifier allows it to have more than one definition in all translation units. always_inline does not have the semantic.

[sivachandra]

Thanks for the review. I will put this in soon. I will see what I can do about clang-format in a different CL.

[abrachet]

Shouldn't these all be static as well, actually?

[sivachandra]

In D72102#1806672, @abrachet wrote:

Shouldn't these all be static as well, actually?

We really want these functions to be inlined. So, I am not sure how static helps in such scenarios. If they are not inlined, there are probably some advantages in ELF land, but we end up with multiple copies.

[abrachet]

In D72102#1806753, @sivachandra wrote:

In D72102#1806672, @abrachet wrote:

Shouldn't these all be static as well, actually?

We really want these functions to be inlined. So, I am not sure how static helps in such scenarios. If they are not inlined, there are probably some advantages in ELF land, but we end up with multiple copies.

Neither inline nor __attribute__((always_inline)) guarantee the symbol will not be emitted. From a quick test it looks like adding inline with always_inline never emitted the symbol but neither of these on their own have have that behavior. https://godbolt.org/z/R5Q-Ft. This is consistent with basically every compiler on godbolt, so it might be safe to leave static off and keep both inline and __attribute__((always_inline)) but I don't understand why these two together would mandate this so I wouldn't personally feel safe relying on that. I'm sure @MaskRay knows, though.

[sivachandra]

In D72102#1806840, @abrachet wrote:

In D72102#1806753, @sivachandra wrote:

In D72102#1806672, @abrachet wrote:

Shouldn't these all be static as well, actually?

We really want these functions to be inlined. So, I am not sure how static helps in such scenarios. If they are not inlined, there are probably some advantages in ELF land, but we end up with multiple copies.

Neither inline nor __attribute__((always_inline)) guarantee the symbol will not be emitted. From a quick test it looks like adding inline with always_inline never emitted the symbol but neither of these on their own have have that behavior. https://godbolt.org/z/R5Q-Ft. This is consistent with basically every compiler on godbolt, so it might be safe to leave static off and keep both inline and __attribute__((always_inline)) but I don't understand why these two together would mandate this so I wouldn't personally feel safe relying on that. I'm sure @MaskRay knows, though.

IIUC, if a direct call to function marked always_inline does not cause inlining, it is a diagnostic error. For indirect function calls though, the compiler will not inline and will also not report it. I think we have examples for both the scenarios in the libc tree: the mmap and munmap implementations make a direct call, while the x86_64 specific test takes the address of these syscall functions.

[miscco]

clang-format comment

libc/config/linux/x86_64/syscall.h.inc

90

For what its worth Microsofts STL uses a default vscode setting.

I am quite sure that you can set similar options for whatever IDE / editor you use.

libc/config/linux/x86_64/syscall.h.inc

-//===------------ inline implementation of x86_64 syscalls --------------*-===//
+//===------------ inline implementation of x86_64 syscalls ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 %%begin()
 
 #include "src/__support/common.h"
 
 #define SYSCALL_CLOBBER_LIST "rcx", "r11", "memory"
 
 namespace __llvm_libc {
 
-inline long syscall(long __number) {
+__attribute__((always_inline)) inline long syscall(long __number) {
   long retcode;
   LIBC_INLINE_ASM("syscall" :
              "=a"(retcode) :
              "a"(__number) :
              SYSCALL_CLOBBER_LIST);
   return retcode;
 }
 
-inline long syscall(long __number, long __arg1) {
+__attribute__((always_inline)) inline long syscall(long __number, long __arg1) {
   long retcode;
   LIBC_INLINE_ASM("syscall" :
              "=a"(retcode) :
              "a"(__number), "D"(__arg1) :
              SYSCALL_CLOBBER_LIST);
   return retcode;
 }
 
-inline long syscall(long __number, long __arg1, long __arg2) {
+__attribute__((always_inline)) inline long syscall(
+    long __number, long __arg1, long __arg2) {
   long retcode;
   LIBC_INLINE_ASM("syscall" :
              "=a"(retcode) :
              "a"(__number), "D"(__arg1), "S"(__arg2) :
              SYSCALL_CLOBBER_LIST);
   return retcode;
 }
 
-inline long syscall(long __number, long __arg1, long __arg2, long __arg3) {
+__attribute__((always_inline)) inline long syscall(
+    long __number, long __arg1, long __arg2, long __arg3) {
   long retcode;
   LIBC_INLINE_ASM("syscall" :
              "=a"(retcode) :
              "a"(__number), "D"(__arg1), "S"(__arg2), "d"(__arg3) :
              SYSCALL_CLOBBER_LIST);
   return retcode;
 }
 
-inline long syscall(
+__attribute__((always_inline)) inline long syscall(
     long __number, long __arg1, long __arg2, long __arg3, long __arg4) {
   long retcode;
   register long r10 __asm__("r10") = __arg4;
   LIBC_INLINE_ASM("syscall" :
              "=a"(retcode) :
              "a"(__number), "D"(__arg1), "S"(__arg2), "d"(__arg3), "r"(r10) :
              SYSCALL_CLOBBER_LIST);
   return retcode;
 }
 
-inline long syscall(long __number, long __arg1, long __arg2, long __arg3,
-                    long __arg4, long __arg5) {
+__attribute__((always_inline)) inline long syscall(
+    long __number, long __arg1, long __arg2, long __arg3, long __arg4,
+    long __arg5) {
   long retcode;
   register long r10 __asm__("r10") = __arg4;
   register long r8 __asm__("r8") = __arg5;
   LIBC_INLINE_ASM(
       "syscall" :
       "=a"(retcode) :
       "a"(__number), "D"(__arg1), "S"(__arg2), "d"(__arg3), "r"(r10), "r"(r8) :
       SYSCALL_CLOBBER_LIST);
   return retcode;
 }
 
-inline long syscall(long __number, long __arg1, long __arg2, long __arg3,
-                    long __arg4, long __arg5, long __arg6) {
+__attribute__((always_inline)) inline long syscall(
+    long __number, long __arg1, long __arg2, long __arg3, long __arg4,
+    long __arg5, long __arg6) {
   long retcode;
   register long r10 __asm__("r10") = __arg4;
   register long r8 __asm__("r8") = __arg5;
   register long r9 __asm__("r9") = __arg6;
   LIBC_INLINE_ASM(
       "syscall" :
       "=a"(retcode) :
       "a"(__number), "D"(__arg1), "S"(__arg2), "d"(__arg3), "r"(r10), "r"(r8), "r"(r9) :
       SYSCALL_CLOBBER_LIST);
   return retcode;
 }
 
 #undef SYSCALL_CLOBBER_LIST
 
 } // namespace __llvm_libc