Message ID | 1482510792.14990.820.camel@redhat.com |
---|---|
State | New |
Headers | show |
On 12/23/2016 11:33 AM, Torvald Riegel wrote: > On Tue, 2016-06-14 at 22:53 +0200, Torvald Riegel wrote: >> I've now tested this patch successfully using our existing tests on ppc, >> ppc64, ppc64le, s390x, and aarch64. I couldn't test on s390 due to >> https://www.sourceware.org/ml/libc-alpha/2016-06/msg00545.html. >> >> The attached patch is a minor revision that just fixes some formatting >> and adds an include (revealing by testing on s390x). > I have attached a followup patch that adds support for the pretty > printers added in the meantime. For ease-of-review, I'm sending this as > a separate path and not a revision of the previous patch; I will commit > it as a single patch though. This looks good to me, though the implementation is fairly rudimentary. I would like us to continue to make this better _before_ the release to help developers working on the various ports that need to make this work. When I say "better" I mean expose more algorithm details. So this should get checked in with the new condvar, but we should cricle back to this before the release. Cheers, Carlos. > commit 157cc3cc3e7c6656337f23079272d4247dd53814 > Author: Torvald Riegel <triegel@redhat.com> > Date: Fri Dec 23 14:52:08 2016 +0100 > > Additional support for pretty printers. > > diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py > index e402f23..c207e9c 100644 > --- a/nptl/nptl-printers.py > +++ b/nptl/nptl-printers.py > @@ -293,16 +293,6 @@ class MutexAttributesPrinter(object): > elif protocol == PTHREAD_PRIO_PROTECT: > self.values.append(('Protocol', 'Priority protect')) > > -CLOCK_IDS = { > - CLOCK_REALTIME: 'CLOCK_REALTIME', > - CLOCK_MONOTONIC: 'CLOCK_MONOTONIC', > - CLOCK_PROCESS_CPUTIME_ID: 'CLOCK_PROCESS_CPUTIME_ID', > - CLOCK_THREAD_CPUTIME_ID: 'CLOCK_THREAD_CPUTIME_ID', > - CLOCK_MONOTONIC_RAW: 'CLOCK_MONOTONIC_RAW', > - CLOCK_REALTIME_COARSE: 'CLOCK_REALTIME_COARSE', > - CLOCK_MONOTONIC_COARSE: 'CLOCK_MONOTONIC_COARSE' > -} > - > class ConditionVariablePrinter(object): > """Pretty printer for pthread_cond_t.""" > > @@ -313,24 +303,8 @@ class ConditionVariablePrinter(object): > cond: A gdb.value representing a pthread_cond_t. > """ > > - # Since PTHREAD_COND_SHARED is an integer, we need to cast it to void * > - # to be able to compare it to the condvar's __data.__mutex member. > - # > - # While it looks like self.shared_value should be a class variable, > - # that would result in it having an incorrect size if we're loading > - # these printers through .gdbinit for a 64-bit objfile in AMD64. > - # This is because gdb initially assumes the pointer size to be 4 bytes, > - # and only sets it to 8 after loading the 64-bit objfiles. Since > - # .gdbinit runs before any objfiles are loaded, this would effectively > - # make self.shared_value have a size of 4, thus breaking later > - # comparisons with pointers whose types are looked up at runtime. > - void_ptr_type = gdb.lookup_type('void').pointer() > - self.shared_value = gdb.Value(PTHREAD_COND_SHARED).cast(void_ptr_type) > - > data = cond['__data'] > - self.total_seq = data['__total_seq'] > - self.mutex = data['__mutex'] > - self.nwaiters = data['__nwaiters'] > + self.wrefs = data['__wrefs'] > self.values = [] > > self.read_values() > @@ -360,7 +334,6 @@ class ConditionVariablePrinter(object): > > self.read_status() > self.read_attributes() > - self.read_mutex_info() > > def read_status(self): > """Read the status of the condvar. > @@ -369,41 +342,22 @@ class ConditionVariablePrinter(object): > are waiting for it. > """ > > - if self.total_seq == PTHREAD_COND_DESTROYED: > - self.values.append(('Status', 'Destroyed')) > - > - self.values.append(('Threads waiting for this condvar', > - self.nwaiters >> COND_NWAITERS_SHIFT)) > + self.values.append(('Threads known to still execute a wait function', > + self.wrefs >> PTHREAD_COND_WREFS_SHIFT)) > > def read_attributes(self): > """Read the condvar's attributes.""" > > - clock_id = self.nwaiters & ((1 << COND_NWAITERS_SHIFT) - 1) > - > - # clock_id must be casted to int because it's a gdb.Value > - self.values.append(('Clock ID', CLOCK_IDS[int(clock_id)])) > + if (self.wrefs & PTHREAD_COND_CLOCK_MONOTONIC_MASK) != 0: > + self.values.append(('Clock ID', 'CLOCK_MONOTONIC')) > + else: > + self.values.append(('Clock ID', 'CLOCK_REALTIME')) > > - shared = (self.mutex == self.shared_value) > - > - if shared: > + if (self.wrefs & PTHREAD_COND_SHARED_MASK) != 0: > self.values.append(('Shared', 'Yes')) > else: > self.values.append(('Shared', 'No')) > > - def read_mutex_info(self): > - """Read the data of the mutex this condvar is bound to. > - > - A pthread_cond_t's __data.__mutex member is a void * which > - must be casted to pthread_mutex_t *. For shared condvars, this > - member isn't recorded and has a special value instead. > - """ > - > - if self.mutex and self.mutex != self.shared_value: > - mutex_type = gdb.lookup_type('pthread_mutex_t') > - mutex = self.mutex.cast(mutex_type.pointer()).dereference() > - > - self.values.append(('Mutex', mutex)) > - > class ConditionVariableAttributesPrinter(object): > """Pretty printer for pthread_condattr_t. > > @@ -453,10 +407,12 @@ class ConditionVariableAttributesPrinter(object): > created in self.children. > """ > > - clock_id = self.condattr & ((1 << COND_NWAITERS_SHIFT) - 1) > + clock_id = (self.condattr >> 1) & ((1 << COND_CLOCK_BITS) - 1) > > - # clock_id must be casted to int because it's a gdb.Value > - self.values.append(('Clock ID', CLOCK_IDS[int(clock_id)])) > + if clock_id != 0: > + self.values.append(('Clock ID', 'CLOCK_MONOTONIC')) > + else: > + self.values.append(('Clock ID', 'CLOCK_REALTIME')) > > if self.condattr & 1: > self.values.append(('Shared', 'Yes')) > diff --git a/nptl/nptl_lock_constants.pysym b/nptl/nptl_lock_constants.pysym > index 303ec61..2ab3179 100644 > --- a/nptl/nptl_lock_constants.pysym > +++ b/nptl/nptl_lock_constants.pysym > @@ -44,26 +44,13 @@ PTHREAD_PRIO_NONE > PTHREAD_PRIO_INHERIT > PTHREAD_PRIO_PROTECT > > --- These values are hardcoded as well: > --- Value of __mutex for shared condvars. > -PTHREAD_COND_SHARED (void *)~0l > - > --- Value of __total_seq for destroyed condvars. > -PTHREAD_COND_DESTROYED -1ull > - > --- __nwaiters encodes the number of threads waiting on a condvar > --- and the clock ID. > --- __nwaiters >> COND_NWAITERS_SHIFT gives us the number of waiters. > -COND_NWAITERS_SHIFT > - > --- Condvar clock IDs > -CLOCK_REALTIME > -CLOCK_MONOTONIC > -CLOCK_PROCESS_CPUTIME_ID > -CLOCK_THREAD_CPUTIME_ID > -CLOCK_MONOTONIC_RAW > -CLOCK_REALTIME_COARSE > -CLOCK_MONOTONIC_COARSE > +-- Condition variable > +-- FIXME Why do macros prefixed with __ cannot be used directly? > +PTHREAD_COND_SHARED_MASK __PTHREAD_COND_SHARED_MASK > +PTHREAD_COND_CLOCK_MONOTONIC_MASK __PTHREAD_COND_CLOCK_MONOTONIC_MASK > +COND_CLOCK_BITS > +-- These values are hardcoded: > +PTHREAD_COND_WREFS_SHIFT 3 > > -- Rwlock attributes > PTHREAD_RWLOCK_PREFER_READER_NP > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > index 6e0dd09..92a9992 100644 > --- a/nptl/pthreadP.h > +++ b/nptl/pthreadP.h > @@ -167,6 +167,13 @@ enum > #define __PTHREAD_ONCE_FORK_GEN_INCR 4 > > > +/* Condition variable definitions. See __pthread_cond_wait_common. > + Need to be defined here so there is one place from which > + nptl_lock_constants can grab them. */ > +#define __PTHREAD_COND_CLOCK_MONOTONIC_MASK 2 > +#define __PTHREAD_COND_SHARED_MASK 1 > + > + > /* Internal variables. */ > > > diff --git a/nptl/pthread_cond_init.c b/nptl/pthread_cond_init.c > index cdd9b7d..c1eac5f 100644 > --- a/nptl/pthread_cond_init.c > +++ b/nptl/pthread_cond_init.c > @@ -29,6 +29,10 @@ __pthread_cond_init (pthread_cond_t *cond, const pthread_condattr_t *cond_attr) > struct pthread_condattr *icond_attr = (struct pthread_condattr *) cond_attr; > > memset (cond, 0, sizeof (pthread_cond_t)); > + > + /* Update the pretty printers if the internal representation of icond_attr > + is changed. */ > + > /* Iff not equal to ~0l, this is a PTHREAD_PROCESS_PRIVATE condvar. */ > if (icond_attr != NULL && (icond_attr->value & 1) != 0) > cond->__data.__wrefs |= __PTHREAD_COND_SHARED_MASK; > diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c > index 984f01f..2b43402 100644 > --- a/nptl/pthread_cond_wait.c > +++ b/nptl/pthread_cond_wait.c > @@ -293,6 +293,8 @@ __condvar_cleanup_waiting (void *arg) > * Bit 1 is the clock ID (0 == CLOCK_REALTIME, 1 == CLOCK_MONOTONIC). > * Bit 0 is true iff this is a process-shared condvar. > * Simple reference count used by both waiters and pthread_cond_destroy. > + (If the format of __wrefs is changed, update nptl_lock_constants.pysym > + and the pretty printers.) > For each of the two groups, we have: > __g_refs: Futex waiter reference count. > * LSB is true if waiters should run futex_wake when they remove the > diff --git a/nptl/test-cond-printers.py b/nptl/test-cond-printers.py > index af0e12e..9e807c9 100644 > --- a/nptl/test-cond-printers.py > +++ b/nptl/test-cond-printers.py > @@ -35,7 +35,7 @@ try: > > break_at(test_source, 'Test status (destroyed)') > continue_cmd() # Go to test_status_destroyed > - test_printer(var, to_string, {'Status': 'Destroyed'}) > + test_printer(var, to_string, {'Threads known to still execute a wait function': '0'}) > > continue_cmd() # Exit > > diff --git a/scripts/test_printers_common.py b/scripts/test_printers_common.py > index c79d7e3..91cf5ec 100644 > --- a/scripts/test_printers_common.py > +++ b/scripts/test_printers_common.py > @@ -157,7 +157,7 @@ def init_test(test_bin, printer_files, printer_names): > > # Load all the pretty printer files. We're assuming these are safe. > for printer_file in printer_files: > - test('source {0}'.format(printer_file)) > + test('source {0}'.format(printer_file), '') > > # Disable all the pretty printers. > test('disable pretty-printer', r'0 of [0-9]+ printers enabled') > diff --git a/sysdeps/x86/bits/pthreadtypes.h b/sysdeps/x86/bits/pthreadtypes.h > index 59b1d0d..38bc454 100644 > --- a/sysdeps/x86/bits/pthreadtypes.h > +++ b/sysdeps/x86/bits/pthreadtypes.h > @@ -161,8 +161,6 @@ typedef union > unsigned int __g1_orig_size; > unsigned int __wrefs; > unsigned int __g_signals[2]; > -#define __PTHREAD_COND_CLOCK_MONOTONIC_MASK 2 > -#define __PTHREAD_COND_SHARED_MASK 1 > } __data; > char __size[__SIZEOF_PTHREAD_COND_T]; > __extension__ long long int __align; -- Cheers, Carlos.
commit 157cc3cc3e7c6656337f23079272d4247dd53814 Author: Torvald Riegel <triegel@redhat.com> Date: Fri Dec 23 14:52:08 2016 +0100 Additional support for pretty printers. diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py index e402f23..c207e9c 100644 --- a/nptl/nptl-printers.py +++ b/nptl/nptl-printers.py @@ -293,16 +293,6 @@ class MutexAttributesPrinter(object): elif protocol == PTHREAD_PRIO_PROTECT: self.values.append(('Protocol', 'Priority protect')) -CLOCK_IDS = { - CLOCK_REALTIME: 'CLOCK_REALTIME', - CLOCK_MONOTONIC: 'CLOCK_MONOTONIC', - CLOCK_PROCESS_CPUTIME_ID: 'CLOCK_PROCESS_CPUTIME_ID', - CLOCK_THREAD_CPUTIME_ID: 'CLOCK_THREAD_CPUTIME_ID', - CLOCK_MONOTONIC_RAW: 'CLOCK_MONOTONIC_RAW', - CLOCK_REALTIME_COARSE: 'CLOCK_REALTIME_COARSE', - CLOCK_MONOTONIC_COARSE: 'CLOCK_MONOTONIC_COARSE' -} - class ConditionVariablePrinter(object): """Pretty printer for pthread_cond_t.""" @@ -313,24 +303,8 @@ class ConditionVariablePrinter(object): cond: A gdb.value representing a pthread_cond_t. """ - # Since PTHREAD_COND_SHARED is an integer, we need to cast it to void * - # to be able to compare it to the condvar's __data.__mutex member. - # - # While it looks like self.shared_value should be a class variable, - # that would result in it having an incorrect size if we're loading - # these printers through .gdbinit for a 64-bit objfile in AMD64. - # This is because gdb initially assumes the pointer size to be 4 bytes, - # and only sets it to 8 after loading the 64-bit objfiles. Since - # .gdbinit runs before any objfiles are loaded, this would effectively - # make self.shared_value have a size of 4, thus breaking later - # comparisons with pointers whose types are looked up at runtime. - void_ptr_type = gdb.lookup_type('void').pointer() - self.shared_value = gdb.Value(PTHREAD_COND_SHARED).cast(void_ptr_type) - data = cond['__data'] - self.total_seq = data['__total_seq'] - self.mutex = data['__mutex'] - self.nwaiters = data['__nwaiters'] + self.wrefs = data['__wrefs'] self.values = [] self.read_values() @@ -360,7 +334,6 @@ class ConditionVariablePrinter(object): self.read_status() self.read_attributes() - self.read_mutex_info() def read_status(self): """Read the status of the condvar. @@ -369,41 +342,22 @@ class ConditionVariablePrinter(object): are waiting for it. """ - if self.total_seq == PTHREAD_COND_DESTROYED: - self.values.append(('Status', 'Destroyed')) - - self.values.append(('Threads waiting for this condvar', - self.nwaiters >> COND_NWAITERS_SHIFT)) + self.values.append(('Threads known to still execute a wait function', + self.wrefs >> PTHREAD_COND_WREFS_SHIFT)) def read_attributes(self): """Read the condvar's attributes.""" - clock_id = self.nwaiters & ((1 << COND_NWAITERS_SHIFT) - 1) - - # clock_id must be casted to int because it's a gdb.Value - self.values.append(('Clock ID', CLOCK_IDS[int(clock_id)])) + if (self.wrefs & PTHREAD_COND_CLOCK_MONOTONIC_MASK) != 0: + self.values.append(('Clock ID', 'CLOCK_MONOTONIC')) + else: + self.values.append(('Clock ID', 'CLOCK_REALTIME')) - shared = (self.mutex == self.shared_value) - - if shared: + if (self.wrefs & PTHREAD_COND_SHARED_MASK) != 0: self.values.append(('Shared', 'Yes')) else: self.values.append(('Shared', 'No')) - def read_mutex_info(self): - """Read the data of the mutex this condvar is bound to. - - A pthread_cond_t's __data.__mutex member is a void * which - must be casted to pthread_mutex_t *. For shared condvars, this - member isn't recorded and has a special value instead. - """ - - if self.mutex and self.mutex != self.shared_value: - mutex_type = gdb.lookup_type('pthread_mutex_t') - mutex = self.mutex.cast(mutex_type.pointer()).dereference() - - self.values.append(('Mutex', mutex)) - class ConditionVariableAttributesPrinter(object): """Pretty printer for pthread_condattr_t. @@ -453,10 +407,12 @@ class ConditionVariableAttributesPrinter(object): created in self.children. """ - clock_id = self.condattr & ((1 << COND_NWAITERS_SHIFT) - 1) + clock_id = (self.condattr >> 1) & ((1 << COND_CLOCK_BITS) - 1) - # clock_id must be casted to int because it's a gdb.Value - self.values.append(('Clock ID', CLOCK_IDS[int(clock_id)])) + if clock_id != 0: + self.values.append(('Clock ID', 'CLOCK_MONOTONIC')) + else: + self.values.append(('Clock ID', 'CLOCK_REALTIME')) if self.condattr & 1: self.values.append(('Shared', 'Yes')) diff --git a/nptl/nptl_lock_constants.pysym b/nptl/nptl_lock_constants.pysym index 303ec61..2ab3179 100644 --- a/nptl/nptl_lock_constants.pysym +++ b/nptl/nptl_lock_constants.pysym @@ -44,26 +44,13 @@ PTHREAD_PRIO_NONE PTHREAD_PRIO_INHERIT PTHREAD_PRIO_PROTECT --- These values are hardcoded as well: --- Value of __mutex for shared condvars. -PTHREAD_COND_SHARED (void *)~0l - --- Value of __total_seq for destroyed condvars. -PTHREAD_COND_DESTROYED -1ull - --- __nwaiters encodes the number of threads waiting on a condvar --- and the clock ID. --- __nwaiters >> COND_NWAITERS_SHIFT gives us the number of waiters. -COND_NWAITERS_SHIFT - --- Condvar clock IDs -CLOCK_REALTIME -CLOCK_MONOTONIC -CLOCK_PROCESS_CPUTIME_ID -CLOCK_THREAD_CPUTIME_ID -CLOCK_MONOTONIC_RAW -CLOCK_REALTIME_COARSE -CLOCK_MONOTONIC_COARSE +-- Condition variable +-- FIXME Why do macros prefixed with __ cannot be used directly? +PTHREAD_COND_SHARED_MASK __PTHREAD_COND_SHARED_MASK +PTHREAD_COND_CLOCK_MONOTONIC_MASK __PTHREAD_COND_CLOCK_MONOTONIC_MASK +COND_CLOCK_BITS +-- These values are hardcoded: +PTHREAD_COND_WREFS_SHIFT 3 -- Rwlock attributes PTHREAD_RWLOCK_PREFER_READER_NP diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h index 6e0dd09..92a9992 100644 --- a/nptl/pthreadP.h +++ b/nptl/pthreadP.h @@ -167,6 +167,13 @@ enum #define __PTHREAD_ONCE_FORK_GEN_INCR 4 +/* Condition variable definitions. See __pthread_cond_wait_common. + Need to be defined here so there is one place from which + nptl_lock_constants can grab them. */ +#define __PTHREAD_COND_CLOCK_MONOTONIC_MASK 2 +#define __PTHREAD_COND_SHARED_MASK 1 + + /* Internal variables. */ diff --git a/nptl/pthread_cond_init.c b/nptl/pthread_cond_init.c index cdd9b7d..c1eac5f 100644 --- a/nptl/pthread_cond_init.c +++ b/nptl/pthread_cond_init.c @@ -29,6 +29,10 @@ __pthread_cond_init (pthread_cond_t *cond, const pthread_condattr_t *cond_attr) struct pthread_condattr *icond_attr = (struct pthread_condattr *) cond_attr; memset (cond, 0, sizeof (pthread_cond_t)); + + /* Update the pretty printers if the internal representation of icond_attr + is changed. */ + /* Iff not equal to ~0l, this is a PTHREAD_PROCESS_PRIVATE condvar. */ if (icond_attr != NULL && (icond_attr->value & 1) != 0) cond->__data.__wrefs |= __PTHREAD_COND_SHARED_MASK; diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c index 984f01f..2b43402 100644 --- a/nptl/pthread_cond_wait.c +++ b/nptl/pthread_cond_wait.c @@ -293,6 +293,8 @@ __condvar_cleanup_waiting (void *arg) * Bit 1 is the clock ID (0 == CLOCK_REALTIME, 1 == CLOCK_MONOTONIC). * Bit 0 is true iff this is a process-shared condvar. * Simple reference count used by both waiters and pthread_cond_destroy. + (If the format of __wrefs is changed, update nptl_lock_constants.pysym + and the pretty printers.) For each of the two groups, we have: __g_refs: Futex waiter reference count. * LSB is true if waiters should run futex_wake when they remove the diff --git a/nptl/test-cond-printers.py b/nptl/test-cond-printers.py index af0e12e..9e807c9 100644 --- a/nptl/test-cond-printers.py +++ b/nptl/test-cond-printers.py @@ -35,7 +35,7 @@ try: break_at(test_source, 'Test status (destroyed)') continue_cmd() # Go to test_status_destroyed - test_printer(var, to_string, {'Status': 'Destroyed'}) + test_printer(var, to_string, {'Threads known to still execute a wait function': '0'}) continue_cmd() # Exit diff --git a/scripts/test_printers_common.py b/scripts/test_printers_common.py index c79d7e3..91cf5ec 100644 --- a/scripts/test_printers_common.py +++ b/scripts/test_printers_common.py @@ -157,7 +157,7 @@ def init_test(test_bin, printer_files, printer_names): # Load all the pretty printer files. We're assuming these are safe. for printer_file in printer_files: - test('source {0}'.format(printer_file)) + test('source {0}'.format(printer_file), '') # Disable all the pretty printers. test('disable pretty-printer', r'0 of [0-9]+ printers enabled') diff --git a/sysdeps/x86/bits/pthreadtypes.h b/sysdeps/x86/bits/pthreadtypes.h index 59b1d0d..38bc454 100644 --- a/sysdeps/x86/bits/pthreadtypes.h +++ b/sysdeps/x86/bits/pthreadtypes.h @@ -161,8 +161,6 @@ typedef union unsigned int __g1_orig_size; unsigned int __wrefs; unsigned int __g_signals[2]; -#define __PTHREAD_COND_CLOCK_MONOTONIC_MASK 2 -#define __PTHREAD_COND_SHARED_MASK 1 } __data; char __size[__SIZEOF_PTHREAD_COND_T]; __extension__ long long int __align;