Re: bind9 failures on mips

[Robert Edmonds <edmonds@debian.org>]

James Cowgill wrote:
> On Mon, 2015-09-07 at 15:04 -0400, Robert Edmonds wrote:
> > Hi,
> > 
> > I've been reviewing bugs filed against bind9, and I came across #778720
> > ("Bin[d]9 hangs on mips jessie based machine"), which points to a forum
> > thread [0] where multiple people have reported problems running our
> > bind9 package on mips-based networking devices.
> [...]
> > That leaves the most complicated operation, isc_atomic_cmpxchg(), which
> > unfortunately isn't exercised by the "t_atomic" test program.  Does this
> > code [5] look correct?
> > 
> >     /*
> >      * This routine atomically replaces the value in 'p' with 'val', if the
> >      * original value is equal to 'cmpval'.  The original value is returned in any
> >      * case.
> >      */
> >     static inline isc_int32_t
> >     isc_atomic_cmpxchg(isc_int32_t *p, int cmpval, int val) {
> >         isc_int32_t orig;
> >         isc_int32_t tmp;
> > 
> >         __asm__ __volatile__ (
> >         "   .set    push        \n"
> >         "   .set    mips2       \n"
> >         "   .set    noreorder   \n"
> >         "   .set    noat        \n"
> >         "1: ll  $1, %1      \n"
> >         "   bne $1, %3, 2f  \n"
> >         "   move    %2, %4      \n"
> >         "   sc  %2, %1      \n"
> >         "   beqz    %2, 1b      \n"
> >         "2: move    %0, $1      \n"
> >         "   .set    pop     \n"
> >         : "=&r"(orig), "+R" (*p), "=r" (tmp)
> >         : "r"(cmpval), "r"(val)
> >         : "memory");
> > 
> >         return (orig);
> >     }
> 
> The tmp register constraint should be earlyclobber ("=&r") because the
> other two registers (%3 and %4) will be read again after tmp (%2) is
> written if the code loops around. At the moment, GCC could make tmp and
> cmpval/val refer to the same register.

Hi, James:

Thanks!  So, "=r" (tmp) should be "=&r" (tmp) ?  (Sorry, I am not an
assembly programmer.)

> But to be honest, I would just replace all the functions in that file
> with calls to the C11 atomic functions which GCC implements itself -
> this way you can be sure they're right:

What minimum version of gcc would be necessary?  The last time I looked
at this topic, gcc still did not have stdatomic.h, but I see that's
changed now.

> #include <stdatomic.h>
> 
> static inline isc_int32_t
> isc_atomic_xadd(isc_int32_t *p, int val) {
> 	return atomic_fetch_add(p, val);
> }
> 
> static inline void
> isc_atomic_store(isc_int32_t *p, isc_int32_t val) {
> 	atomic_store(p, val);
> }
> 
> static inline isc_int32_t
> isc_atomic_cmpxchg(isc_int32_t *p, int cmpval, int val) {
>         atomic_compare_exchange_strong(p, &cmpval, val);
>         return cmpval;
> }

This would indeed be much nicer, but I suspect upstream would still want
to keep the old implementations around for compatibility reasons,
necessitating some configure-time detection macros and more #ifdef's.

-- 
Robert Edmonds
edmonds@debian.org