trace2: fix tracing when NO_PTHREADS is defined

As Duy suggested, pthread_getspecific() just returns NULL when NO_PTHREADS is defined. And pthread_setspecific() silently does not nothing. So this problem was hidden from view.

I have to wonder if we should update pthread_*specific() to call BUG() when NO_PTHREADS is defined as a way to catch unguarded usages easier or make this issue more clear.

Quoted 7 lines · Jeff Hostetler via GitGitGadget

As Duy suggested, pthread_getspecific() just returns NULL when NO_PTHREADS is defined. And pthread_setspecific() silently does not nothing. So this problem was hidden from view.

I have to wonder if we should update pthread_*specific() to call BUG() when NO_PTHREADS is defined as a way to catch unguarded usages easier or make this issue more clear.

I think it should actually store the data asked for by the caller, as if we were the single thread running. We discussed this as the time of refactoring NO_PTHREADS, but there was only one caller that would have benefited. Now there are two. ;)

Discussion in the subthread of this patch:

   https://public-inbox.org/git/[email protected]/

-Peff

I was wondering about that too as the proper long term solution. We would need (as the discussion suggests [1]) to properly respect/represent the pthread_key_t argument.

For now, I've guarded my usage of pthread_getspecific() in the trace2 (similar to what index-pack does), so its not urgent that we update it. And I'd rather we take this simple trace2 fix now and not try to combine it with fixes for the pthread macros. Especially now as we're in the RC cycle for 2.22.

I'll make a note to revisit the pthread code after 2.22.

Quoted 9 lines · Jeff Hostetler

I was wondering about that too as the proper long term solution. We would need (as the discussion suggests [1]) to properly respect/represent the pthread_key_t argument.

For now, I've guarded my usage of pthread_getspecific() in the trace2 (similar to what index-pack does), so its not urgent that we update it. And I'd rather we take this simple trace2 fix now and not try to combine it with fixes for the pthread macros. Especially now as we're in the RC cycle for 2.22.

Yeah, I think that makes sense.

Quoted 1 line

I'll make a note to revisit the pthread code after 2.22.

For fun, here's a constant-time zero-allocation implementation that I came up with. It passes t0211 with NO_PTHREADS, but I didn't test it beyond that.

thread-utils.h +31 -5

index 4961487ed9..f466215742 100644
@@ -18,7 +18,7 @@
  #define pthread_t int
  #define pthread_mutex_t int
  #define pthread_cond_t int
-#define pthread_key_t int
+#define pthread_key_t git_pthread_key_t
 
  #define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)
  #define pthread_mutex_lock(mutex)

@@ -31,16 +31,49 @@
  #define pthread_cond_broadcast(cond)
  #define pthread_cond_destroy(cond)
 
-#define pthread_key_create(key, attr) dummy_pthread_init(key)
-#define pthread_key_delete(key)
+#define pthread_key_create(key, destroy) git_pthread_key_create(key, destroy)
+#define pthread_key_delete(key) git_pthread_key_delete(key)
 
  #define pthread_create(thread, attr, fn, data) \
  	dummy_pthread_create(thread, attr, fn, data)
  #define pthread_join(thread, retval) \
  	dummy_pthread_join(thread, retval)
 
