Re: [PING][PATCH v3] Add pretty printers for the NPTL lock types

[Martin Galvan <martin dot galvan at tallertechnologies dot com>]

On Wed, Nov 18, 2015 at 3:29 PM, Torvald Riegel <triegel@redhat.com> wrote:
> I haven't checked consistency with glibc's implementation in detail.  I
> really think we should have tests for that instead of having to do that
> manually.

I looked at how the glibc tests are implemented, and they seem to be
all written strictly in C. Tom said in a previous e-mail that the
libstdc++ printers have unit tests which run gdb and so on, but they
seem to use DejaGNU, which I didn't see in glibc. FWIW I'd rather not
write TCL and instead use something like PExpect, but that's just me.
What do you suggest?

> There is still a good amount of glibc implementation internals mirrored
> in the pretty printers implementation.  It would be nice if this would
> not be the case, but I haven't thought about whether/what might improve
> this.

I don't think that's possible. By definition, the printers need to
understand how the NPTL data structures work in order to print useful
information about them.

>> +    def read_status(self):
>> +        """Read the mutex's status.
>> +
>> +        For architectures which support lock elision, this method reads
>> +        whether the mutex is actually locked (i.e. it may show it as unlocked
>> +        even after calling pthread_mutex_lock).
>
> Is an uncommitted transaction actually visible to debuggers?  It may if
> the debugger is injecting code into the binary, so that no interrupt or
> context switch happen.  It may also be visible if we have something like
> a suspendable transaction, and the debugger thinks it can peek inside of
> txns.  I believe that currently, we can assume that uncommitted state is
> invisible even to the debugger.

It should be, although I didn't dig that deeply into this.

> Either way, "actually locked" in the comment isn't correct.  The mutex
> is actually locked when using lock elision, it's just that this isn't
> reflected in memory necessarily.

Ok. Will rewrite this to "this method reads whether the mutex appears
as locked in memory".

>> +        if self.lock == PTHREAD_MUTEX_UNLOCKED:
>> +            self.values.append(('Status', 'Unlocked'))
>> +        else:
>> +            if self.lock & FUTEX_WAITERS:
>> +                self.values.append(('Status', 'Locked, possibly with waiters'))
>> +            else:
>> +                self.values.append(('Status', 'Locked, no waiters'))
>
> I think those strings should consider that there might be waiters that
> just spin.  When waiters register, it is because they want to wait using
> futexes.  Right now, we don't spin, but this should change eventually,
> or is already the case for adaptive mutexes.  I suggest changing the
> strings to something like "locked, possibly with waiters" and "locked,
> possibly no waiters" to reflect that.

I will, although maybe that could be a bit confusing for the user?

>> +        self.values.append(('Threads waiting for this condvar',
>> +                            self.nwaiters >> COND_NWAITERS_SHIFT))
>
> Generally, the same applies as for mutexes.  However, it's not the case
> in the current condvar implementation, and we'll replace that before we
> add any substantial spinning.

Should I rewrite this as "Threads possibly waiting for this condvar"?

>> +-- These values are hardcoded as well:
>> +-- Value of __mutex for shared condvars.
>> +PTHREAD_COND_SHARED             ~0l
>
> Will this do the right thing for 32b, 64b, and x32?

It should. After all, those values are hardcoded just like that on the
NPTL code, which is compiled the same way as the output of
gen-py-const.awk.