This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by sivachandra on Jan 2 2020, 12:56 PM.

Details

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.

Event Timeline

sivachandra created this revision.Jan 2 2020, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 2 2020, 12:56 PM

Remove an unintentional change that slipped through.

abrachet accepted this revision.Jan 2 2020, 1:19 PM
abrachet added inline comments.
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.

This revision is now accepted and ready to land.Jan 2 2020, 1:19 PM
sivachandra marked 3 inline comments as done.

Fix format.

sivachandra added inline comments.Jan 6 2020, 10:59 AM
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 added inline comments.Jan 6 2020, 11:48 AM
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 accepted this revision.Jan 6 2020, 12:11 PM
MaskRay added inline comments.
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.

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

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

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.

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.

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.

This revision was automatically updated to reflect the committed changes.
miscco marked an inline comment as done.Jan 6 2020, 10:02 PM
miscco added a subscriber: 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.