Skip to content

[libc++] Sync the filesystem clock_gettime handling with chrono for OpenBSD #92675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

brad0
Copy link
Contributor

@brad0 brad0 commented May 19, 2024

Copy the same OS checks for clock_gettime from chrono to filesystem and
tweak the comment as Hurd is in the same situation.

…penBSD

Copy the same OS checks for clock_gettime from chrono to filesystem and
tweak the comment as Hurd is in the same situation.
@brad0 brad0 requested a review from a team as a code owner May 19, 2024 02:09
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 19, 2024
@llvmbot
Copy link
Member

llvmbot commented May 19, 2024

@llvm/pr-subscribers-libcxx

Author: Brad Smith (brad0)

Changes

Copy the same OS checks for clock_gettime from chrono to filesystem and
tweak the comment as Hurd is in the same situation.


Full diff: https://github.com/llvm/llvm-project/pull/92675.diff

2 Files Affected:

  • (modified) libcxx/src/chrono.cpp (+2-2)
  • (modified) libcxx/src/filesystem/filesystem_clock.cpp (+3-1)
diff --git a/libcxx/src/chrono.cpp b/libcxx/src/chrono.cpp
index e7d6dfbc22924..e8ebb67284bbb 100644
--- a/libcxx/src/chrono.cpp
+++ b/libcxx/src/chrono.cpp
@@ -31,8 +31,8 @@
 #  include <sys/time.h> // for gettimeofday and timeval
 #endif
 
-// OpenBSD does not have a fully conformant suite of POSIX timers, but
-// it does have clock_gettime and CLOCK_MONOTONIC which is all we need.
+// GNU Hurd and OpenBSD do not implement a fully conformant suite of POSIX timers, but
+// they do have clock_gettime and CLOCK_MONOTONIC which are all we need.
 #if defined(__APPLE__) || defined(__gnu_hurd__) || defined(__OpenBSD__) || (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0)
 #  define _LIBCPP_HAS_CLOCK_GETTIME
 #endif
diff --git a/libcxx/src/filesystem/filesystem_clock.cpp b/libcxx/src/filesystem/filesystem_clock.cpp
index e13b2853e367c..a120be14978a3 100644
--- a/libcxx/src/filesystem/filesystem_clock.cpp
+++ b/libcxx/src/filesystem/filesystem_clock.cpp
@@ -29,7 +29,9 @@
 #  include <sys/time.h> // for gettimeofday and timeval
 #endif
 
-#if defined(__APPLE__) || defined(__gnu_hurd__) || (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0)
+// GNU Hurd and OpenBSD do not implement a fully conformant suite of POSIX timers, but
+// they do have clock_gettime and CLOCK_MONOTONIC which are all we need.
+#if defined(__APPLE__) || defined(__gnu_hurd__) || defined(__OpenBSD__) || (defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0)
 #  define _LIBCPP_HAS_CLOCK_GETTIME
 #endif
 

@brad0 brad0 requested a review from Ericson2314 May 19, 2024 02:25
@mordante
Copy link
Member

In general we like to have a CI runner to test platform specific behaviour. Do you intent to add a GNU Hurd runner? (This change is small and innocent, but without CI tests there is no guarantee there won't be regressions.)

Can you make the commit message a bit more descriptive. Something along the lines of
[libc++] Add GNU Hurd clock_gettime support seems better describing the goal of the change.

@ldionne
Copy link
Member

ldionne commented Jun 25, 2024

Gentle ping -- I'll close this as inactive if there is no plan to officially support this platform.

@brad0
Copy link
Contributor Author

brad0 commented Jun 27, 2024

Gentle ping -- I'll close this as inactive if there is no plan to officially support this platform.

I'd like to have official support for the platform. But looking at the current code and support policy this will consistently be an issue for us. Our current Clang / libc++ is 16.

@Ericson2314
Copy link
Member

@brad0 I'll need to forward-port some patches just to get the bare minimum of simple executables able to run. Maybe that is good enough even if OpenBSD is still officially hanging back on LLVM 16 for a while longer?

OpenBSD is a faster-moving ABI with the security experiments (and I would not want that to change!), but I think it should be possible to have less outsanding patches against upstream without compromising that ABI agility or incurring painful upstreaming drudgery.

@ldionne
Copy link
Member

ldionne commented Jun 27, 2024

What kind of downstream changes do you folks have? We may be able to accommodate some of them or at least make them easier to maintain by introducing a few "extension points" in the library. This is a common situation and we're quite used to finding ways to make it work both for the upstream project and for downstream needs.

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 27, 2024

@ldionne I am learning as I go, as my cross compiled binaries do not yet work :).

