Re[2]: [Gc] [libatomic_ops] bug with gcc/x86_64/CAS

Ivan Maidanski ivmai at mail.ru
Wed Feb 17 11:45:44 PST 2010


Hi!
Andrew Haley <aph at redhat.com> wrote:
> On 02/17/2010 04:45 PM, Ivan Maidanski wrote:
> > Hi!
> > Andrew Haley <aph at redhat.com> wrote:
> >> On 02/17/2010 12:17 PM, Patrick MARLIER wrote:
> >>
> >>> I think I found a bug into libatomic_ops into AO_compare_and_swap_full
> >>> function for gcc and x86_64 cpu.
> >>>
> >>> **** Possible FIX 2: set RAX as earlyclobbered output ****
> >>> AO_INLINE int
> >>> AO_compare_and_swap_full(volatile AO_t *addr,
> >>> AO_t old, AO_t new_val)
> >>> {
> >>> char result;
> >>> __asm__ __volatile__("lock; cmpxchgq %4, %0; setz %1"
> >>> : "=m"(*addr), "=q"(result) , "=&a" (old)
> >>> : "m"(*addr), "r" (new_val), "0"(old) : "memory");
> >>> return (int) result;
> >>> }
> >>
> >> I think this asm is best, but it's pretty questionable to use an asm
> >> at all, given that this is a built-in gcc function.
> > 
> > Andrew -
> > 1. could you explain why fix 1 is not so good as fix 2;
> 
> There is no powerful technical reason.

I chose fix 1 just because of less changes.

> 
> > 2. thanks for noting that __sync_... built-in funcs exist in GCC but:
> > - from which GCC version they are supported?;
> 
> I can't remember, but for Intel for a very long time.  If you really
> need to know, I can do some archaeology.

Do all v3 versions or all v4 ones support all the required primitives?

> > - they are supported for all targets (where applicable) or for Intel only?;
> 
> They work on others too, but I know certainly on Intel.

We could create gcc/common_defs.h (as for VC++ case) and include it from gcc/x86.h and friends (for which we know exactly).

> 
> > - it would be good if someone send me a draft (at least) code using them;
> 
> > - as only bug fixes are accepted for gc72, the changes would go to
> > the next major release (unless Hans says the opposite).
> 
> Sure, but I will suggest that a correct bug fix is
> 
> AO_INLINE int
> AO_compare_and_swap_full(volatile AO_t *addr,
> 			 AO_t old, AO_t new_val)
> {
>   return __sync_bool_compare_and_swap (addr, old, new_val);
> }
> 
> unless there is a problem with old gcc, in which case don't do this.

The smaller changes - the better, so I just replaced "=q" with "=a" for gcc/sunc x86/amd64 for now (including for cmpxchg8b/16b).

> Andrew.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bdwgc-ivmai-231.diff
Type: application/octet-stream
Size: 5730 bytes
Desc: not available
Url : http://napali.hpl.hp.com/pipermail/gc/attachments/20100217/75d8f16a/bdwgc-ivmai-231.obj


More information about the Gc mailing list