This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Siddhesh,

On Sun, Apr 10, 2016 at 3:37 AM, Siddhesh Poyarekar
<sid@reserved-bit.com> wrote:
> In addition to a couple of specific issues that I have pointed out
> inline, please also note the following:
>
> - There are a number of lines longer than the allowed width.  For
>   python sources the limit is 80 columns and for C it is 79.

Yes, I know that. I've tried to split lines wherever I could, as long
as it doesn't hurt readability too much. However, doing so on the test
programs confuses gdb to the point where I need to issue a (somewhat
random) number of 'next' commands to advance. Those should stay as
they are.

In any case, seeing as there are heaps of code with lines longer than
80 chars in the codebase already, and my lines exceed the limit only
by 4-5 chars at worst, I don't think it's that big an issue.

> - Comment formatting is incorrect in a number of places - the last
>   line of a C comment should be followed by two spaces and then the
>   close of comment.

Will fix it for v5.

> I think we're very close to getting this in now.  Please post an
> updated version and I'll review and push next week if nobody else
> objects.

That's great to hear! Thanks.

>> +$(py-const): $(common-objpfx)%.py: %.pysym $(py-const-script) $(common-before-compile)
>
> Line longer than 79 chars.

I can split it after the target pattern, but it would hurt
readability. I'd prefer if it stays as it is.

>> +     sed -n -r 's/^.*@name@([^@]+)@value@[^[:xdigit:]Xx-]*([[:xdigit:]Xx-]+)@.*/\1 = \2/p' \
>> +               $(addsuffix .tmp,$@) >> $@
>> +     rm -f $(addsuffix .tmp,$@)
>
> This is not safe.  Write to a temp file first and then move it to the target.

I don't understand. Why isn't it safe?

>         echo '# GENERATED FILE\n' > $@.tmp2
>         echo '# Constant definitions for pretty printers.' >> $@.tmp2
>         echo '# See gen-py-const.awk for details.\n' >> $@.tmp2
>         sed -n -r 's/^.*@name@([^@]+)@value@[^[:xdigit:]Xx-]*([[:xdigit:]Xx-]+)@.*/\1 = \2/p' \
>                   $(addsuffix .tmp,$@) >> $@.tmp2
>         mv -f $@{.tmp2,}

I take it that this is what I should do instead. Also, I didn't know
about that kind of brace expansion until I saw that last line. I guess
it's the same as 'mv -f $@.tmp2 $@', right?

> Also, you're writing the file to the build root directory.  Since the
> file will only change occasionally, it would be a good idea to
> regenerate it only when required and put it in the pretty-printers
> directory.

I remember trying to do that and it failing to work. I think it's
because Makerules runs for each submodule, and the
build/pretty-printers directory doesn't yet exist when Makerules is
running for nptl. I can give it another shot, but I don't think it'll
work.

>> +-- These are hardcoded all over the code; there are no enums/macros for them.
>
> Would you be able to do this cleanup?  That will make the code more
> consistent to read alongside the pretty printers.  This would be a
> separate task of course.

Yeah, I can look into it after this patch is merged. I think the
install stuff should be handled first, though.

>> +# Contributed by Martin Galvan <martin.galvan@tallertechnologies.com>
>
> We don't add the Contributed by line anymore.

Ok, will remove them.

>> +CFLAGS-test-rwlock-printer.c := -O0 -ggdb3 -D__OPTIMIZE__
>
> Build it with -DIS_IN_build instead, which is a much less horrendous hack.

Ok, will try. Thanks.

>> +we register them for the current objfile by calling gdb.printing.register_pretty_printer().
>
> Line longer than 80 chars.

Does that apply to READMEs as well?

>> +* Older versions of the gdb Python API have a bug where gdb.RegexpCollectionPrettyPrinter
>
> Line longer than 80 chars.

Ditto.

>> +* The test programs must be compiled without optimizations.  This is necessary
>> +because the test scripts rely on the C code structure being preserved when
>> +stepping through the programs.  Things like aggressive instruction reordering
>> +or optimizing variables out may make this kind of testing impossible.
>
> This is true for C code in general, so I don't see the point of adding it here.

Can't hurt to state it here again :)

>> +  if (pthread_condattr_setpshared (attr, PTHREAD_PROCESS_SHARED) == 0 /* Set shared */
>
> Line longer than 80 chars.

See the beginning of this e-mail.

>> +static int test_setprotocol (pthread_mutex_t *mutex, pthread_mutexattr_t *attr);
>
> Line longer than 79 columns.

Will wrap it.

>> +  if (pthread_mutexattr_settype (attr, PTHREAD_MUTEX_ERRORCHECK) == 0 /* Set type */
>
> Line longer than 79 columns.

See the beginning of this e-mail.

>> +  if (pthread_mutexattr_setrobust (attr, PTHREAD_MUTEX_ROBUST) == 0 /* Set robust */
>
> Line longer than 79 columns.

Ditto.

>> +  if (pthread_mutexattr_setpshared (attr, PTHREAD_PROCESS_SHARED) == 0 /* Set shared */
>
> Line longer than 79 columns.

Ditto.

>> +  if (pthread_mutexattr_setprotocol (attr, PTHREAD_PRIO_INHERIT) == 0 /* Set protocol */
>
> Line longer than 79 columns.

Ditto.

>> +      && pthread_mutex_setprioceiling(mutex, PRIOCEILING, &old_prioceiling) == 0
>
> Line longer than 79 columns.

Ditto.

>> +  if (pthread_create (&thread, NULL, thread_function, mutex) == 0 /* Test locking and state (robust) */
>
> Line longer than 79 columns.

Ditto.

>> +  if (pthread_rwlockattr_setkind_np (attr, PTHREAD_RWLOCK_PREFER_READER_NP) == 0 /* Set kind */
>> +      && rwlock_reinit (rwlock, attr) == PASS
>> +      && pthread_rwlockattr_setkind_np (attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP) == 0
>
> Lines longer than 79 columns.

Ditto.

>> +  if (pthread_rwlockattr_setpshared (attr, PTHREAD_PROCESS_SHARED) == 0 /* Set shared */
>
> Line longer than 79 columns.

Ditto.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]