Created attachment 8177 [details] Update section virtual size when it's compressed or decompressed PE binary support for objcopy --compress-debug-sections added in bug #14067 (commit a29a8af8) seems to have a problem when compression makes sections larger. Examining the unstripped XWin.exe for which the problem was reported [1] (using my own PE dumper as objdump -h transparently decompresses the compressed sections for you.) Section Name Virtual Size VMA RawSize File Offset Characteristics .text 0017df74 00001000 0017e000 00000600 60500060 .data 00004184 0017f000 00004200 0017e600 c0700040 .rdata 000321e0 00184000 00032200 00182800 40700040 .buildid 00000035 001b7000 00000200 001b4a00 40300040 /4 .eh_frame 00049240 001b8000 00049400 001b4c00 40300040 .bss 0000eae0 00202000 00000000 00000000 c0700080 .edata 0002a0a5 00211000 0002a200 001fe000 40300040 .idata 00004c70 0023c000 00004e00 00228200 c0300040 .rsrc 00007800 00241000 00007400 0022d000 c0300040 .reloc 0000d1bc 00249000 0000d200 00234800 42300040 /14 .debug_aranges 00002fb0 00257000 00003000 00241a00 42400040 /29 .debug_info 007c423e 0025a000 007c4400 00244a00 42100040 /41 .debug_abbrev 00056487 00a1f000 00056600 00a08e00 42100040 /55 .debug_line 0009576a 00a76000 00095800 00a5f400 42100040 /67 .debug_frame 00000038 00b0c000 00000200 00af4c00 42300040 /80 .debug_str 00026a8b 00b0d000 00026c00 00af4e00 42100040 /91 .debug_loc 001799f0 00b34000 00179a00 00b1ba00 42100040 /102 .debug_ranges 00038b88 00cae000 00038c00 00c95400 42100040 Compare with this after it's compressed using objcopy -compressed-debug-sections Section Name Virtual Size VMA RawSize File Offset Characteristics .text 0017df74 00001000 0017e000 00000600 60500060 .data 00004184 0017f000 00004200 0017e600 c0700040 .rdata 000321e0 00184000 00032200 00182800 40700040 .buildid 00000035 001b7000 00000200 001b4a00 40300040 /4 .eh_frame 00049240 001b8000 00049400 001b4c00 40300040 .bss 0000eae0 00202000 00000000 00000000 c0700080 .edata 0002a0a5 00211000 0002a200 001fe000 40300040 .idata 00004c70 0023c000 00004e00 00228200 c0300040 .rsrc 00007800 00241000 00007400 0022d000 c0300040 .reloc 0000d1bc 00249000 0000d200 00234400 42300040 /14 .zdebug_aranges 00002fb0 00257000 00001200 00241600 42400040 /30 .zdebug_info 007c423e 0025a000 00356200 00242800 42100040 /43 .zdebug_abbrev 00056487 00a1f000 0000d600 00598a00 42100040 /58 .zdebug_line 0009576a 00a76000 00040600 005a6000 42100040 /71 .zdebug_frame 00000038 00b0c000 00000200 005e6600 42300040 /85 .zdebug_str 00026a8b 00b0d000 00005c00 005e6800 42100040 /97 .zdebug_loc 001799f0 00b34000 0007de00 005ec400 42100040 /109 .zdebug_ranges 00038b88 00cae000 00013000 0066a200 42100040 It can be seen that the virtual size of the compressed sections is not updated, although the raw size has decreased. Normally this is not a problem, as nothing is accessing the section contents after the raw size. However, if the compressor has made the data bigger, it is truncated to the virtual size and decompression fails. Unfortunately, small .debug_frame sections seem to be quite normal on x86. Attached is a patch which updates the virtual size, which seems to fix this issue. Section Name Virtual Size VMA RawSize File Offset Characteristics .text 0017df74 00001000 0017e000 00000600 60500060 .data 00004184 0017f000 00004200 0017e600 c0700040 .rdata 000321e0 00184000 00032200 00182800 40700040 .buildid 00000035 001b7000 00000200 001b4a00 40300040 /4 .eh_frame 00049240 001b8000 00049400 001b4c00 40300040 .bss 0000eae0 00202000 00000000 00000000 c0700080 .edata 0002a0a5 00211000 0002a200 001fe000 40300040 .idata 00004c70 0023c000 00004e00 00228200 c0300040 .rsrc 00007800 00241000 00007400 0022d000 c0300040 .reloc 0000d1bc 00249000 0000d200 00234400 42300040 /14 .zdebug_aranges 0000103e 00257000 00001200 00241600 42400040 /30 .zdebug_info 003561b2 0025a000 00356200 00242800 42100040 /43 .zdebug_abbrev 0000d5aa 00a1f000 0000d600 00598a00 42100040 /58 .zdebug_line 0004049b 00a76000 00040600 005a6000 42100040 /71 .zdebug_frame 00000045 00b0c000 00000200 005e6600 42300040 /85 .zdebug_str 00005b42 00b0d000 00005c00 005e6800 42100040 /97 .zdebug_loc 0007dc89 00b34000 0007de00 005ec400 42100040 /109 .zdebug_ranges 00012e79 00cae000 00013000 0066a200 42100040 Possibly related to bug #15350 [1] https://cygwin.com/ml/cygwin/2015-03/msg00080.html
Yes, it is likely that we have here an issue in updating section size, if compression actually increases, instead of decreasing size. IMO the saner approach would be to reject compression, if content gets larger as before
(In reply to Kai Tietz from comment #1) > Yes, it is likely that we have here an issue in updating section size, if > compression actually increases, instead of decreasing size. > > IMO the saner approach would be to reject compression, if content gets > larger as before I think my patch is no good because an output PE which is unstripped but has compressed debug sections is not valid, because it breaks the "sections must have contiguous VMAs" constraint. So not compressing if the section gets larger sounds like a better approach. I'll have a go at implementing that.
Created attachment 8178 [details] Don't compress section if it doesn't get smaller
(In reply to Jon TURNEY from comment #2) > So not compressing if the section gets larger sounds like a better approach. > I'll have a go at implementing that. This turns out to be not quite so simple due to the bfd interface for dealing with compressed sections, but an attempt is attached. With this patch, objcopy --compress-debug-sections on the same test binary produces: Name VirtSize VMA RawSize Offset Flags .text 0017df74 00001000 0017e000 00000600 60500060 .data 00004184 0017f000 00004200 0017e600 c0700040 .rdata 000321e0 00184000 00032200 00182800 40700040 .buildid 00000035 001b7000 00000200 001b4a00 40300040 /4 .eh_frame 00049240 001b8000 00049400 001b4c00 40300040 .bss 0000eae0 00202000 00000000 00000000 c0700080 .edata 0002a0a5 00211000 0002a200 001fe000 40300040 .idata 00004c70 0023c000 00004e00 00228200 c0300040 .rsrc 00007800 00241000 00007400 0022d000 c0300040 .reloc 0000d1bc 00249000 0000d200 00234400 42300040 /14 .zdebug_aranges 00002fb0 00257000 00001200 00241600 42400040 /30 .zdebug_info 007c423e 0025a000 00356200 00242800 42100040 /43 .zdebug_abbrev 00056487 00a1f000 0000d600 00598a00 42100040 /58 .zdebug_line 0009576a 00a76000 00040600 005a6000 42100040 /71 .debug_frame 00000038 00b0c000 00000200 005e6600 42300040 /84 .zdebug_str 00026a8b 00b0d000 00005c00 005e6800 42100040 /96 .zdebug_loc 001799f0 00b34000 0007de00 005ec400 42100040 /108 .zdebug_ranges 00038b88 00cae000 00013000 0066a200 42100040 Note that the .debug_frame section didn't get compressed. The unstripped output of objcopy can also successfully be executed.
Created attachment 8192 [details] Extended version of Jon's patch Hi Jon, Your patch is good, but it misses a couple of points: * A change similar to the one in coffgen.c is needed in elf.c for ELF based targets. * The assembler needs to be updated as well so that it's --compress-debug-sections option only compresses when it is effective. * The documentation needs to be updated to explicitly mention that compression only happens when it is worthwhile. I have uploaded an extended version of your patch which should handle these points. Please could you try it out and let me know if it works for you. Cheers Nick
(In reply to Nick Clifton from comment #5) Thanks for taking a look. > Your patch is good, but it misses a couple of points: > > * A change similar to the one in coffgen.c is needed in elf.c for ELF > based targets. I thought I had done that. > * The assembler needs to be updated as well so that it's > --compress-debug-sections option only compresses when it is effective. > > * The documentation needs to be updated to explicitly mention that > compression only happens when it is worthwhile. > > > I have uploaded an extended version of your patch which should handle these > points. Please could you try it out and let me know if it works for you. This seems to work fine.
(In reply to Nick Clifton from comment #5) > > I have uploaded an extended version of your patch which should handle these > points. Please could you try it out and let me know if it works for you. > The testcase is missing in the patch.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=273a49858fa9c8d73de87167618ef99d70f9731a commit 273a49858fa9c8d73de87167618ef99d70f9731a Author: Jon Turney <jon.turney@dronecode.org.uk> Date: Wed Mar 18 15:47:13 2015 +0000 Fix debug section compression so that it is only performed if it would make the section smaller. PR binutils/18087 gas * doc/as.texinfo: Note that when gas compresses debug sections the compression is only performed if it makes the section smaller. * write.c (compress_debug): Do not compress a debug section if doing so would make it larger. tests * gas/i386/dw2-compress-1.d: Do not expect the .debug_abbrev or .debug_info sections to be compressed. binu * doc/binutils.texi: Note that when objcopy compresses debug sections the compression is only performed if it makes the section smaller. bfd * coffgen.c (make_a_section_from_file): Only prepend a z to a debug section's name if the section was actually compressed. * elf.c (_bfd_elf_make_section_from_shdr): Likewise. * compress.c (bfd_init_section_compress_status): Do not compress the section if doing so would make it bigger. In such cases leave the section alone and return COMPRESS_SECTION_NONE.
Patch committed, with an addition of a fix to the gas testsuite for the x86 targets as pointed out by H.J.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d94077e27d279c4ff0ee26bad786f89c350e2aea commit d94077e27d279c4ff0ee26bad786f89c350e2aea Author: H.J. Lu <hjl.tools@gmail.com> Date: Wed Mar 18 09:20:38 2015 -0700 Add a testcase for PR gas/18087 PR gas/18087 * gas/i386/dw2-compress-1.d: Revert the last change. * gas/i386/dw2-compress-3.d: New. * gas/i386/dw2-compress-3.s: Likewise. * gas/i386/i386.exp: Run dw2-compress-3 for ELF targets.
(In reply to Nick Clifton from comment #9) > Patch committed, with an addition of a fix to the gas testsuite for the x86 > targets as pointed out by H.J. Thanks.
The master branch has been updated by Nick Clifton <nickc@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e2575e05e73c3b2f08a8b5f579a504ac6a45ad60 commit e2575e05e73c3b2f08a8b5f579a504ac6a45ad60 Author: Nick Clifton <nickc@redhat.com> Date: Thu Mar 19 12:14:56 2015 +0000 Fix building and testing dwarf debug section compression feature when zlib is not available. PR gas/18087 gas/test * gas/i386/dw2-compress-1.d: Allow the test to pass regardless of whether the .debug_info section was compressed on not. bfd * compress.c (bfd_compress_section_contents): Do not define this function if it is not used.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=8d00121477371cfd1596118af062fe6ff4e263b7 commit 8d00121477371cfd1596118af062fe6ff4e263b7 Author: H.J. Lu <hjl.tools@gmail.com> Date: Tue Mar 24 13:27:52 2015 -0700 Don't write the zlib header if not used No need to write the zlib header if compression didn't make the section smaller. PR binutils/18087 * compress.c (bfd_compress_section_contents): Don't write the zlib header and set contents as well as compress_status if compression didn't make the section smaller. (bfd_init_section_compress_status): Don't check compression size here.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b422eb499be2858969fb7723b4e4e08cab20fcdc commit b422eb499be2858969fb7723b4e4e08cab20fcdc Author: H.J. Lu <hjl.tools@gmail.com> Date: Tue Mar 24 19:06:22 2015 -0700 Don't write the zlib header if not used No need to write the zlib header if compression didn't make the section smaller. PR gas/18087 * write.c (compress_debug): Don't write the zlib header if compression didn't make the section smaller.
I believe this is resolved.