[libc][stdlib] Remove LIBC_THREAD_LOCAL from rand_next

[PiJoules]

srand and rand do not need to be threadsafe, so it should be permissible to remove thise from rand_next whose only users are srand and rand. This is helpful for the baremetal runtimes where we still need to setup the TLS machinery.

[llvmbot]

@llvm/pr-subscribers-libc

Author: None (PiJoules)

Changes

srand and rand do not need to be threadsafe, so it should be permissible to remove thise from rand_next whose only users are srand and rand. This is helpful for the baremetal runtimes where we still need to setup the TLS machinery.

---

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

2 Files Affected:

• (modified) libc/src/stdlib/rand_util.cpp (+1-1)
• (modified) libc/src/stdlib/rand_util.h (+1-1)

diff --git a/libc/src/stdlib/rand_util.cpp b/libc/src/stdlib/rand_util.cpp
index 1f3dbce9c7860..4aa03bfe3eba9 100644
--- a/libc/src/stdlib/rand_util.cpp
+++ b/libc/src/stdlib/rand_util.cpp
@@ -18,7 +18,7 @@ ThreadLocal<unsigned long> rand_next;
 #else
 // C standard 7.10p2: If 'rand' is called before 'srand' it is to proceed as if
 // the 'srand' function was called with a value of '1'.
-LIBC_THREAD_LOCAL unsigned long rand_next = 1;
+unsigned long rand_next = 1;
 #endif
 
 } // namespace LIBC_NAMESPACE
diff --git a/libc/src/stdlib/rand_util.h b/libc/src/stdlib/rand_util.h
index cadd6b5cdcbb8..b32302f486dce 100644
--- a/libc/src/stdlib/rand_util.h
+++ b/libc/src/stdlib/rand_util.h
@@ -34,7 +34,7 @@ template <typename T> class ThreadLocal {
 
 extern ThreadLocal<unsigned long> rand_next;
 #else
-extern LIBC_THREAD_LOCAL unsigned long rand_next;
+extern unsigned long rand_next;
 #endif
 
 } // namespace LIBC_NAMESPACE

[nickdesaulniers]

Sounds like you can change this for GPU, too?

[jhuber6]

You know, looking into this it's baffling that POSIX made rand_r to be reentrant but made it take an unsigned *r instead of some opaque type defined by the implementation, so it's effectively unusable.

For the GPU, it's unlikely people would be able to use this seriously if it's not thread safe, but the thread local hack isn't really serviceable either. It's prefer a functional reentrant version above anything else. I thought LIBC_THREAD_LOCAL was just empty on platforms that didn't support it (baremetal) so this is a no-op?

[nickdesaulniers]

I thought LIBC_THREAD_LOCAL was just empty on platforms that didn't support it (baremetal) so this is a no-op?

This code is also the only user of ThreadLocal, so we could probably delete src/stdlib/rand_util.h if we remove ThreadLocal.

[jhuber6]

I thought LIBC_THREAD_LOCAL was just empty on platforms that didn't support it (baremetal) so this is a no-op?

This code is also the only user of ThreadLocal, so we could probably delete src/stdlib/rand_util.h if we remove ThreadLocal.

I thought errno used it?

[michaelrj-google]

thread local is used in a couple places: errno, strerror, and strsignal

[nickdesaulniers]

I'm talking about the class ThreadLocal we have in src/stdlib/rand_util.h, of which rand is the only user. I think that can get deleted, too, in this PR.

[SchrodingerZhu]

Thread-safety

All interfaces defined by this specification will be thread-safe, except that the following interfaces need not be thread-safe:
POSIX Interfaces

asctime()
ctime()
getc_unlocked()
getchar_unlocked()
getgrgid()
getgrnam()
getlogin()
getopt()
getpwnam()
getpwuid()
gmtime()
localtime()
putc_unlocked()
putchar_unlocked()
rand()
readdir()
strtok()
ttyname()

X/Open Interfaces

basename()
catgets()
dbm_clearerr()
dbm_close()
dbm_delete()
dbm_error()
dbm_fetch()
dbm_firstkey()
dbm_nextkey()
dbm_open()
dbm_store()
dirname()
drand48()
ecvt()
encrypt()
endgrent()
endpwent()
endutxent()
fcvt()
gamma()
gcvt()
getdate()
getenv()
getgrent()
getpwent()
getutxent()
getutxid()
getutxline()
getw()
l64a()
lgamma()
lrand48()
mrand48()
nl_langinfo()
ptsname()
putenv()
pututxline()
setgrent()
setkey()
setpwent()
setutxent()
strerror()

The interfaces ctermid() and tmpnam() need not be thread-safe if passed a NULL argument.

The interfaces in the Legacy Feature Group need not be thread-safe.

Implementations will provide internal synchronisation as necessary in order to satisfy this requirement.

[SchrodingerZhu]

I am curious about what MT-Safe means exactly here.

For example, this function https://android.googlesource.com/platform/bionic/+/android-4.2.2_r1/libc/bionic/tdestroy.c is clearly not to be called concurrently on the same tree, while POSIX and this manpage claims it is MT-safe. I assume it is safe in the sense that it can be invoked concurrently on different trees in the same time, but there is no clarification on this.

[PiJoules]

I'm talking about the class ThreadLocal we have in src/stdlib/rand_util.h, of which rand is the only user. I think that can get deleted, too, in this PR.

Done

[jhuber6]

Looking at some other implementations, it seems like ISO C states that the random seed is per-process, but that multiple threads should be not able to interfere with it? I was looking at glibc and this seems to be the case https://github.com/bminor/glibc/blob/master/stdlib/random.c#L287.

So, this to me seems like we'd need to implement some kind of RMW atomic on the underlying state, not just get rid of the thread_local.

Honestly, I might try to write it this way since it would solve some issues on the GPU and it's easier than implementing THREAD_LOCAL. After that the only thread local thing is errno, but I'm just going to state that errno doesn't exist on the GPU.

[nickdesaulniers]

Looking at some other implementations, it seems like ISO C states that the random seed is per-process, but that multiple threads should be not able to interfere with it? I was looking at glibc and this seems to be the case https://github.com/bminor/glibc/blob/master/stdlib/random.c#L287.

So, this to me seems like we'd need to implement some kind of RMW atomic on the underlying state, not just get rid of the thread_local.

FWIW, Bionic uses NetBSD's srandom, which uses a mutex_t to guard access to the random seed.
https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/upstream-netbsd/common/lib/libc/stdlib/random.c#304

[jhuber6]

I feel like updating this with a CAS loop is a good middle-ground #96692. This also lets the GPU implementation actually work, since previously I couldn't statically initialize it to 1. Though, it's not like CAS loops are cheap, but I didn't want to use actual locks since they're a PITA to make work on the GPU (see the RPC implementation).

[frobtech]

I feel like updating this with a CAS loop is a good middle-ground #96692. This also lets the GPU implementation actually work, since previously I couldn't statically initialize it to 1. Though, it's not like CAS loops are cheap, but I didn't want to use actual locks since they're a PITA to make work on the GPU (see the RPC implementation).

I concur that the single atomic is the best approach for this case.

[jhuber6]

Thanks for bringing this to attention, I think we ended up with a good solution.