Worst bug I've had today

[gerryg400]

It's taken 2 days of staring at my code and some unbelievable luck to find this bug. It caused very occasional crashes and lockups.

I was trying to disable interrupts before acking the APIC so I could return from my interrupt handler before the next interrupt arrived, like this.

Code: Select all

    __asm__ ("cli\n\t");
    apic_write(LAPIC_EOI, 0);

Here's the corrected code.

Code: Select all

    __asm__ ("cli": : :"memory\n\t");
    apic_write(LAPIC_EOI, 0);

GCC was kindly swapping the 2 lines of code to help me.

Now I'm wondering how many other problems I have like this.

[cyr1x]

GCC was kindly swapping the 2 lines of code to help me.

GCC has '__asm__ volatile' for that, which should be used always in my opinion.

[Chandra]

GCC has '__asm__ volatile' for that, which should be used always in my opinion.

Always? That's doesn't sound right. GCC has it's own way of optimizing things. Always using volatile can prevent GCC from performing flexible optimization. So I think it should be used where necessary, especially under the cases where you don't want your piece of code shuffled.

[cyr1x]

Always? That's doesn't sound right.

I think whenever you write inline assembly you're trying to be smarter than the compiler or you want to use special instructions(sti, cli, lgdt, ...), thus you probably don't want the compiler to interfere.
There are some exceptions to this, like when writing SSE-code, but I'd rather use the GCC's builtins for that.

[Solar]

There's a reason for things like the clobber list in GCC's inline ASM syntax: So the compiler knows what your ASM code is doing and what it isn't doing, so it can properly optimize its way around your inline ASM.

The asm volatile syntax is when you want to be specific about where in your code - relative to surrounding code - your ASM code should be located. Use it for that purpose, but don't start using volatile "just because", for the same reason you shouldn't make every C/C++ variable in your code "volatile".

[gerryg400]

GCC has '__asm__ volatile' for that, which should be used always in my opinion.

That's not what volatile is for. It prevents the asm statement from being deleted but it doesn't stop GCC from changing the order.

For example

Code: Select all

__asm__ volatile ("cli");
apic_write(LAPIC_EOI, 0);

becomes

Code: Select all

ffffffff80002e92:	c7 04 25 b0 e0 1f 80 	movl   $0x0,0xffffffff801fe0b0
ffffffff80002e99:	00 00 00 00 
ffffffff80002e9d:	fa                   	cli    

BUT to get the correct outcome you need

Code: Select all

__asm__ ("cli": : :"memory");
apic_write(LAPIC_EOI, 0);

becomes

Code: Select all

ffffffff80002e92:	fa                   	cli    
ffffffff80002e93:	c7 04 25 b0 e0 1f 80 	movl   $0x0,0xffffffff801fe0b0

[gerryg400]

As an aside, it is better to add the volatile keyword like this

Code: Select all

__asm__ volatile ("cli": : :"memory");

However it is not strictly necessary. Because there are no outputs specified, GCC assumes that the assembler sequence must have side effects and does not delete it. Volatile is only required if there are outputs specified and those outputs are not used in the optimised code.

[Solar]

That's not what volatile is for. It prevents the asm statement from being deleted but it doesn't stop GCC from changing the order.

Uh...strange.

http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html says:

"The volatile keyword indicates that the instruction has important side-effects. GCC will not delete a volatile asm if it is reachable."

But http://www.ibiblio.org/gferg/ldp/GCC-In ... html#ss5.4 (which I based my post above upon) says:

"If our assembly statement must execute where we put it, (i.e. must not be moved out of a loop as an optimization), put the keyword volatile after asm and before the ()’s."

Teaches you not to rely on third-party docs...

[gerryg400]

If our assembly statement must execute where we put it, (i.e. must not be moved out of a loop as an optimization), put the keyword volatile after asm and before the ()’s. So to keep it from moving, deleting and all, we declare it as

asm volatile ( ... : ... : ... : ...);

I have a feeling (though I haven't tested it) that this statement is probably correct if by 'moving' they mean 'moving out of a loop'. Moving out of a loop is the same as deleting. (e.g. moving out of a 10x loop is the same a deleting 9 copies of the code).

I must admit though that I have always read this document precisely as you did until yesterday, so let's blame the document.