BUG_ON and gcc don't mix

BUG_ON and gcc don't mix

[Anton Blanchard]

Hi,

I noticed our BUG_ON macros were taking a large number of instructions.
I've built a testcase to analyse it:


#if defined(ASMBUG)

#define BUG_ON(x) do {	\
	__asm__ __volatile__("tdnei %0,0\n" : : "r" ((long)(x))); \
} while (0)

#elif defined(BUILTIN)

#define BUG_ON(x) do { \
	if (x) \
		__builtin_trap(); \
} while (0)

#else

#define BUG_ON(x) do { \
	if (x) { \
		__asm__ __volatile__("twi 31,0,0\n"); \
		__builtin_unreachable(); \
	} \
} while (0)

#endif

int foo(unsigned int *bar)
{
	unsigned int holder_cpu;

	holder_cpu = *bar & 0xffff;
	BUG_ON(holder_cpu >= 32);

	return 1;
}


3 versions. First our current upstream behaviour (-DASMBUG):

   0:	00 00 23 a1 	lhz     r9,0(r3)
   4:	1f 00 89 2b 	cmplwi  cr7,r9,31
   8:	26 00 20 7d 	mfcr    r9
   c:	fe f7 29 55 	rlwinm  r9,r9,30,31,31
  10:	00 00 09 0b 	tdnei   r9,0
  14:	01 00 60 38 	li      r3,1
  18:	20 00 80 4e 	blr

What a load of work. We do the compare, then pull it out of the
condition register and do some more work. We are trying to help gcc
but it seems to be backfiring. Let's try doing a simple version in c:

   0:	00 00 23 a1 	lhz     r9,0(r3)
   4:	1f 00 89 2b 	cmplwi  cr7,r9,31
   8:	0c 00 9d 41 	bgt     cr7,14
   c:	01 00 60 38 	li      r3,1
  10:	20 00 80 4e 	blr
  14:	00 00 e0 0f 	twui    r0,0

Better, we branch out of line to do the trap. But if we could do a
conditional trap properly then we should be able to do even better
(-DBUILTIN):

   0:	00 00 23 a1 	lhz     r9,0(r3)
   4:	01 00 60 38 	li      r3,1
   8:	20 00 a9 0c 	twlgei  r9,32
   c:	20 00 80 4e 	blr

Nice! I remember chasing this down before and the issue is we need the
address of the trap instruction for our bug exception table. Maybe
we need a gcc builtin in which we can get a label on the trap
instruction. Would that be possible?

Anton

Re: BUG_ON and gcc don't mix

[Benjamin Herrenschmidt]

On Tue, 2013-08-20 at 12:37 +1000, Anton Blanchard wrote:
>    0:   00 00 23 a1     lhz     r9,0(r3)
>    4:   01 00 60 38     li      r3,1
>    8:   20 00 a9 0c     twlgei  r9,32
>    c:   20 00 80 4e     blr
> 
> Nice! I remember chasing this down before and the issue is we need the
> address of the trap instruction for our bug exception table. Maybe
> we need a gcc builtin in which we can get a label on the trap
> instruction. Would that be possible?

I suppose we can always just do a label before builtin_trap and have
the BUG code search a few instructions :-)

Cheers,
Ben.

Re: BUG_ON and gcc don't mix

[Alan Modra]

On Tue, Aug 20, 2013 at 12:37:50PM +1000, Anton Blanchard wrote:
> address of the trap instruction for our bug exception table. Maybe
> we need a gcc builtin in which we can get a label on the trap
> instruction. Would that be possible?

Not your actual _EMIT_BUG_ENTRY, but something like this ought to work.
The only trick here is not putting anything after __builtin_trap()..

#define BUG_ON(x) do { \
	if (x) {					\
		__asm__ __volatile__ ("\n1:"		\
			"\t.section __bug_table,\"a\""	\
			"\n\t.long 1b"			\
			"\n\t.previous");		\
                __builtin_trap();			\
	}						\
} while (0)

int foo(unsigned int *bar)
{
        unsigned int holder_cpu;

        holder_cpu = *bar & 0xffff;
        BUG_ON(holder_cpu >= 32);

        return 1;
}

-- 
Alan Modra
Australia Development Lab, IBM



-- 
Alan Modra
Australia Development Lab, IBM

Re: BUG_ON and gcc don't mix

[Alan Modra]

On Tue, Aug 20, 2013 at 02:02:11PM +0930, Alan Modra wrote:
> On Tue, Aug 20, 2013 at 12:37:50PM +1000, Anton Blanchard wrote:
> > address of the trap instruction for our bug exception table. Maybe
> > we need a gcc builtin in which we can get a label on the trap
> > instruction. Would that be possible?
> 
> Not your actual _EMIT_BUG_ENTRY, but something like this ought to work.
> The only trick here is not putting anything after __builtin_trap()..

Doh!  I guess the whole point was to get the condition folded into the
trap, which is foiled by emitting an asm between the condition and
buildin_trap().

-- 
Alan Modra
Australia Development Lab, IBM

RE: BUG_ON and gcc don't mix

[David Laight]

> On Tue, Aug 20, 2013 at 02:02:11PM +0930, Alan Modra wrote:
> > On Tue, Aug 20, 2013 at 12:37:50PM +1000, Anton Blanchard wrote:
> > > address of the trap instruction for our bug exception table. Maybe
> > > we need a gcc builtin in which we can get a label on the trap
> > > instruction. Would that be possible?
> >
> > Not your actual _EMIT_BUG_ENTRY, but something like this ought to =
work.
> > The only trick here is not putting anything after __builtin_trap()..
>
> Doh!  I guess the whole point was to get the condition folded into the
> trap, which is foiled by emitting an asm between the condition and
> buildin_trap().

I was thinking that you could add the label after the trap and
then use '.long 1b-4'. But you'd have to put the asm outside the
conditional - so that wouldn't work if the condition was more
complicated and the trap had to be out of line.

If the trap is out of line, then it could be a long way from
the surrounding code block - so a label 'nearby' in the C isn't
any use.

With fix sized instructions the .text segment of the object files
could be scanned for trap instructions.

	David

Re: BUG_ON and gcc don't mix

[Anton Blanchard]

Hi David,

> I was thinking that you could add the label after the trap and
> then use '.long 1b-4'. But you'd have to put the asm outside the
> conditional - so that wouldn't work if the condition was more
> complicated and the trap had to be out of line.
> 
> If the trap is out of line, then it could be a long way from
> the surrounding code block - so a label 'nearby' in the C isn't
> any use.
> 
> With fix sized instructions the .text segment of the object files
> could be scanned for trap instructions.

I tried something similar a while ago and the 1b-4 trick worked for
95%+ of cases but we did end up with out of line traps. I hacked
something up to look for an example, tagging the start and the end of a
BUG_ON, and seeing if the trap was in fact out of line:

               asm volatile("1:\n");
               if (x)
                       __builtin_trap();
               asm volatile("2:\n");

And this one popped in arch/powerpc/platforms/powernv/pci-ioda.c:

        1:

        ld 9,0(29)
        rlwinm. 8,9,0,29,30 
        beq- 0,.L287

        2:

...

.L287:
        trap

You could follow branches and look for the trap, but I'm sure you could
construct things that would be very hard to automatically parse, eg
multiple back to back BUG_ON()'s. At that point I figured some gcc help
would be nice :)

Anton