-#define pthread_setspecific(key, data)
-#define pthread_getspecific(key) NULL
+#define pthread_setspecific(key, data) git_pthread_setspecific(key, data)
+#define pthread_getspecific(key) git_pthread_getspecific(key)
+
+typedef struct {
+	void *data;
+	/* extra indirection because setspecific is passed key by value */
+	void **vdata;
+} git_pthread_key_t;
+
+static inline int git_pthread_key_create(git_pthread_key_t *key,
+					 void (*destroy)(void *))
+{
+	key->data = NULL;
+	key->vdata = &key->data;
+	/* We don't use this; alternatively we could all via atexit(). */
+	if (destroy)
+		BUG("NO_PTHREADS does not support pthread key destructors");
+	return 0;
+}
+
+static inline int git_pthread_key_delete(git_pthread_key_t key)
+{
+	/* noop */
+	return 0;
+}
+
+static inline void git_pthread_setspecific(git_pthread_key_t key, void *data)
+{

For fun, here's a constant-time zero-allocation implementation that I came up with. It passes t0211 with NO_PTHREADS, but I didn't test it beyond that.

thread-utils.h +3 -3

index 4961487ed9..f466215742 100644
@@ -18,7 +18,7 @@
 #define pthread_t int
 #define pthread_mutex_t int
 #define pthread_cond_t int
-#define pthread_key_t int
+#define pthread_key_t git_pthread_key_t
 
 #define pthread_mutex_init(mutex, attr) dummy_pthread_init(mutex)
 #define pthread_mutex_lock(mutex)

@@ -31,16 +31,49 @@
 #define pthread_cond_broadcast(cond)
 #define pthread_cond_destroy(cond)
 
-#define pthread_key_create(key, attr) dummy_pthread_init(key)
-#define pthread_key_delete(key)
+#define pthread_key_create(key, destroy) git_pthread_key_create(key, destroy)
+#define pthread_key_delete(key) git_pthread_key_delete(key)
 
 #define pthread_create(thread, attr, fn, data) \
        dummy_pthread_create(thread, attr, fn, data)
 #define pthread_join(thread, retval) \
        dummy_pthread_join(thread, retval)

thread-utils.h +28 -0

+} git_pthread_key_t;
+
+static inline int git_pthread_key_create(git_pthread_key_t *key,
+                                        void (*destroy)(void *))
+{
+       key->data = NULL;
+       key->vdata = &key->data;
+       /* We don't use this; alternatively we could all via atexit(). */
+       if (destroy)
+               BUG("NO_PTHREADS does not support pthread key destructors");
+       return 0;
+}
+
+static inline int git_pthread_key_delete(git_pthread_key_t key)
+{
+       /* noop */
+       return 0;
+}
+
+static inline void git_pthread_setspecific(git_pthread_key_t key, void *data)
+{
+       *(key.vdata) = data;
+}
+
+static inline void *git_pthread_getspecific(git_pthread_key_t key)
+{
+       return key.data;
+}
 
 int dummy_pthread_create(pthread_t *pthread, const void *attr,
                         void *(*fn)(void *), void *data);

thread-utils.h line 48

Quoted 4 lines

thread-utils.h +4 -0

+typedef struct {
+       void *data;
+       /* extra indirection because setspecific is passed key by value */
+       void **vdata;

Ha! I was thinking a separate key->value mapping which is complicated in C. But this works pretty well for a single thread, and it even supports multiple keys.

Quoted 8 lines · Duy Nguyen

thread-utils.h line 48

Quoted 4 lines

thread-utils.h +4 -0

+typedef struct {
+       void *data;
+       /* extra indirection because setspecific is passed key by value */
+       void **vdata;

Ha! I was thinking a separate key->value mapping which is complicated in C. But this works pretty well for a single thread, and it even supports multiple keys.

I really wish that all of the functions passed the pthread_key_t by reference. That would make it possible to define the key as a single pointer.

I'm not sure if pthread_key_t's are meant to be shallow-copyable. I.e., should this work:

   void foo(pthread_key_t *out)
   {
	pthread_key_t tmp;
	pthread_key_create(&tmp, NULL);
	*out = tmp;
   }
   ...
   pthread_key_t k;
   foo(&k);
   pthread_setspecific(k, some_ptr);

It does not with my proposed plan, because the pointer in tmp.data went out of scope, leaving tmp.vdata (and thus k.vdata) as a dangling pointer.

The code above seems like a vaguely crazy thing to do. But if we want to be absolutely paranoid, we'd have to malloc() an extra pointer in the create() function, instead of carrying it inside the key. Or just make a global "void *thread_specific_data[PTHREAD_KEYS_MAX]" and make each key an integer index into it.

It's pretty clear that they expect one of those two implementations, given that POSIX says key creation can report either ENOMEM, or EAGAIN if we exceed PTHREAD_KEYS_MAX. :)

-Peff

For now, I've guarded my usage of pthread_getspecific() in the trace2 (similar to what index-pack does), so its not urgent that we update it. And I'd rather we take this simple trace2 fix now and not try to combine it with fixes for the pthread macros. Especially now as we're in the RC cycle for 2.22.

I'll make a note to revisit the pthread code after 2.22.