Page 1 of 1
Worst bug I've had today
Posted: Tue Sep 20, 2011 7:27 am
by 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.
Re: Worst bug I've had today
Posted: Tue Sep 20, 2011 7:38 am
by cyr1x
gerryg400 wrote: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.
Re: Worst bug I've had today
Posted: Tue Sep 20, 2011 9:35 am
by Chandra
cyr1x wrote:
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.
Re: Worst bug I've had today
Posted: Tue Sep 20, 2011 10:02 am
by cyr1x
Chandra wrote: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.
Re: Worst bug I've had today
Posted: Tue Sep 20, 2011 10:13 am
by 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".
Re: Worst bug I've had today
Posted: Tue Sep 20, 2011 5:09 pm
by gerryg400
cyr1x wrote: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
Re: Worst bug I've had today
Posted: Tue Sep 20, 2011 6:44 pm
by 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.
Re: Worst bug I've had today
Posted: Wed Sep 21, 2011 4:04 am
by Solar
gerryg400 wrote: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...
Re: Worst bug I've had today
Posted: Wed Sep 21, 2011 5:18 pm
by 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.