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

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

Hi Joseph! Thanks a lot for the feedback.

On Fri, May 15, 2015 at 5:42 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> I think we must eliminate this duplication before this patch can go in.
> That is, set up some mechanism for the values to be extracted at build /
> install time.  In addition, whenever this depends on some aspect of NPTL
> internals, there need to be comments on the relevant internals (e.g.
> structure field definitions) explaining how this code depends on them and
> so needs updating for any change.  Similarly, if this code is meant to
> handle all values in an enumeration / all flags from some set of flags,
> there needs to be a comment on that enumeration / set of flags drawing
> attention to the need to update this code when new values are added.

Wouldn't commenting on the headers suffice? Those values are scattered
all over the place. I *could* try to build an additional Python script
to extract them from the headers somehow, but it's probably gonna take
a lot of time.

> Another issue: ctypes relates, I presume, to the types used by the
> compiler used to compile Python.  But what you want here is the types used
> in glibc by the program being debugged, which may be different, both
> because of cross-debugging, and because a native 64-bit GDB may debug both
> 32-bit and 64-bit inferiors.
>
> That is, you can't compute these using ctypes; they have to be computed
> using the types for the current inferior.  I don't know enough about the
> GDB Python interfaces to know whether what you have elsewhere in this
> patch

Understood.

>> +PTHREAD_MUTEX_INCONSISTENT = gdb.parse_and_eval('~0u >> 1')
>
> is safe (whether this Python code will be loaded separately for 32-bit and
> 64-bit inferiors and that evaluation, taking place when this module is
> loaded, will use the correct type sizes for the inferior in question).

It should be.

> If it is safe, I'd expect it to be possible to use the same approach for
> PTHREAD_COND_*.

Probably. I'll give it a try and send v2.

> (Of course, if you're relying on a value like this that
> isn't a constant defined in and extracted from a header, the relevant
> glibc code needs a comment to point out this dependency.  Or, better,
> define the value as a macro in an internal header, use that macro where
> glibc depends on the value, and extract from the internal header for
> Python use.)

These values are hardcoded in many places. While I agree that it would
be good to define them as macros, I think improving the glibc coding
style is beyond the scope of this particular contribution :)

Again, thanks a lot for the feedback. I'll wait until someone else
takes a look at the patch and suggests more changes/improvements, then
I'll send v2.