[libc] Mutex implementation for single-threaded baremetal

[saturn691]

Part of #145349. Required to allow atexit to work. As part of HermeticTestUtils.cpp, there is a reference to atexit(), which eventually instantiates an instance of a Mutex.

Instead of copying the implementation from libc/src/__support/threads/gpu/mutex.h, we allow platforms to select an implementation based on configurations, allowing the GPU and single-threaded baremetal platforms to share an implementation. This can be configured or overridden.

Later, when the threading API is more complete, we can add an option to support multithreading (or set it as the default), but having single-threading (in tandem) is in line with other libraries for embedded devices.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-libc

Author: William Huynh (saturn691)

Changes

In order to add, #145349, we need a Mutex, to allow atexit to work.

---

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

3 Files Affected:

• (added) libc/src/__support/threads/baremetal/CMakeLists.txt (+5)
• (added) libc/src/__support/threads/baremetal/mutex.h (+32)
• (modified) libc/src/__support/threads/mutex.h (+2)

diff --git a/libc/src/__support/threads/baremetal/CMakeLists.txt b/libc/src/__support/threads/baremetal/CMakeLists.txt
new file mode 100644
index 0000000000000..ea89feb0c5c68
--- /dev/null
+++ b/libc/src/__support/threads/baremetal/CMakeLists.txt
@@ -0,0 +1,5 @@
+add_header_library(
+  mutex
+  HDRS
+    mutex.h
+)
diff --git a/libc/src/__support/threads/baremetal/mutex.h b/libc/src/__support/threads/baremetal/mutex.h
new file mode 100644
index 0000000000000..77a0b61ea9f5b
--- /dev/null
+++ b/libc/src/__support/threads/baremetal/mutex.h
@@ -0,0 +1,32 @@
+//===--- Implementation of a mutex class for baremetal ----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC___SUPPORT_THREADS_BAREMETAL_MUTEX_H
+#define LLVM_LIBC_SRC___SUPPORT_THREADS_BAREMETAL_MUTEX_H
+
+#include "src/__support/macros/attributes.h"
+#include "src/__support/macros/config.h"
+#include "src/__support/threads/mutex_common.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+/// Implementation of a simple passthrough mutex which guards nothing. For
+/// single threaded processors, this is the implementation.
+struct Mutex {
+  LIBC_INLINE constexpr Mutex(bool, bool, bool, bool) {}
+
+  LIBC_INLINE MutexError lock() { return MutexError::NONE; }
+  LIBC_INLINE MutexError unlock() { return MutexError::NONE; }
+  LIBC_INLINE MutexError reset() { return MutexError::NONE; }
+};
+
+// TODO: add multithreading support here
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC___SUPPORT_THREADS_BAREMETAL_MUTEX_H
diff --git a/libc/src/__support/threads/mutex.h b/libc/src/__support/threads/mutex.h
index 392b38984dc0a..a600e2ff017f5 100644
--- a/libc/src/__support/threads/mutex.h
+++ b/libc/src/__support/threads/mutex.h
@@ -41,6 +41,8 @@
 #include "src/__support/threads/linux/mutex.h"
 #elif defined(LIBC_TARGET_ARCH_IS_GPU)
 #include "src/__support/threads/gpu/mutex.h"
+#elif defined(__ELF__)
+#include "src/__support/threads/baremetal/mutex.h"
 #endif // __linux__
 
 #endif // LLVM_LIBC_SRC___SUPPORT_THREADS_MUTEX_H

[jhuber6]

I've been considering if it's possible to just make atexit lock free. it would be pretty easy if we defined what happens when one thread calls atexit() while someone is consuming it. Easy mode is that we ignore any attempt to modify after exit is already happening. Hard mode is allowing it to interject and calling it as well.

[mysterymath]

I've been considering if it's possible to just make atexit lock free. it would be pretty easy if we defined what happens when one thread calls atexit() while someone is consuming it. Easy mode is that we ignore any attempt to modify after exit is already happening. Hard mode is allowing it to interject and calling it as well.

That would conflict with the C standard, right?

[jhuber6]

I've been considering if it's possible to just make atexit lock free. it would be pretty easy if we defined what happens when one thread calls atexit() while someone is consuming it. Easy mode is that we ignore any attempt to modify after exit is already happening. Hard mode is allowing it to interject and calling it as well.

That would conflict with the C standard, right?

7.24.4.2.2
The atexit function registers the function pointed to by func, to be called without arguments at
normal program termination.363) It is unspecified whether a call to the atexit function that does
not happen before the exit function is called will succeed. 

I think glibc does a stack that allows this. Possible we'd want to be compatible with that. We currently aren't, because we just lock the interface... Doesn't that mean that if an atexit handler calls atexit it'll deadlock?

[mysterymath]

7.24.4.2.2
The atexit function registers the function pointed to by func, to be called without arguments at
normal program termination.363) It is unspecified whether a call to the atexit function that does
not happen before the exit function is called will succeed. 

Whoa, I'm out of date! That's interesting; C11 doesn't have the second sentence, which would ostensibly put it in scope for 7.1.4 (in the draft): Implementations may share their own internal objects between threads if the objects are not visible to users and are protected against data races.

