use arm assembly bits only for gcc < 4.6 on ARM > 6

I talked this through with doko: Long story short we still want the asm parts in master for backports, were we still might have gcc < 4.6.

Commited to master as:
http://cgit.freedesktop.org/libreoffice/ure/commit/?id=83f2c071758ae7d74669d992e272e50057b895ed

gcc 4.5 generates the same code as in the assembly - or is it just gcc-4.5 linaro on natty?
Even better it adds a dmb instruction which the asm block is missing, causing it to not be SMP safe.

Code generated by default gcc 4.5 on natty.

   a: f04f 0201 mov.w r2, #1
   e: f3bf 8f5f dmb sy
  12: e853 1f00 ldrex r1, [r3]
  16: 4411 add r1, r2
  18: e843 1000 strex r0, r1, [r3]
  1c: f090 0f00 teq r0, #0
  20: d1f7 bne.n 12 <a+0x12>

On 28.02.2011 17:49, Jani Monoses wrote:
> gcc 4.5 generates the same code as in the assembly - or is it just gcc-4.5 linaro on natty?

yes.

> Even better it adds a dmb instruction which the asm block is missing, causing it to not be SMP safe.
>
> Code generated by default gcc 4.5 on natty.
>
> a: f04f 0201 mov.w r2, #1
> e: f3bf 8f5f dmb sy
> 12: e853 1f00 ldrex r1, [r3]
> 16: 4411 add r1, r2
> 18: e843 1000 strex r0, r1, [r3]
> 1c: f090 0f00 teq r0, #0
> 20: d1f7 bne.n 12 <a+0x12>

what dmart explain on #linaro ;)

<dmart> doko_: I would expect __sync_{add,sub}_and_fetch() to be close in
performance to the inlined asm for v6/v7. __sync_*() also has the needed
barriers -- the libreoffice code I see there doesn't, so it might not always
work correctly in some contexts on true SMP... but maybe the barriers happen
elsewhere

Jani: If I understood doko's discussion correctly it we would still need it as 4.5 Linaro not 4.5 FSF.
@doko: Please confirm.

Other than that, the patch would not hurt either way as-is (because it generates the same code), right?

From what doko says indeed on non-Ubuntu (and thus non Linaro) gcc 4.5 does not generate the more optimal code.

In general I was confused by the fact that this is not natty specific. I was thinking lucid/maverick etc have their own packaging branches and the natty package does not carry legacy stuff.

In either case the missing dmb instruction makes this not SMP safe. I think the asm patches were originally written before there were SMP ARM boards available making it a non-issue back then.

removed the arm patches on natty:
http://git.debian.org/?p=pkg-openoffice/libreoffice.git;a=commit;h=47a49ddab9633c45c33a949fe109a8087ef5b141

reenabled the patch for maverick and natty:
http://git.debian.org/?p=pkg-openoffice/libreoffice.git;a=commit;h=b8943507ab7a3c96c21d3c9ae0292b9cf7fa7152
http://git.debian.org/?p=pkg-openoffice/libreoffice.git;a=commit;h=5247b174659b56d55dcea24e0cd7260dc05a8a14

@Jani: As the original code by Erics Bachard is not guaranteed to be SMP-safe, could you please post a patch with the linaro generated code, so we could completely replace the asm code in:
 http://cgit.freedesktop.org/libreoffice/ure/commit/?id=83f2c071758ae7d74669d992e272e50057b895ed
at upstream with it?

This is how the two routines need to be modified:

The code block needs to be wrapped by dmb (memory barrier instruction)

    register int nCount __asm__ ("r1");
    int nResult;

    __asm__ __volatile__ (
"dmb\n" <- here
"1: ldrex %0, [%3]\n"
" add %0, %0, #1\n"
" strex %1, %0, [%3]\n"
" teq %1, #0\n"
" bne 1b\n"
"dmb\n" <-here
        : "=&r" (nCount), "=&r" (nResult), "=m" (*pCount)
        : "r" (pCount)
        : "memory");

I also sent a patch against LibO in email

This bug was fixed in the package libreoffice - 1:3.3.1-1ubuntu5

---------------
libreoffice (1:3.3.1-1ubuntu5) natty; urgency=low

  * completely disable the arm patch for natty (LP: #726529)
  * make libreoffice-gtk recommend either tango or human (LP: #726921)
  * added ppc fixes (LP: #727118)
  * enable lzma everywhere except armel, not the other way around
  * regenerate control file
 -- Bjoern Michaelsen <email address hidden> Tue, 08 Mar 2011 11:45:10 +0100

Added memory barriers upstream as:
 http://cgit.freedesktop.org/libreoffice/ure/commit/?id=75f9926ecd485c349b5c66b8881aa5137aacde26