OSDev.org

The Place to Start for Operating System Developers
It is currently Fri Mar 29, 2024 9:20 am

All times are UTC - 6 hours




Post new topic Reply to topic  [ 9 posts ] 
Author Message
 Post subject: Worst bug I've had today
PostPosted: Tue Sep 20, 2011 7:27 am 
Offline
Member
Member

Joined: Thu Mar 25, 2010 11:26 pm
Posts: 1801
Location: Melbourne, Australia
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:
    __asm__ ("cli\n\t");
    apic_write(LAPIC_EOI, 0);
Here's the corrected code.
Code:
    __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.

_________________
If a trainstation is where trains stop, what is a workstation ?


Top
 Profile  
 
 Post subject: Re: Worst bug I've had today
PostPosted: Tue Sep 20, 2011 7:38 am 
Offline
Member
Member

Joined: Tue Aug 21, 2007 1:41 am
Posts: 207
Location: Germany
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.


Top
 Profile  
 
 Post subject: Re: Worst bug I've had today
PostPosted: Tue Sep 20, 2011 9:35 am 
Offline
Member
Member
User avatar

Joined: Sat Jul 17, 2010 12:45 am
Posts: 487
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.

_________________
Programming is not about using a language to solve a problem, it's about using logic to find a solution !


Top
 Profile  
 
 Post subject: Re: Worst bug I've had today
PostPosted: Tue Sep 20, 2011 10:02 am 
Offline
Member
Member

Joined: Tue Aug 21, 2007 1:41 am
Posts: 207
Location: Germany
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.


Top
 Profile  
 
 Post subject: Re: Worst bug I've had today
PostPosted: Tue Sep 20, 2011 10:13 am 
Offline
Member
Member
User avatar

Joined: Thu Nov 16, 2006 12:01 pm
Posts: 7612
Location: Germany
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".

_________________
Every good solution is obvious once you've found it.


Top
 Profile  
 
 Post subject: Re: Worst bug I've had today
PostPosted: Tue Sep 20, 2011 5:09 pm 
Offline
Member
Member

Joined: Thu Mar 25, 2010 11:26 pm
Posts: 1801
Location: Melbourne, Australia
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:
__asm__ volatile ("cli");
apic_write(LAPIC_EOI, 0);
becomes
Code:
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:
__asm__ ("cli": : :"memory");
apic_write(LAPIC_EOI, 0);
becomes
Code:
ffffffff80002e92:   fa                      cli   
ffffffff80002e93:   c7 04 25 b0 e0 1f 80    movl   $0x0,0xffffffff801fe0b0

_________________
If a trainstation is where trains stop, what is a workstation ?


Top
 Profile  
 
 Post subject: Re: Worst bug I've had today
PostPosted: Tue Sep 20, 2011 6:44 pm 
Offline
Member
Member

Joined: Thu Mar 25, 2010 11:26 pm
Posts: 1801
Location: Melbourne, Australia
As an aside, it is better to add the volatile keyword like this
Code:
__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.

_________________
If a trainstation is where trains stop, what is a workstation ?


Top
 Profile  
 
 Post subject: Re: Worst bug I've had today
PostPosted: Wed Sep 21, 2011 4:04 am 
Offline
Member
Member
User avatar

Joined: Thu Nov 16, 2006 12:01 pm
Posts: 7612
Location: Germany
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-Inline-Assembly-HOWTO.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... :?

_________________
Every good solution is obvious once you've found it.


Top
 Profile  
 
 Post subject: Re: Worst bug I've had today
PostPosted: Wed Sep 21, 2011 5:18 pm 
Offline
Member
Member

Joined: Thu Mar 25, 2010 11:26 pm
Posts: 1801
Location: Melbourne, Australia
Quote:
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.

_________________
If a trainstation is where trains stop, what is a workstation ?


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 9 posts ] 

All times are UTC - 6 hours


Who is online

Users browsing this forum: No registered users and 34 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB © 2000, 2002, 2005, 2007 phpBB Group