The Binutils "simple objcopy of executable" test is failing for msp430-elf/-mlarge since commit 30fe183248b2523ecff9da36853e2f893c4c4b91 Author: Alan Modra <amodra@gmail.com> Date: Wed Oct 23 17:40:51 2019 +1030 PR4499, assign file positions assumes segment offsets increasing This is similar to PR23595, where the fact that the first segment contains the program headers and a section of type SHT_NOBITS causes problems. This time, the difference between the original executable file, and the output from copying that file using objcopy is the sh_offset of the .bss section, which is the only section in the first segment, and also contains the program headers. Original executable: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 5] .data PROGBITS 00000500 0001b0 00013e 00 WA 0 0 2 [ 7] .bss NOBITS 0000063e 000116 000028 00 WA 0 0 2 ... Program Headers: Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align LOAD 0x000000 0x00000528 0x00000528 0x00114 0x0013e RW 0x4 ... Section to Segment mapping: Segment Sections... 00 .bss Objcopy output executable: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al [ 5] .data PROGBITS 00000500 0001b0 00013e 00 WA 0 0 2 [ 7] .bss NOBITS 0000063e 000114 000028 00 WA 0 0 2 Note 0x116 sh_offset in the original, and 0x114 sh_offset in the objcopy output. The problem is that copy_elf_program_header calculates a p_vaddr_offset of -2 for the segment, since .bss is offset 2 bytes from the end of the program headers in that segment, due to the required alignment of the segment. So p_vaddr is temporarily being calculated as 0x63c, which means in assign_file_positions_for_load_sections segment aligment is not required since (0x63c + 0x114) % 4 == 0. Hence the .bss sh_offset is output incorrectly - (0x63e + 0x114) % 4 = 2. Since .bss is the first loadable section in the segment, it must be aligned to the segment align of 4 rather than the section alignment of 2, right?
Created attachment 12368 [details] patch The attached patch fixes the issue, although I haven't been able to totally convince myself of the reasoning behind the change. In this situation, assign_file_positions_for_load_sections will align the first section in the segment to the required segment alignment, but then to avoid wasting space, the offset of the next segment will be adjusted back to the end of the no_contents segment, even if that offset overlaps the SHT_NOBITS section. So in a similar manner we can disregard any "padding" between the start of the no_contents segment and the first SHT_NOBITS section, by keeping p_vaddr_offset set to 0. The gap between the segment and section start is not used as padding in the normal sense to adjust the address of later sections and segments, it is just there to align that first SHT_NOBITS section. If that sounds about right I'll post the patch to the mailing list for approval.
Please attach the original executable. I'd like to see all the program headers.
Created attachment 12370 [details] original executable Attached the original executable file.
Created attachment 12403 [details] Alternate patch This also fixes the testcase. I'm inclined to go this way since it's really the lack of sh_offset adjustment for nobits sections that is breaking things for you. What do you think?
(In reply to Alan Modra from comment #4) > Created attachment 12403 [details] > Alternate patch > > This also fixes the testcase. I'm inclined to go this way since it's really > the lack of sh_offset adjustment for nobits sections that is breaking things > for you. What do you think? Looks good to me, I prefer the reasoning behind your changes to what I originally suggested as well. I successfully regtested your patch for msp430-elf in the -msmall and -mlarge configurations. Thanks!
The master branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d16e3d2e5b717cf83c1fe47a70e0a9f2c2024a44 commit d16e3d2e5b717cf83c1fe47a70e0a9f2c2024a44 Author: Alan Modra <amodra@gmail.com> Date: Tue Mar 24 10:42:45 2020 +1030 PR25662, invalid sh_offset for first section in segment with phdrs PR 25662 * elf.c (assign_file_positions_for_load_sections): Adjust offset for SHT_NOBITS section if first in segment.
Updated patch applied. Jozef, perhaps you could add a testcase to ensure this doesn't regress. Ideally one that doesn't require a C compiler.
Created attachment 12404 [details] testcase (In reply to Alan Modra from comment #7) > Updated patch applied. Jozef, perhaps you could add a testcase to ensure > this doesn't regress. Ideally one that doesn't require a C compiler. How about the attached patch? I tested that it fails on x86_64-pc-linux-gnu, msp430-elf and arm-eabi before your patch, and passes with the patch applied.
The testcase patch looks mostly good, thanks. At first I thought you had the remote host file names wrong, but it doesn't seem too important what file names are used on the host. Some other issues: - The new test is using an ELF .type directive. Either remove that or make the test ELF only. - The test will error if ld isn't built, as is the case for a few targets, ia64-vms being an example. Please post to the mailing list for any further discussion.
(In reply to Alan Modra from comment #9) > The testcase patch looks mostly good, thanks. At first I thought you had > the remote host file names wrong, but it doesn't seem too important what > file names are used on the host. > > Some other issues: > - The new test is using an ELF .type directive. Either remove that or make > the test ELF only. > - The test will error if ld isn't built, as is the case for a few targets, > ia64-vms being an example. > > Please post to the mailing list for any further discussion. I posted an updated patch here https://sourceware.org/pipermail/binutils/2020-March/110408.html. BTW, the test fails for i386-pe, but as expected it's not fixed by your patch to elf.c. A quick comparison of objdump -x output shows that the "Time/Date" field has been reset to epoch 0.
The master branch has been updated by Jozef Lawrynowicz <jozefl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=1fafefd59438f92206e65ea6b2022d82c5ccfe12 commit 1fafefd59438f92206e65ea6b2022d82c5ccfe12 Author: Jozef Lawrynowicz <jozef.l@mittosystems.com> Date: Fri Mar 27 10:54:26 2020 +0000 Add testcase for PR 25662 invalid sh_offset for section binutils/ChangeLog: 2020-03-27 Jozef Lawrynowicz <jozef.l@mittosystems.com> PR binutils/25662 * testsuite/binutils-all/objcopy.exp (objcopy_test): Add argument to specify whether an object file or executable should be built and tested. Change test names to report whether an object file or executable is being tested. * testsuite/binutils-all/pr25662.ld: New test. * testsuite/binutils-all/pr25662.s: New test.
The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=790147a9e9ee05542c621a36669288413880c876 commit 790147a9e9ee05542c621a36669288413880c876 Author: H.J. Lu <hjl.tools@gmail.com> Date: Tue Sep 8 10:01:45 2020 -0700 Pass --disable-reloc-section on PE targets for PR 25662 test Pass --disable-reloc-section on PE targets for PR 25662 test since commit 514b4e191d5f46de8e142fe216e677a35fa9c4bb Author: Jeremy Drake <sourceware-bugzilla@jdrake.com> Date: Thu Aug 27 12:58:27 2020 +0100 Change the default characteristics of DLLs built by the linker to more secure settings. defaulted to --enable-reloc-section. PR ld/26587 * testsuite/binutils-all/objcopy.exp: Pass --disable-reloc-section to ld on PE targets for PR 25662 test.