Re: CVS commit: src/sys/arch/sparc

[Martin Husemann <martin%duskware.de@localhost>]

On Sun, Oct 20, 2013 at 10:05:30AM +0200, Alan Barrett wrote:
> For example:
> 
> >#define raise_ipi(cpi,lvl)   do {                    \
> >-    volatile int x;                                 \
> >+    int x;                                          \
> >     (cpi)->intreg_4m->pi_set = PINTR_SINTRLEV(lvl); \
> >-    x = (cpi)->intreg_4m->pi_pend;                  \
> >+    x = (cpi)->intreg_4m->pi_pend; __USE(x);        \
> >} while (0)

I don't think this change is incorrect, but I would do this differently.

If the RHS is not properly marked volatile, the compiler might cache the
value and repeatadly store the same value again, so the volatile hidden
somewhere in the RHS is important. I wonder, however, why it does not
properly warn here (loss of qualifiers), so we should realy investigate
this closer.

However, assuming the RHS is ok, it is IMHO better to get rid of "i" completely
and do:

#define raise_ipi(cpi,lvl)      do {                    \
        (cpi)->intreg_4m->pi_set = PINTR_SINTRLEV(lvl); \
        (void)(cpi)->intreg_4m->pi_pend;                \
} while (0)

See for example src/sys/dev/sun/cgsix.c:

/*
 * Run a blitter command
 */
#define CG6_BLIT(f) { (void)f->fbc_blit; }

/*
 * Run a drawing command
 */
#define CG6_DRAW(f) { (void)f->fbc_draw; }


Martin