This is an archive of the discontinued LLVM Phabricator instance.

Implement `getrandom` function for linux targets.
ClosedPublic

Authored by SchrodingerZhu on Sep 26 2022, 12:35 PM.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 26 2022, 12:35 PM
SchrodingerZhu requested review of this revision.Sep 26 2022, 12:35 PM

Overall this patch is almost there, it just needs a couple more mechanical things since it adds a new header. Specifically, you should add the header definition to spec/linux.td. You can look at how HeaderSpecs are defined in spec/stdc.td, I would recommend looking at the one for string.h as an example, or feel free to reach out if you need help figuring it out.

libc/include/CMakeLists.txt
237

you need to add this target to include/llvm-libc-macros/CMakeLists.txt (it should be the same as the others in there) as well as the cmake file in the linux subfolder.

libc/src/sys/CMakeLists.txt
8

nit: please fix

libc/src/sys/random/getrandom.h
2–3

nit: this should all be on one line

libc/src/sys/random/linux/CMakeLists.txt
13

nit: please fix

libc/src/sys/random/linux/getrandom.cpp
22

if there's only one possible syscall, we usually don't bother with checking if it's defined, since the compiler will do that for us.

libc/test/src/sys/random/linux/getrandom_test.cpp
48

nit: please fix

  • address code reviews:
    • add getrandom to linux specification
    • add sys/random.h to header target
    • random format bits

Thanks for the patch and working through all the details. Overall LGTM but needs one code policy related change.

libc/include/llvm-libc-macros/linux/sys-random-macros.h
16

An empty line between 15 and 16?

libc/test/src/sys/random/linux/getrandom_test.cpp
6

We don't include C++ headers from unit tests. Also, for functions like fabs, we use internal functions. So:

  1. Either add M_PI definition to math.h or use a constant for this test.
  2. Instead of ::fabs, use __llvm_libc::fabs.
  • address code reviews
    • add blank lines in headers
    • use internal math functions in test
    • let getrandom_test depend on lib.src.math.fabs
  • additional fix
    • fix wrong names in macro headers (rename STAT to RANDOM)
sivachandra accepted this revision.Sep 27 2022, 3:45 PM
This revision is now accepted and ready to land.Sep 27 2022, 3:45 PM

is this patch ready for merge?

@michaelrj would you please consider merging this or give more instructions on updating this?
I see that some syscall related changes may have already made this patch incompatible. I am worried that it will become even harder to keep track given the active development of libc.

Hi, if you can update after rebasing, I can submit the patch for you.

I've left two comments for things to fix, and once you fix those I'll approve the patch and one of us will land it.

libc/spec/linux.td
132–133

you need to add SysRandom to this list, preferably right after SysMMan.

libc/src/sys/random/linux/getrandom.cpp
22

this needs to be syscall_impl

  • rebase to main
  • address code reviews:
    • use syscall_impl instead
    • add SysRandom to linux spec file
michaelrj accepted this revision.Oct 10 2022, 10:04 AM

Approved with one last nit

libc/src/sys/CMakeLists.txt
8

nit: please fix

  • address code review: add blank line to CMakeLists.txt
This revision was automatically updated to reflect the committed changes.