Hi Andrew,
On Fri, Oct 11, 2013 at 01:24:29AM +0100, Andrew Pinski wrote:
On Mon, Oct 7, 2013 at 2:51 AM, Will Deacon will.deacon@arm.com wrote:
On Fri, Oct 04, 2013 at 10:51:02PM +0100, Andrew Pinski wrote:
Yes this will be a full schedule barrier due to the inline-asm being a volatile inline-asm (implicitly because there are no outputs). What I meant about the compiler could move around code only happens when you don't there are other implicit non schedule barrier instructions, in this case wmb is an volatile inline-asm which is also a scheduler barrier.
volatile != scheduling barrier (example below).
Actually it does mean that but scheduling barrier != code motion barrier; at least in GCC.
Bah, terminology. I can tell you're a compiler engineer ;)
Assembles to the following with GCC 4.9 for ARM:
00000000 <foo>: 0: e5900000 ldr r0, [r0] 4: f57ff04f dsb sy 8: e320f000 nop {0} c: f57ff04f dsb sy 10: e280000a add r0, r0, #10 14: e12fff1e bx lr
Yes but this is not the scheduler doing the code motion (it is SCEV Constant prop followed by TER [temporary expression replacement] which is causing the code motion to happen).
Regardless, it's still not suitable for breakpoints in general.
Note all other targets (ARM, PPC, x86, tile, etc.) all use a static inline function which does exactly the same as what is being added here.
Agreed, I didn't mean to imply this was an issue with this particular patch. I'm just curious as to why this has been deemed acceptable by other architectures, when it doesn't look especially useful to me.
The main reason why it is acceptable by other architectures is due to the full definition of the function itself: void kgdb_breakpoint(void) { atomic_inc(&kgdb_setting_breakpoint); wmb(); /* Sync point before breakpoint */ arch_kgdb_breakpoint(); wmb(); /* Sync point after breakpoint */ atomic_dec(&kgdb_setting_breakpoint); }
All of these function calls are all volatile inline-asm so there is not going to be any code motion happening. This is true in all architectures. So then you ask if kgdb_breakpoint can be inlined anywhere that we could get the breakpoint and not have the "correct" value, the answer is no and I audited each and every call to kgdb_breakpoint. If you want just add the attribute noinline to kgdb_breakpoint and the problem you mentioned about the code motion happening goes away.
Thanks for taking a look at this. I think adding the noinline would be a good idea (as a seperate patch).
Cheers,
Will