[x86] nptl/tst-sem13: sem_timedwait

[Thomas Schwinge]

Hi!

Carlos, thanks for reviewing!

On Fri, 17 Feb 2012 18:14:56 -0500, Carlos O'Donell <carlos@systemhalted.org> wrote:
> On Mon, Feb 6, 2012 at 6:28 AM, Thomas Schwinge <thomas@codesourcery.com> wrote:
> > As shown by the added test case, in
> > 9554ebf2d4da22591e974d3cf2ed09a2b8dbdbd8 there has another error mode
> > been introduced: ``2nd sem_timedwait modified nwaiters''.

> >        * nptl/sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S
> >        (sem_timedwait): Fix updating nwaiters.
> 
> In the future please provide a detailed explanation of the
> failure mode and how your fix corrects the failure.

Ack.

> Given that you appear to understand the assembly implementation
> is there any way you could add comments *everywhere* explaining
> exactly what it's doing? :-)

I don't think it's worth it, given there are many such implementations,
and just having a single one documented but not the others is not ideal.
But what I'd like to suggest is that -- unless it's obvious -- there
should be a comment inside such assembly implementation files (where also
a generic C implementation exists) that motivates why we need/benefit
From a specific assembly implementation.  Also, if an assembly file has
been generated by taking the output gcc -S [generic.c], this should be
noted, along with the inormation of what has been changed.  I think this
would improve maintainability.  (As demonstrated here...)


> > +       movl    28(%esp), %ebx  /* Load semaphore address.  */
> > +       LOCK
> > +       decl    NWAITERS(%ebx)
> > +.Lerrno_exit:
> >  #ifdef PIC
> >        call    __i686.get_pc_thunk.bx
> >  #else
> > @@ -168,7 +172,6 @@ sem_timedwait:
> >        movl    %esi, (%eax)
> >  #endif
> >
> > -       movl    28(%esp), %ebx  /* Load semaphore address.  */
> 
> This is a spurious load and you remove it...
> 
> Odd that it should appear here... the author probably meant
> to use the loaded address to decrement NWAITERS on exit but
> forgot.

In fact, I just moved it back to where it was in the
pre-9554ebf2d4da22591e974d3cf2ed09a2b8dbdbd8 state.


> The changes look good to me.

Committed.


> Do you need to adjust .eh_frame or .gcc_except_table to adjust
> for your changes? If not, why not?

No, because I have not done any structural changes to the blocks these
are concerned about.  (And that should probably be re-coded using CFI
statments, etc.)


Grüße,
 Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 489 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20120308/1c6cac62/attachment.sig>