Hi Chris, your commit 103140 broke PR397 for llvm-gcc (in LLVM 2.9) and dragonegg. In the PR, asm renaming creates two linkonce functions with the same asm name (in the IR they are @"\01lstat64" and "@lstat64". What used to happen is that they were both output to the assembler file, both with the name lstat64, exactly like GCC does. The assembler and linker are perfectly happy about this, presumably because the functions have weak linkage. What happens now is that compilation fails with "label emitted multiple times to assembly file". Do you agree that it is reasonable to support outputting multiple functions with the same name, as long as they have weak linkage? Ciao, Duncan. PS: The alternative is to follow clang and have the front-end take care of dropping one of the functions, rather than leaving it to the linker.> Index: test/CodeGen/X86/label-redefinition.ll > ==================================================================> --- test/CodeGen/X86/label-redefinition.ll (revision 0) > +++ test/CodeGen/X86/label-redefinition.ll (revision 103140) > @@ -0,0 +1,15 @@ > +; PR7054 > +; RUN: not llc %s -o - |& grep {'_foo' label emitted multiple times to assembly} > +target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128-n8:16:32" > +target triple = "i386-apple-darwin10.0.0" > + > +define i32 @"\01_foo"() { > + unreachable > +} > + > +define i32 @foo() { > +entry: > + unreachable > +} > + > +declare i32 @xstat64(i32, i8*, i8*) > Index: lib/CodeGen/AsmPrinter/AsmPrinter.cpp > ==================================================================> --- lib/CodeGen/AsmPrinter/AsmPrinter.cpp (revision 103139) > +++ lib/CodeGen/AsmPrinter/AsmPrinter.cpp (revision 103140) > @@ -408,7 +408,13 @@ > /// EmitFunctionEntryLabel - Emit the label that is the entrypoint for the > /// function. This can be overridden by targets as required to do custom stuff. > void AsmPrinter::EmitFunctionEntryLabel() { > - OutStreamer.EmitLabel(CurrentFnSym); > + // The function label could have already been emitted if two symbols end up > + // conflicting due to asm renaming. Detect this and emit an error. > + if (CurrentFnSym->isUndefined()) > + return OutStreamer.EmitLabel(CurrentFnSym); > + > + report_fatal_error("'" + Twine(CurrentFnSym->getName()) + > + "' label emitted multiple times to assembly file"); > } >
On 13 March 2012 12:26, Duncan Sands <baldrick at free.fr> wrote:> Hi Chris, your commit 103140 broke PR397 for llvm-gcc (in LLVM 2.9) and > dragonegg. In the PR, asm renaming creates two linkonce functions with > the same asm name (in the IR they are @"\01lstat64" and "@lstat64". What > used to happen is that they were both output to the assembler file, both > with the name lstat64, exactly like GCC does. The assembler and linker > are perfectly happy about this, presumably because the functions have weak > linkage. What happens now is that compilation fails with "label emitted > multiple times to assembly file". > > Do you agree that it is reasonable to support outputting multiple functions > with the same name, as long as they have weak linkage? > > Ciao, Duncan. > > PS: The alternative is to follow clang and have the front-end take care of > dropping one of the functions, rather than leaving it to the linker.If you can implement this I think it is better. Having two functions with the same name can cause problems to libLTO for example. Which function should it use? Cheers, Rafael
On Mar 13, 2012, at 8:26 AM, Duncan Sands wrote:> Hi Chris, your commit 103140 broke PR397 for llvm-gcc (in LLVM 2.9) and > dragonegg.Wow, this is an old patch :).> In the PR, asm renaming creates two linkonce functions with > the same asm name (in the IR they are @"\01lstat64" and "@lstat64". What > used to happen is that they were both output to the assembler file, both > with the name lstat64, exactly like GCC does. The assembler and linker > are perfectly happy about this, presumably because the functions have weak > linkage. What happens now is that compilation fails with "label emitted > multiple times to assembly file". > > Do you agree that it is reasonable to support outputting multiple functions > with the same name, as long as they have weak linkage?No, I don't. I think that an IR module should be required to be well defined and obey the rules. For GCC/clang (and any other compilers that support things like asm renaming and USER_LABEL_PREFIX), I think it is best for the frontend to not use the "\01" prefix in a case that conflicts with the normal USER_LABEL_PREFIX. For example, on an _'y system, if asm-renamed to "_foo", the IR name should be just @"foo", not "\01_foo". -Chris
Hi Chris, On 13/03/12 21:11, Chris Lattner wrote:> > On Mar 13, 2012, at 8:26 AM, Duncan Sands wrote: > >> Hi Chris, your commit 103140 broke PR397 for llvm-gcc (in LLVM 2.9) and >> dragonegg. > > Wow, this is an old patch :). > >> In the PR, asm renaming creates two linkonce functions with >> the same asm name (in the IR they are @"\01lstat64" and "@lstat64". What >> used to happen is that they were both output to the assembler file, both >> with the name lstat64, exactly like GCC does. The assembler and linker >> are perfectly happy about this, presumably because the functions have weak >> linkage. What happens now is that compilation fails with "label emitted >> multiple times to assembly file". >> >> Do you agree that it is reasonable to support outputting multiple functions >> with the same name, as long as they have weak linkage? > > No, I don't. I think that an IR module should be required to be well defined and obey the rules. For GCC/clang (and any other compilers that support things like asm renaming and USER_LABEL_PREFIX), I think it is best for the frontend to not use the "\01" prefix in a case that conflicts with the normal USER_LABEL_PREFIX. For example, on an _'y system, if asm-renamed to "_foo", the IR name should be just @"foo", not "\01_foo".I think I agree. My agreement is helped by noticing that the assembler produced by gcc for the testcase from PR397 doesn't assemble! One of my side worries was that the MC layer wouldn't be able to assemble the assembler produced by gcc if it contained multiple functions with the same name, but since gas rejects it too there is no compatibility issue there after all. Ciao, Duncan.