-
Notifications
You must be signed in to change notification settings - Fork 12
An alternative TLS implementation to fix a crash on musl/aarch64 #600
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
base: main
Are you sure you want to change the base?
Changes from all commits
8d2b637
02614cf
9840b2f
a4aa192
17889ce
f38c9cd
b387308
c2b7014
a632c1c
a3d8950
792a4a3
f9c9b28
86c2e2e
2e788fd
282a346
5cf4a1c
337b80f
c190286
13c8891
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| /* | ||
| * Copyright 2026, Datadog, Inc. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| #ifndef _THREADLOCAL_H | ||
| #define _THREADLOCAL_H | ||
|
|
||
| #include <cassert> | ||
| #include <cstring> | ||
| #include <pthread.h> | ||
| #include "arch.h" | ||
|
|
||
| /** | ||
| * This file implements an alternative to C/C++ thread local. | ||
| * Due to some restrictions of the language implementations, especially, on musl/aarch64, | ||
| * they cannot be safely used in profiler. | ||
| * | ||
| * How to use? | ||
| * A ThreadLocal should be declared as a static variable, e.g. | ||
| * | ||
| * static void* create_my_object() { | ||
| * return new MyObject(); | ||
| * } | ||
| * | ||
| * static void delete_my_object(void* p) { | ||
| * MyObject* obj = (MyObject*)p; | ||
| * delete obj; | ||
| * } | ||
| * | ||
| * static ThreadLocal<MyObject*, create_my_object, delete_my_object> var; | ||
| * MyObject* value = var.get(); | ||
| * ... | ||
| * var.clear(); | ||
| * ... | ||
| * MyObject* new_value = new MyObject(); | ||
| * var.set(new_value); | ||
| * ... | ||
| * var.clear(); | ||
| * | ||
| */ | ||
|
|
||
| // The function to create value if it does not exist | ||
| typedef void* (*CREATE_FUNC)(void); | ||
| // Cleanup the value when deleting the key | ||
| typedef void (*CLEAN_FUNC)(void*); | ||
| template <typename T, CREATE_FUNC C = nullptr, CLEAN_FUNC F = nullptr> | ||
| class ThreadLocal { | ||
| protected: | ||
| pthread_key_t _key; | ||
|
|
||
| public: | ||
| ThreadLocal(const ThreadLocal&) = delete; | ||
| ThreadLocal& operator=(const ThreadLocal&) = delete; | ||
|
|
||
| ThreadLocal() { | ||
| int err = pthread_key_create(&_key, F); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH · robustness] Suggestion: replace the assert with an unconditional abort: if (err != 0) { fprintf(stderr, "ThreadLocal: pthread_key_create failed: %d\n", err); abort(); }This matches the existing error handling in thread.cpp and makes failures visible in production. |
||
| // What to do if we can not create a key? | ||
| assert(err == 0); | ||
|
Comment on lines
+57
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the process exhausts pthread TLS keys, Useful? React with 👍 / 👎. |
||
| } | ||
|
zhengyu123 marked this conversation as resolved.
|
||
|
|
||
| ~ThreadLocal() { | ||
| pthread_key_delete(_key); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH · robustness] Suggestion: track construction success with a _key_valid = (pthread_key_create(&_key, F) == 0);
// ...
if (_key_valid) pthread_key_delete(_key);Apply the same pattern to the |
||
| } | ||
|
|
||
| void set(T value) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM · correctness] The primary template casts Suggestion: add a static_assert(sizeof(T) == sizeof(void*),
"ThreadLocal<T> requires sizeof(T)==sizeof(void*); use a pointer type or add a specialization");
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM · correctness] Suggestion: document explicitly that
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM · completeness] Suggestion: in void set(T value) {
void* prev = pthread_getspecific(_key);
int err = pthread_setspecific(_key, (const void*)value);
assert(err == 0);
if (prev != nullptr && F != nullptr) F(prev);
}Alternatively, document that |
||
| int err = pthread_setspecific(_key, (const void*)value); | ||
| assert(err == 0); | ||
| } | ||
|
Copilot marked this conversation as resolved.
|
||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM · necessity] Suggestion: either remove |
||
| T get() { | ||
| void* p = pthread_getspecific(_key); | ||
| if (p == nullptr && C != nullptr) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW · robustness] Suggestion: document that |
||
| p = C(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH · robustness] After Suggestion: check p = C();
if (pthread_setspecific(_key, (const void*)p) != 0) {
if (F) F(p);
return T{};
}Alternatively, make |
||
| set((T)p); | ||
| } | ||
| return (T)p; | ||
| } | ||
|
|
||
| // Clear the value | ||
| void clear() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM · correctness] Suggestion: always zero the TSD slot and only call void clear() {
void* p = pthread_getspecific(_key);
if (p != nullptr) {
pthread_setspecific(_key, nullptr);
if (F != nullptr) F(p);
}
} |
||
| void* p = nullptr; | ||
| if (F != nullptr && (p = pthread_getspecific(_key)) != nullptr) { | ||
| int err = pthread_setspecific(_key, nullptr); | ||
| // Safety: if reset the value failed, get() can see staled value if | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW · consistency] The comment here says "get() can see staled value if it is freed", but in this code path the value is explicitly not freed ( Suggestion: // Safety: only free after the slot is cleared; if the slot reset failed,
// skip the free to avoid get() returning a dangling pointer. |
||
| // it is freed. | ||
| if (err == 0) { | ||
| F(p); | ||
| } | ||
| } | ||
| } | ||
|
Copilot marked this conversation as resolved.
|
||
| }; | ||
|
|
||
| template <> | ||
| class ThreadLocal<double> { | ||
| protected: | ||
| pthread_key_t _key; | ||
| public: | ||
| ThreadLocal(const ThreadLocal&) = delete; | ||
| ThreadLocal& operator=(const ThreadLocal&) = delete; | ||
|
|
||
| ThreadLocal() { | ||
| // Only support 64-bit platforms, double and void* are the same size | ||
| static_assert(sizeof(void*) == 8); | ||
| static_assert(sizeof(double) == 8); | ||
| int err = pthread_key_create(&_key, nullptr); | ||
| // What to do if we can not create a key? | ||
| assert(err == 0); | ||
| } | ||
|
Copilot marked this conversation as resolved.
|
||
|
|
||
| ~ThreadLocal() { | ||
| pthread_key_delete(_key); | ||
| } | ||
|
|
||
| // double <--> u64 cast, preserve bit format | ||
| // Can use std::bit_cast after upgrade C++ version to 20 | ||
| void set(double value) { | ||
| u64 val; | ||
| memcpy(&val, &value, sizeof(value)); | ||
| int err = pthread_setspecific(_key, (const void*)val); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW · consistency] Integer-to-pointer cast at line 120 uses a C-style // line 120:
int err = pthread_setspecific(_key, reinterpret_cast<const void*>(val));
// line 130:
u64 val = reinterpret_cast<u64>(p); |
||
| assert(err == 0); | ||
| } | ||
|
Copilot marked this conversation as resolved.
|
||
|
|
||
| double get() { | ||
| void* p = pthread_getspecific(_key); | ||
| if (p == nullptr) { | ||
| return 0.0; | ||
| } | ||
|
|
||
| u64 val = (u64)p; | ||
| double value; | ||
| memcpy(&value, &val, sizeof(val)); | ||
| return value; | ||
| } | ||
|
|
||
| void clear() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM · robustness] Suggestion: mirror the defensive pattern in the generic |
||
| int err = pthread_setspecific(_key, nullptr); | ||
| assert(err == 0); | ||
| } | ||
|
Copilot marked this conversation as resolved.
|
||
| }; | ||
|
|
||
| #endif // _THREADLOCAL_H | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LOW · correctness] (Pre-existing, not introduced by this PR)
skipped.set(0)resets the accumulated skipped-bytes counter even when the current event was dropped (table full), losing weight that should carry forward to the next successfully recorded sample.Suggestion: move
skipped.set(0)inside theidx < _table_capbranch so it only resets when the sample was actually recorded.