Re: Bug in atomic_sub_if_positive

[Thiemo Seufer <ths@networkno.de>]

Atsushi Nemoto wrote:
> On Wed, 2 Jul 2008 10:59:56 +0100, Thiemo Seufer <ths@networkno.de> wrote:
> > > --- a/include/asm-mips/atomic.h	2008-06-25 22:38:43.159739000 -0700
> > > +++ b/include/asm-mips/atomic.h	2008-06-25 22:39:07.552065000 -0700
> > > @@ -292,10 +292,10 @@ static __inline__ int atomic_sub_if_posi
> > >  		"	beqz	%0, 2f					\n"
> > >  		"	 subu	%0, %1, %3				\n"
> > >  		"	.set	reorder					\n"
> > > -		"1:							\n"
> > >  		"	.subsection 2					\n"
> > >  		"2:	b	1b					\n"
> > >  		"	.previous					\n"
> > > +		"1:							\n"
> > 
> > AFAICS this change should make no difference to the generated code. I
> > suspect you assembler handles .subsection incorrectly. Can you provide
> > a disassembled exapmle which gets altered by this patch? Also, please
> > tell us the exact version of the assembler you use.
> 
> Why no defference?  The '1b' in subsection refer backword '1' label on
> the source code (which is a label for LL insn in this case with the
> patch) ?

Oh. Now that you spelled it out I see it, too. :-)

> Anyway I can provide them.  I'm using vanilla 2.17 and 2.18.
> 
> Without the patch:
> 801233bc <try_acquire_console_sem>:
> 801233bc:	lui	v0,0x8038
> 801233c0:	ll	a0,31976(v0)
> 801233c4:	addiu	v1,a0,-1
> 801233c8:	bltz	v1,801233dc <try_acquire_console_sem+0x20>
> 801233cc: 	nop
> 801233d0: 	sc	v1,31976(v0)
> 801233d4: 	beqz	v1,80124dac <sys_syslog+0x8>
> 801233d8: 	addiu	v1,a0,-1
> 801233dc: 	bltz	v1,801233fc <try_acquire_console_sem+0x40>
> 801233e0: 	li	v0,-1
> ...
> 80124dac:	b	801233dc <try_acquire_console_sem+0x20>
> 80124db0:	nop
> 
> With the patch:
> 801233bc <try_acquire_console_sem>:
> 801233bc:	lui	v0,0x8038
> 801233c0:	ll	a0,31976(v0)
> 801233c4:	addiu	v1,a0,-1
> 801233c8:	bltz	v1,801233dc <try_acquire_console_sem+0x20>
> 801233cc:	nop
> 801233d0:	sc	v1,31976(v0)
> 801233d4:	beqz	v1,80124dac <sys_syslog+0x8>
> 801233d8:	addiu	v1,a0,-1
> 801233dc:	bltz	v1,801233fc <try_acquire_console_sem+0x40>
> 801233e0:	li	v0,-1
> ...
> 80124dac:	b	801233c0 <try_acquire_console_sem+0x4>
> 80124db0:	nop
> 
> 
> The patch looks correct.

Agreed.


Thiemo