Do we have a go-to policy for cases in libc/libc++ where the standard loosens its requirements from version to version? (Presuming that I'm interpreting things reasonably and that happened here; I haven't looked into this very far. Need to find the rationale doc for that change.)

[mysterymath]

We currently aren't, because we just lock the interface... Doesn't that mean that if an atexit handler calls atexit it'll deadlock?

It shouldn't; the implementation explicitly unlocks the stack before calling a handler:

llvm-project/libc/src/stdlib/exit_handler.h

Line 53 in 0f173a0

handler_list_mtx.unlock();

[petrhosek]

The main concern I have with this change is that it furthers the notion of baremetal as a singular platform whereas in practice there is a diverse set of baremetal platforms. For that reason, I'm starting to consider the existence of baremetal implementation subdirectories in LLVM libc to be an antipattern and I think we should aim to eliminate these directories and baremetal should be just a build configuration.

Rather than introducing threads/baremetal/mutex.h, I think we should introduce a threading mode configuration option akin to what we've done for errno. That is, we would introduce LIBC_CONF_THREAD_MODE with values such as:

• LIBC_THREAD_MODE_SINGLE: this would be the implementation in this PR,
• LIBC_THREAD_MODE_EXTERNAL: we could make it a no-op or #error for now,
• LIBC_THREAD_MODE_PLATFORM: this would be the platform specific implementation for platforms such as Linux.

For baremetal build configuration we would default to LIBC_THREAD_MODE_SINGLE.

I don't expect this to be significantly more complicated than the current PR, but it would more flexible and there might be other build configurations beyond baremetal where LIBC_THREAD_MODE_SINGLE might be useful.

[saturn691]

1. Extremely sorry for the amount of force-pushes, I don't have access to some platforms on my host machine and am using the CI to test that I haven't broken anything (I have tested the baremetal configuration downstream).
2. Totally agree with your approach, I think it makes a lot of sense here, where we can share the implementation with the GPU, however, I still think in other places it makes a bit more sense, e.g. src/time/baremetal. I think overusing this build configuration will make the codebase harder to read, compared to the "switch-case" like nature of keeping the baremetal folder.

On further thought, I wish to share some of the logic for errno and mutexes, in the sense that if I choose single threading, I want to get the single-threaded errno implementation (currently I use the external interface and it gets messy). We can discuss this another time, as this is outside the scope of this PR.

[saturn691]

ping

[saturn691]

Implemented changes as suggested from @petrhosek.
As a side effect, it may change the GPU build @jhuber6. However, I'm pretty sure these implementations can be safely shared.

cc: @michaelrj-google, @frobtech this PR has been here some time, how can I progress this issue further? Is anything lacking, if not who should review this? Thanks a lot!

[petrhosek]

This is error-prone, every time libc is ported to a new OS, we would need to update this list otherwise the baremetal build is going to break ("action at a distance"). I think a better approach would be to pass this value from the build system. I think it'd be also cleaner to introduce this in a separate PR since it's technically unrelated.

[saturn691]

Ok, I'll submit another PR to fix this, we can discuss that over there

[saturn691]

Do you think it would be better to share the configuration of LIBC_THREAD_MODE with LIBC_ERRNO_MODE?

1. LIBC_ERRNO_MODE_SHARED (single thread), LIBC_ERRNO_MODE_SYSTEM (platform) and LIBC_ERRNO_MODE_EXTERNAL (external) already exist.
2. Ideally I want the LIBC_THREAD_LOCAL to be controlled by this build variable. Unfortunately libcxx includes headers directly through shared/* but then we can just define it in errno.h and include that in shared/libc_common.h like so: #include <errno.h>
3. I can't think of another mode to LIBC_THREAD_MODE that isn't compatible with LIBC_ERRNO_MODE, so we can just stick with LIBC_ERRNO_MODE (which deduces the Mutex type).

If you're happy with that I can redo this PR in one go.

[petrhosek]

Do you think it would be better to share the configuration of LIBC_THREAD_MODE with LIBC_ERRNO_MODE?

1. LIBC_ERRNO_MODE_SHARED (single thread), LIBC_ERRNO_MODE_SYSTEM (platform) and LIBC_ERRNO_MODE_EXTERNAL (external) already exist.
2. Ideally I want the LIBC_THREAD_LOCAL to be controlled by this build variable. Unfortunately libcxx includes headers directly through shared/* but then we can just define it in errno.h and include that in shared/libc_common.h like so: #include <errno.h>
3. I can't think of another mode to LIBC_THREAD_MODE that isn't compatible with LIBC_ERRNO_MODE, so we can just stick with LIBC_ERRNO_MODE (which deduces the Mutex type).

If you're happy with that I can redo this PR in one go.

I'd keep them separate since they're technically orthogonal and I think we should avoid overloading the meaning of LIBC_ERRNO_MODE. LLVM libc is meant to be usable "a la carte" and we want to give users flexibility to configure the implementation as they fit.

I expect we'll redo the Mutex interface in the future since the current design is quite limited and at that point we may revisit LIBC_THREAD_MODE and potentially remove/replace it, but I'm happy with this change as an incremental step.