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: [x86] nptl/tst-sem13: sem_timedwait


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''.
>
> 2012-01-24 ?Thomas Schwinge ?<thomas@codesourcery.com>
>
> ? ? ? ?* nptl/tst-sem13.c (do_test): Add another test case.

The test case changes are OK.

> ? ? ? ?* 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.

I know that the test case explains some of this, but it is
good for *me* to double check that your understanding of
the failure and the test case are actually equal.

> Index: nptl/sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S
> ===================================================================
> --- nptl/sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S ? ? ?(revision 356686)
> +++ nptl/sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S ? ? ?(working copy)

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

> @@ -65,7 +65,7 @@ sem_timedwait:
> ? ? ? ?/* Check for invalid nanosecond field. ?*/
> ? ? ? ?cmpl ? ?$1000000000, 4(%edi)
> ? ? ? ?movl ? ?$EINVAL, %esi
> - ? ? ? jae ? ? 6f
> + ? ? ? jae ? ? .Lerrno_exit

We haven't incremented NWAITERS yet so we jump right to exit...

> ? ? ? ?LOCK
> ? ? ? ?incl ? ?NWAITERS(%ecx)
> @@ -147,6 +147,10 @@ sem_timedwait:
> ?.Lafter_ret:
> ?3: ? ? negl ? ?%esi
> ?6:

There is still one jump to 6f that is done by the ETIMEDOUT case.
However, by the time the ETIMEDOUT case is reached we've already
incremeneted NWAITERS. Therefore your fix here is to decrement
NWAITERS as you exit and keep NWAITERS consistent with reality.

There is also a jump to 3f from the -EWOULDBLOCK case that is
also fixed 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.

> ? ? ? ?orl ? ? $-1, %eax
> ? ? ? ?jmp ? ? 10b
> ? ? ? ?.size ? sem_timedwait,.-sem_timedwait

The changes look good to me.

However...

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

Cheers,
Carlos.


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