Bug 9302
| Summary: | system_64.h: switch_to inline asm should be more robbust wrt optimizations | ||
|---|---|---|---|
| Product: | Platform Specific/Hardware | Reporter: | Török Edwin (edwin+bugs) |
| Component: | x86-64 | Assignee: | platform_x86_64 (platform_x86_64) |
| Status: | CLOSED OBSOLETE | ||
| Severity: | low | CC: | akpm, alan, andi-bz, randy.dunlap, sabre |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 2.6.24-rc1, commit b4d367fb20ed19be4a53fa88b407248aeb8bd461 | Subsystem: | |
| Regression: | No | Bisected commit-id: | |
|
Description
Andrew Morton
2007-11-04 01:11:35 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 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. 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 BTW the asm is marked volatile. Shouldn't that stop duplication too? Why would volatile mean don't duplicate me? 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. 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? __COUNTER__ 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%=: __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. Will fix, when GCC barfs or someone sends a patch according to http://bugzilla.kernel.org/show_bug.cgi?id=9302#c0 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 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. |