Bug 9302 - system_64.h: switch_to inline asm should be more robbust wrt optimizations
Summary: system_64.h: switch_to inline asm should be more robbust wrt optimizations
Status: CLOSED OBSOLETE
Alias: None
Product: Platform Specific/Hardware
Classification: Unclassified
Component: x86-64 (show other bugs)
Hardware: All Linux
: P1 low
Assignee: platform_x86_64@kernel-bugs.osdl.org
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-04 01:28 UTC by Török Edwin
Modified: 2012-05-17 15:01 UTC (History)
5 users (show)

See Also:
Kernel Version: 2.6.24-rc1, commit b4d367fb20ed19be4a53fa88b407248aeb8bd461
Tree: Mainline
Regression: No


Attachments

Description Andrew Morton 2007-11-04 01:11:35 UTC
Please send a tested kernel patch as per
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

to

Thomas Gleixner <tglx@linutronix.de>
Ingo Molnar <mingo@elte.hu>,
Andrew Morton <akpm@linux-foundation.org>
linux-kernel@vger.kernel.org

thanks.
Comment 1 Török Edwin 2007-11-04 01:28:48 UTC
Most recent kernel where this bug did not occur: unknown
Distribution: Debian unstable
Hardware Environment: x86-64
Software Environment: llvm-gcc svn version (llvm.org)
Problem Description:
switch_to declares a global symbol in inline assembly, and switch_to is called within a conditional branch. Some optimizers can create 2 copies of the asm for each path, thus leading to a 'symbol thread_return already defined error'.

For example, this occurs when trying to compile the kernel with llvm.
The llvm developers have implemented a workaround to compile this code,
however please consider their comments, see discussion here:
http://llvm.org/bugs/show_bug.cgi?id=1764 

Quote:
"Duncan is right, the code is invalid.  It should use "local labels" with
forward/backward so that each copy of the asm block gets its own unique label.
With this patch, the code "works".  Your code is still buggy and I reserve the
right to revert the patch if there is a non-zero negative performance impact on
the nightly testers tonight"


Steps to reproduce:
CROSS_COMPILE=llvm- O=/home/edwin/builds/o make
/tmp/cck288u7.s: Assembler messages:
/tmp/cck288u7.s:56: Error: symbol `thread_return' is already defined
Comment 2 Chris Lattner 2007-11-04 10:58:54 UTC
To clarify, a compiler is allowed to duplicate inline asm blocks.  Declaring a label in an inline asm block is thus a recipe for getting duplicate symbol errors.
Comment 3 Andi Kleen 2007-11-13 05:18:41 UTC
The global label was intentional to mark thread return in backtraces. Losing
that would decrease backtrace quality considerably. This is a very common
case.

iirc some kernel debuggers even special case it and require that label.

I suspect the only good fix for LLVM would be to move it to a separate function
 
Comment 4 Andi Kleen 2007-11-13 05:22:05 UTC
BTW the asm is marked volatile. Shouldn't that stop duplication too?
Comment 5 H. Peter Anvin 2007-11-13 08:08:55 UTC
Why would volatile mean don't duplicate me?
Comment 6 Chris Lattner 2007-11-13 21:42:53 UTC
volatile just means that the compiler can't delete it if it has no outputs.  The code can be duplicated statically as many times as it wants, so long as any dynamic path through the code executes it the right number of times.

GCC has the same issue in other cases, you are depending on really fragile behavior here.
Comment 7 Andi Kleen 2007-11-14 06:49:52 UTC
We'll worry about it when gcc does it.

What would be also an option would be to find a way to make 
the label unique. Is there something like __LINE__ on pure assembler
level that could be added to it?
Comment 8 Chris Lattner 2007-11-14 08:26:46 UTC
__COUNTER__
Comment 9 Chris Lattner 2007-11-14 08:30:49 UTC
Better yet, GCC inline asm allows you to use "%=" in the asm, which turns into a unique ID.  Unlike __COUNTER__ this is portable to very old versions of GCC.

Just use mylabel%=:
Comment 10 Andi Kleen 2007-11-14 08:37:40 UTC
__COUNTER__ is for the C preprocessor isn't it? That is useless in this case
because the duplication happens below the preprocessor level.
%= might work.
Comment 11 Thomas Gleixner 2008-09-04 13:45:58 UTC
Will fix, when GCC barfs or someone sends a patch according to http://bugzilla.kernel.org/show_bug.cgi?id=9302#c0
Comment 12 Andrew Thomas Pinski 2008-11-24 11:20:26 UTC
GCC can duplicate inline-asm in some cases, like unrolling.  Now unrolling is not on by default for GCC but that does not mean it will not always work.  Yes using %= is better.

Thanks,
Andrew Pinski
Comment 13 Török Edwin 2008-11-24 11:26:28 UTC
Reopening on apinski's request.

llvm-gcc no longer duplicates the asm in this case, but according to comment #12, gcc can duplicate it in some cases.

FWIW there is an inconsistency with the 32-bit code in include/asm-x86/system.h which doesn't have .global thread_return, and the 64-bit code which does.

Note You need to log in before you can comment on or make changes to this bug.