First on my docket is that LLD needs some linker script modifications so that special sections like .openbsd.mutable and .openbsd.syscalls end of being linked correctly.

The change that currently does this just uses #ifdef __OpenBSD__ but that is not right for cross compilation, so this will need to be fixed up.

To be clear, I haven't seen any critical downstream modification in libc++ itself yet, but I would consider LLD / Clang / LLVM changes blockers insofar is if binaries don't run, then the libc++ testsuite cannot run --- unless you would consider a "does it at least build" smoke test a decent first step.

@brad0
Copy link
Contributor Author

brad0 commented Jun 28, 2024

What kind of downstream changes do you folks have? We may be able to accommodate some of them or at least make them easier to maintain by introducing a few "extension points" in the library. This is a common situation and we're quite used to finding ways to make it work both for the upstream project and for downstream needs.

For libc++. Nothing.

Various local patches for Clang, LLVM and LLD though.

@ldionne
Copy link
Member

ldionne commented Jul 4, 2024

@Ericson2314 @brad0 I think that to begin with, we could start with only compiling the test suite. We could at least see how far we're able to go in that direction. I really want to avoid having "phantom" platforms in the code base (where we have 0 visibility into what's working and what's not working).

@ldionne
Copy link
Member

ldionne commented Jul 4, 2024

I can provide support for adding a new OpenBSD build bot if you want. The documentation for adding a new CI job using BuildKite is here: https://libcxx.llvm.org/AddingNewCIJobs.html#addingnewcijobs

It's also possible to set up a custom Github Actions runner if you prefer that, either way is fine by us.

@brad0
Copy link
Contributor Author

brad0 commented Jul 6, 2024

I can provide support for adding a new OpenBSD build bot if you want. The documentation for adding a new CI job using BuildKite is here: https://libcxx.llvm.org/AddingNewCIJobs.html#addingnewcijobs

There is not a Buildkite binary for OpenBSD. But it built Ok. I'll see about creating a package.

I'll be able to try setting something up around the end of the month.

@ldionne
Copy link
Member

ldionne commented Jul 9, 2024

Ack. Please let me know when/whether I can provide support. I will set up a reminder to ping this at the end of the month.

I would aim to have this done in time for the LLVM 20 release. In terms of process, if we are not able to set this up in time for LLVM 20, I would remove patches that intend to provide support for OpenBSD until we are able to actually provide support for it, to avoid an accumulation of half-supported things in the codebase.

@ldionne
Copy link
Member

ldionne commented Jul 9, 2024

I'll close this for the time being but let's reopen it once we have what we need CI-wise.

@ldionne ldionne closed this Jul 9, 2024
@ldionne
Copy link
Member

ldionne commented Jul 31, 2024

Scheduled ping. Any interest for setting up CI? Should I start cleaning up OpenBSD support from the upstream tree?

@brad0
Copy link
Contributor Author

brad0 commented Aug 1, 2024

Scheduled ping. Any interest for setting up CI? Should I start cleaning up OpenBSD support from the upstream tree?

I have the system. I just got the OS installed and it's ready to go.

@ldionne
Copy link
Member

ldionne commented Aug 1, 2024

Can you reach out to me on Discord @ldionne, I can help you set up the machine w/ BuildKite tokens and so on.

@brad0
Copy link
Contributor Author

brad0 commented Aug 24, 2024

Can you reach out to me on Discord @ldionne, I can help you set up the machine w/ BuildKite tokens and so on.

@ldionne I have. You don't respond.

@ldionne
Copy link
Member

ldionne commented Aug 26, 2024

Sorry, new message requests on Discord end up in a weird place, I missed it but I see it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants