Bug 18010 - gold -O2 breaks LLVM's TableGen on ppc64
Summary: gold -O2 breaks LLVM's TableGen on ppc64
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gold (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: ---
Assignee: Cary Coutant
URL:
Keywords:
: 18085 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-02-24 06:36 UTC by Markus Trippelsdorf
Modified: 2015-03-22 04:13 UTC (History)
3 users (show)

See Also:
Host: powerpc64-unknown-linux-gnu
Target: powerpc64-unknown-linux-gnu
Build: powerpc64-unknown-linux-gnu
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Trippelsdorf 2015-02-24 06:36:42 UTC
also see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65169

gold on ppc64 breaks LLVM's TableGen when --gc-sections is used:

trippels@gcc2-power8 TableGen % /home/trippels/gcc_test/usr/local/bin/g++ -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wnon-virtual-dtor -Wno-comment -std=c++11 -ffunction-sections -fdata-sections -O3 -mcpu=power8 -fno-strict-aliasing -pipe -Wl,-O1,--hash-style=gnu,--gc-sections -Wl,-allow-shlib-undefined -Wl,-O3 -Wl,--gc-sections CMakeFiles/llvm-tblgen.dir/AsmMatcherEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/AsmWriterEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/AsmWriterInst.cpp.o CMakeFiles/llvm-tblgen.dir/CallingConvEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/CodeEmitterGen.cpp.o CMakeFiles/llvm-tblgen.dir/CodeGenDAGPatterns.cpp.o CMakeFiles/llvm-tblgen.dir/CodeGenInstruction.cpp.o CMakeFiles/llvm-tblgen.dir/CodeGenMapTable.cpp.o CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o CMakeFiles/llvm-tblgen.dir/CodeGenSchedule.cpp.o CMakeFiles/llvm-tblgen.dir/CodeGenTarget.cpp.o CMakeFiles/llvm-tblgen.dir/DAGISelEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/DAGISelMatcherEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/DAGISelMatcherGen.cpp.o CMakeFiles/llvm-tblgen.dir/DAGISelMatcherOpt.cpp.o CMakeFiles/llvm-tblgen.dir/DAGISelMatcher.cpp.o CMakeFiles/llvm-tblgen.dir/DFAPacketizerEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/DisassemblerEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/FastISelEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/FixedLenDecoderEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/InstrInfoEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/IntrinsicEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/OptParserEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/PseudoLoweringEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/RegisterInfoEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/SubtargetEmitter.cpp.o CMakeFiles/llvm-tblgen.dir/TableGen.cpp.o CMakeFiles/llvm-tblgen.dir/X86DisassemblerTables.cpp.o CMakeFiles/llvm-tblgen.dir/X86ModRMFilters.cpp.o CMakeFiles/llvm-tblgen.dir/X86RecognizableInstr.cpp.o CMakeFiles/llvm-tblgen.dir/CTagsEmitter.cpp.o -o ../../bin/llvm-tblgen ../../lib/libLLVMSupport.so.3.7.0svn ../../lib/libLLVMTableGen.so.3.7.0svn -Wl,-rpath,"\$ORIGIN/../lib"  

ld.bfd or gold without -Wl,--gc-sections is fine.

rippels@gcc2-power8 llvm_build % ./bin/llvm-tblgen -gen-intrinsic -I /home/trippels/llvm/include/llvm/IR -I /home/trippels/llvm/lib/Target -I /home/trippels/llvm/include /home/trippels/llvm/include/llvm/IR/Intrinsics.td -o /home/trippels/llvm_build/include/llvm/IR/Intrinsics.gen.tmp

--- /home/trippels/Intrinsics.gen.tmp   2015-02-23 07:34:46.987705642 +0000
+++ /home/trippels/llvm_build/include/llvm/IR/Intrinsics.gen.tmp        2015-02-23 07:37:26.600608412 +0000
@@ -42034,7 +42034,7 @@
       const Attribute::AttrKind Atts[] = {Attribute::NoUnwind,Attribute::ReadNone};
       AS[0] = AttributeSet::get(C, AttributeSet::FunctionIndex, Atts);
       NumAttrs = 1;
-      break;
+        eak;
       }
     case 17: {
       const Attribute::AttrKind AttrParam1[]= {Attribute::NoCapture};
...
   }
