Jump to conversation
Unresolved conversations (0)
Nice work!

Nice work!

All of your conversations have been resolved.

Resolved conversations (15)
@nickdesaulniers nickdesaulniers Mar 21, 2024
Orthogonal to the other issues we're hitting, this statement produces the following diagnostics: ``` [5/7] Building CXX object projects/libc/test/src/sys/epoll...inux.epoll_pwait_test.__build__.dir/epoll_pwait_test.cpp.o /android0/llvm-project/libc/test/src/sys/epoll/linux/epoll_pwait_test.cpp:33:50: warning: designated initializers are a C++20 extension [-Wc++20-designator] 33 | epoll_event event{.events = EPOLLOUT, .data = {.fd = pipefd[0]}}; | ^ /android0/llvm-project/libc/test/src/sys/epoll/linux/epoll_pwait_test.cpp:33:21: warning: designated initializers are a C++20 extension [-Wc++20-designator] 33 | epoll_event event{.events = EPOLLOUT, .data = {.fd = pipefd[0]}}; | ^ ``` as much as I love designated initializers, and am glad they were added to C++, they only did so for C++20. Consider building this test in `-std=c++20`.
Outdated
.../src/sys/epoll/linux/epoll_pwait_test.cpp
michaelrj-google
@nickdesaulniers nickdesaulniers Mar 21, 2024
Should this be using `struct epoll_event *` for `events`, rather than `epoll_event*` for `events` param? Then we don't even need any forward declarations. TIL that in C++ not having the `struct`/(or probably `class`) keyword(s) doesn't allow you to have an incomplete or opaque type. Example: https://godbolt.org/z/nfE8K7Whd There's other cases of this above, like in libc/src/sys/epoll/epoll_ctl.h.
Outdated
libc/src/sys/epoll/epoll_pwait.h
@nickdesaulniers nickdesaulniers Mar 13, 2024
I think this needs to be adjusted to "struct_epoll_data.h", based on the build failures. https://buildkite.com/llvm-project/github-pull-requests/builds/46591#018e348b-db56-4d69-a0a2-ca7d6daf9f76
Outdated
...lude/llvm-libc-types/struct_epoll_event.h
@nickdesaulniers nickdesaulniers Mar 13, 2024
ah, this still needs to be removed
Outdated
libc/src/sys/epoll/epoll_ctl.h
@nickdesaulniers nickdesaulniers Mar 12, 2024
What is libc/config/linux/syscall_numbers.h.inc (which also looks like it defines SYS_epoll_create and SYS_epoll_create1)? I wonder if we should be using that, too, for the same reasons for not using `<sys/epoll.h>` listed above? Looks like that generates a sys/syscall.h, but I'm guessing that't only for fullbuild mode and not overlay mode? (I may revisit this later for resolving the remaining lint warnings; it's not something that needs to be resolved in this PR. Just curiosity on my part).
libc/src/sys/epoll/linux/epoll_create.cpp
michaelrj-google
@nickdesaulniers nickdesaulniers Mar 11, 2024
Why do you pull in this system header, for this test and the ones below?
Outdated
...rc/sys/epoll/linux/epoll_create1_test.cpp
nickdesaulniers michaelrj-google
@nickdesaulniers nickdesaulniers Mar 11, 2024
https://android.googlesource.com/platform/bionic/+/refs/heads/main/tests/sys_epoll_test.cpp has a test that input `0` fails. Consider adding a test for that.
...src/sys/epoll/linux/epoll_create_test.cpp
@nickdesaulniers nickdesaulniers Mar 11, 2024
Maybe https://android.googlesource.com/platform/bionic/+/refs/heads/main/tests/sys_epoll_test.cpp has some inspiration, like testing the CLOEXEC bit.
...rc/sys/epoll/linux/epoll_create1_test.cpp
michaelrj-google
@nickdesaulniers nickdesaulniers Mar 11, 2024
again, if pipe2 is newer, than shouldn't we be checking for then using that first? man page says linux 2.6.27 (ancient)
libc/src/unistd/linux/pipe.cpp
@nickdesaulniers nickdesaulniers Mar 11, 2024
Should we be checking for `SYS_epoll_create1` first, then falling back to `SYS_epoll_create`? I'd imaging that the systems that have `SYS_epoll_create1` also have `SYS_epoll_create`, but not the other way around. Then again, the man page says these are from Linux 2.6, which is ancient pre-history. We don't need to support systems that old, so perhaps it's worthwhile to do: ``` #if !defined(SYS_epoll_create) || !defined(SYS_epoll_create1) #error "upgrade your old sh!t" #endif int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_epoll_create1, 0); ```
libc/src/sys/epoll/linux/epoll_create.cpp
michaelrj-google
@nickdesaulniers nickdesaulniers Mar 11, 2024
add `[[gnu::maybe_used]]` on the parameter. `(void) size;` only works for clang but not yet for GCC.
Outdated
libc/src/sys/epoll/linux/epoll_create.cpp
@nickdesaulniers nickdesaulniers Mar 11, 2024
What's the issue preventing this?
Outdated
libc/src/sys/epoll/epoll_ctl.h
nickdesaulniers michaelrj-google
@nickdesaulniers nickdesaulniers Mar 11, 2024
Do we not already have support for array params?
libc/spec/posix.td
michaelrj-google
@nickdesaulniers nickdesaulniers Mar 11, 2024
`<linux/eventpoll.h>`?
Outdated
...llvm-libc-macros/linux/sys-epoll-macros.h
nickdesaulniers michaelrj-google
@lntue lntue Mar 9, 2024
Use relative path here.
Outdated
...llvm-libc-macros/linux/sys-epoll-macros.h