Jump to conversation
Unresolved conversations (6)
@michaelrj-google michaelrj-google Apr 23, 2024
The way we interact with public headers changed since this patch was written. The short version is we're now using proxy headers in `/hdr/` instead of including the types from `include` directly. You can see an example of this in the patch that adds `pipe`: https://github.com/llvm/llvm-project/pull/84587/files#diff-0d107f2b3067e045736b456ccc6d3523aedade072afc031562b9103cb1972805 Sorry about the delay and the changing design, but this should be the final change needed before landing.
libc/test/src/unistd/pipe2_test.cpp
@muffpy muffpy Mar 20, 2024
`git clang-format` does not like this. Please ignore for now as this file may not even be required.
...vm-libc-macros/linux/watch-queue-macros.h
jhuber6
@nickdesaulniers nickdesaulniers Mar 19, 2024
Ah, this is not something we should be providing; it's ok to `#include <linux/watch_queue.h>` for the definition of `O_NOTIFICATION_PIPE`. I think that include should go in libc/include/llvm-libc-macros/unistd-macros.h. libc/include/llvm-libc-macros/unistd-macros.h should then be included in libc/test/src/unistd/pipe2_test.cpp.
...vm-libc-macros/linux/watch-queue-macros.h
nickdesaulniers muffpy
@nickdesaulniers nickdesaulniers Mar 19, 2024
Please add a comment `// TODO: use int array for the first ArgSpec when possible`.
Outdated
libc/spec/linux.td
@nickdesaulniers nickdesaulniers Mar 18, 2024
In terms of flags we define, it looks like we only define `O_CLOEXEC` and `O_NONBLOCK`. You should add definitions of `O_DIRECT` and `O_NOTIFICATION_PIPE` to include/llvm-libc-macros/linux/fcntl-macros.h.
libc/test/src/unistd/pipe2_test.cpp
muffpy
@nickdesaulniers nickdesaulniers Mar 18, 2024
Please include include/llvm-libc-macros/linux/fcntl-macros.h instead.
libc/test/src/unistd/pipe2_test.cpp
muffpy nickdesaulniers
Resolved conversations (12)
@michaelrj-google michaelrj-google Mar 19, 2024
nit: formatting
Outdated
libc/test/src/unistd/pipe2_test.cpp
muffpy
@michaelrj-google michaelrj-google Mar 19, 2024
nit: formatting
Outdated
libc/src/unistd/pipe2.h
@michaelrj-google michaelrj-google Mar 19, 2024
nit: formatting
Outdated
libc/src/unistd/linux/pipe2.cpp
@michaelrj-google michaelrj-google Mar 19, 2024
nit: formatting
Outdated
...vm-libc-macros/linux/watch-queue-macros.h
@nickdesaulniers nickdesaulniers Mar 18, 2024
The linter is not happy with this change. You can do `git clang-format HEAD~<N>` to format `<N>` commits. `<N>` is optional when `N == 1`. For example, `git clang-format HEAD~5` would format that past 5 commits.
Outdated
libc/test/src/unistd/pipe2_test.cpp
@michaelrj-google michaelrj-google Mar 18, 2024
you can't use `SYS_pipe` for `SYS_pipe2` since it doesn't respect the `flags` argument. We only define one syscall in terms of another when it can completely mimic the behavior.
Outdated
libc/src/unistd/linux/pipe2.cpp
@lntue lntue Mar 18, 2024
Missing new line at the end of the file?
Outdated
libc/test/src/unistd/CMakeLists.txt
@nickdesaulniers nickdesaulniers Mar 18, 2024
Note that this is currently failing in presubmit. ``` /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-6mhbj-1/llvm-project/github-pull-requests/libc/test/src/unistd/pipe2_test.cpp:54: FAILURE Failed to match LIBC_NAMESPACE::pipe2(pipefd, invalidflags) against Fails(EINVAL). Expected return value to be equal to -1 but got -22. ``` This is because upon failure, the kernel will return `-EINVAL` to userspace, so your existing logic is returning that, too. Upon failure, we should set the return value of `pipe2` to `-1`.
libc/test/src/unistd/pipe2_test.cpp
@nickdesaulniers nickdesaulniers Mar 18, 2024
This include should be unnecessary.
Outdated
libc/src/unistd/pipe2.h
@nickdesaulniers nickdesaulniers Mar 18, 2024
```suggestion if (ret >= 0) return ret; libc_errno = -ret; return -1; ``` (updated)
Outdated
libc/src/unistd/linux/pipe2.cpp
muffpy michaelrj-google
@nickdesaulniers nickdesaulniers Mar 18, 2024
Put these 3 includes together with no extra new lines, then sort.
Outdated
libc/src/unistd/linux/pipe2.cpp
@nickdesaulniers nickdesaulniers Mar 18, 2024
@michaelrj-google can you check these types? This is slightly different than how you chose to define an `int [2]` in https://github.com/llvm/llvm-project/pull/84587/files.
Outdated
libc/spec/linux.td
muffpy nickdesaulniers
michaelrj-google