-  return Intrinsic::not_intrinsic;
+    retu Intrinsic::not_intrinsic;
 }
 #endif

@@ -70182,7 +70182,7 @@
       break;
     return Intrinsic::arm_mrc2;         // "_MoveFromCoprocessor2"
   }
-}  return Intrinsic::not_intrinsic;
+}    retu Intrinsic::not_intrinsic;
 }
 #endif
Comment 1 Alan Modra 2015-02-24 07:17:26 UTC
We have a bunch of strings in a string merge section
 .section .rodata.str1.8,"aMS",@progbits,1
all nicely aligned to 8 byte boundaries (as you'd expect from the .8 in the name).  Somehow the alignment isn't respected and we end up with misaligned strings.  Since a powerpc64 ld reg+offset form address must be a multiple of 4, we lose low bits of the address.

This is the code reading the "      break;\n" string.  .LC84 happens to be defined at .rodata.str1.8+0x570.

    1d50:       3d 02 00 00     addis   r8,r2,0
                        1d52: R_PPC64_TOC16_HA  .LC84+0x8
    1d54:       80 e8 00 00     lwz     r7,0(r8)
                        1d56: R_PPC64_TOC16_LO  .LC84+0x8
    1d58:       3d 02 00 00     addis   r8,r2,0
                        1d5a: R_PPC64_TOC16_HA  .LC84+0xc
    1d5c:       89 28 00 00     lbz     r9,0(r8)
                        1d5e: R_PPC64_TOC16_LO  .LC84+0xc
    1d60:       3d 02 00 00     addis   r8,r2,0
                        1d62: R_PPC64_TOC16_HA  .rodata.str1.8+0x570
    1d64:       90 ea 00 08     stw     r7,8(r10)
    1d68:       99 2a 00 0c     stb     r9,12(r10)
    1d6c:       e9 28 00 00     ld      r9,0(r8)
                        1d6e: R_PPC64_TOC16_LO_DS       .rodata.str1.8+0x570

This is linked to
    100fdad0:   3d 02 ff fb     addis   r8,r2,-5
    100fdad4:   80 e8 7b 22     lwz     r7,31522(r8)
    100fdad8:   3d 02 ff fb     addis   r8,r2,-5
    100fdadc:   89 28 7b 26     lbz     r9,31526(r8)
    100fdae0:   3d 02 ff fb     addis   r8,r2,-5
    100fdae4:   90 ea 00 08     stw     r7,8(r10)
    100fdae8:   99 2a 00 0c     stb     r9,12(r10)
    100fdaec:   e9 28 7b 18     ld      r9,31512(r8)

For starters, I'll be committing a patch that makes gold complain if LO_DS relocs attempt to apply a misaligned value.
Comment 2 cvs-commit@gcc.gnu.org 2015-02-24 07:52:51 UTC
The master branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ec86f43468e2591127c493d67882de59dbfd79de

commit ec86f43468e2591127c493d67882de59dbfd79de
Author: Alan Modra <amodra@gmail.com>
Date:   Tue Feb 24 18:16:26 2015 +1030

    PowerPC64 GOLD: complain on misaligned _DS relocs
    
    	PR 18010
    	* powerpc.cc (Powerpc_relocate_functions::addr16_ds): Always
    	complain if value is not a multiple of four.
    	(Target_powerpc::Relocate::relocate): Correct handling of
    	R_POWERPC_GOT_TPREL16 and R_POWERPC_GOT_TPREL16_LO for ppc64.
Comment 3 Markus Trippelsdorf 2015-02-24 13:30:15 UTC
Ah, you're right -Wl,-O1,--hash-style=gnu,--gc-sections is fine,
-Wl,-O2,--hash-style=gnu,--gc-sections isn't.
Comment 4 Alan Modra 2015-02-24 13:56:35 UTC
git commit 884151a7 adds a testcase for this gold failure to the bfd ld testsuite.

Expected x86_64 output is
Contents of section .text:
 1000 20100000 10100000 18100000            ...........    
Contents of section .rodata:
 1010 64656667 00000000 30313233 34353637  defg....01234567
 1020 61626364 65666700                    abcdefg.        
[snip]

gold output with -O2 is
Contents of section .text:
 1000 18100000 1b100000 10100000           ............    
Contents of section .rodata:
 1010 30313233 34353637 61626364 65666700  01234567abcdefg.
[snip]
Comment 5 Markus Trippelsdorf 2015-02-25 08:18:25 UTC
Given that the -Wl,-O2 optimization is rather unimportant
,see the comment in stringpool.cc, optimize_ could be simply
left as false for ppc64 targets in that file.
Comment 6 Cary Coutant 2015-02-25 18:01:54 UTC
It seems that optimization of string merge sections is even less useful when the section has an alignment > 1 -- I just don't think it would be worth trying to find tails that just happen to fall on the required alignment boundary. Does it seem reasonable to simply disable optimization for sections with alignment > 1?
Comment 7 Alan Modra 2015-02-25 23:01:54 UTC
> Does it seem reasonable to simply disable optimization for sections with alignment > 1?

Works for me.

I'll note that for aligned strings the most useful optimisation is likely removal of exact duplicates.
Comment 8 Cary Coutant 2015-02-26 00:00:19 UTC
>> Does it seem reasonable to simply disable optimization for sections with alignment > 1?
>
> Works for me.
>
> I'll note that for aligned strings the most useful optimisation is likely
> removal of exact duplicates.

Oh, sorry, I didn't mean to suggest disabling duplicate merging --
just the optimization where we look for strings that are suffixes of
others. I'd argue that your statement is true for unaligned strings as
well. (I'm actually surprised that anyone is actually using -O2!)

-cary
Comment 9 Markus Trippelsdorf 2015-02-26 13:07:08 UTC
(In reply to Cary Coutant from comment #8)
> >> Does it seem reasonable to simply disable optimization for sections with alignment > 1?
> >
> > Works for me.
> >
> > I'll note that for aligned strings the most useful optimisation is likely
> > removal of exact duplicates.
> 
> Oh, sorry, I didn't mean to suggest disabling duplicate merging --
> just the optimization where we look for strings that are suffixes of
> others. I'd argue that your statement is true for unaligned strings as
> well. (I'm actually surprised that anyone is actually using -O2!)

LLVM does, see: cmake/modules/AddLLVM.cmake:161:

156 function(add_link_opts target_name)
157   # Pass -O3 to the linker. This enabled different optimizations on different
158   # linkers.
159   if(NOT (${CMAKE_SYSTEM_NAME} MATCHES "Darwin" OR WIN32))
160     set_property(TARGET ${target_name} APPEND_STRING PROPERTY
161                  LINK_FLAGS " -Wl,-O3")
162   endif()
163
Comment 10 Alan Modra 2015-03-06 04:31:13 UTC
*** Bug 18085 has been marked as a duplicate of this bug. ***
Comment 11 cvs-commit@gcc.gnu.org 2015-03-18 13:23:47 UTC
The binutils-2_25-branch branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=cff59f06b3b54ae3f5b61795c2719d4e28abf5d4

commit cff59f06b3b54ae3f5b61795c2719d4e28abf5d4
Author: Alan Modra <amodra@gmail.com>
Date:   Tue Feb 24 18:16:26 2015 +1030

    PowerPC64 GOLD: complain on misaligned _DS relocs
    
    	PR 18010
    	* powerpc.cc (Powerpc_relocate_functions::addr16_ds): Always
    	complain if value is not a multiple of four.
    	(Target_powerpc::Relocate::relocate): Correct handling of
    	R_POWERPC_GOT_TPREL16 and R_POWERPC_GOT_TPREL16_LO for ppc64.
Comment 12 cvs-commit@gcc.gnu.org 2015-03-22 04:11:02 UTC
The master branch has been updated by Cary Coutant <ccoutant@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1c582fe71858efabae951c5f3ed7dccfb23fb86e

commit 1c582fe71858efabae951c5f3ed7dccfb23fb86e
Author: Cary Coutant <ccoutant@gmail.com>
Date:   Sat Mar 21 21:09:46 2015 -0700

    Fix bug when optimizing string pools of aligned strings.
    
    Tail optimization of string pools (enabled when linker is run with -O2
    or greater) should not be done when the section alignment is greater
    than the size of the characters in the strings; otherwise, unaligned
    strings may result.
    
    gold/
    	PR gold/18010
    	* stringpool.cc (Stringpool_template): Don't optimize if section
    	alignment is greater than sizeof(char).
Comment 13 Cary Coutant 2015-03-22 04:13:21 UTC
Fixed on trunk.