Re: [x86] nptl/tst-sem13: sem_timedwait

[Thomas Schwinge <thomas at codesourcery dot com>]

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

Attachment: pgp00000.pgp
Description: PGP signature