[PATCH] Use offsets instead of addresses in ELF_SECTION_IN_SEGMENT

Alan Hayward Alan.Hayward@arm.com
Wed May 23 09:53:00 GMT 2018


(swapping original email order around)
Thanks for the review!

> On 22 May 2018, at 15:04, Alan Modra <amodra@gmail.com> wrote:
> 
>> diff --git a/include/elf/internal.h b/include/elf/internal.h
>> index 05f9fab89c..5d93e0070e 100644
>> --- a/include/elf/internal.h
>> +++ b/include/elf/internal.h
>> @@ -347,9 +347,9 @@ struct elf_segment_map
>>        || ((sec_hdr)->sh_flags & SHF_ALLOC) == 0			\
>>        || ((sec_hdr)->sh_addr >= (segment)->p_vaddr			\
>> 	   && (!(strict)						\
>> -	       || ((sec_hdr)->sh_addr - (segment)->p_vaddr		\
>> +	       || ((sec_hdr)->sh_offset - (segment)->p_offset		\
>> 		   <= (segment)->p_memsz - 1))				\
>> -	   && (((sec_hdr)->sh_addr - (segment)->p_vaddr			\
>> +	   && (((sec_hdr)->sh_offset - (segment)->p_offset		\
>> 		+ ELF_SECTION_SIZE(sec_hdr, segment))			\
>> 	       <= (segment)->p_memsz)))					\
>>    /* No zero size sections at start or end of PT_DYNAMIC.  */		\
> 
> We test sh_offset already, just above the code you are patching.  So I
> think this VMA test should continue to test sh_addr.
> 
> Now there are reasons you might want to disable the VMA checks
> entirely, for example when overlays are present.  To do that, use
> ELF_SECTION_IN_SEGMENT_1 with check_vma false.  See the example in
> elf.c assign_files_positions_for_load_sections.
> 

Hmm, yes I somehow hadn’t spotted that.

My issue with ELF_SECTION_IN_SEGMENT was it’s use in
_bfd_elf_make_section_from_shdr(). If I remove my patch and replace it
with the following then everything “works” for me:

diff --git a/bfd/elf.c b/bfd/elf.c
index 6c66bbca02..1cf3131b2a 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -1140,7 +1140,7 @@ _bfd_elf_make_section_from_shdr (bfd *abfd,
          if (((phdr->p_type == PT_LOAD
                && (hdr->sh_flags & SHF_TLS) == 0)
               || phdr->p_type == PT_TLS)
-             && ELF_SECTION_IN_SEGMENT (hdr, phdr))
+             && ELF_SECTION_IN_SEGMENT_1 (hdr, phdr, 0, 0))
            {
              if ((flags & SEC_LOAD) == 0)
                newsect->lma = (phdr->p_paddr

Whilst this logically gives the same result as my patch, it feels
less like the correct solution.


> On Tue, May 22, 2018 at 11:52:15AM +0100, Alan Hayward wrote:
>> Use sh_offset and p_offset instead of sh_addr and p_vaddr when calculating if a section fits in a segment. Both methods are valid when using the GNU linker.
>> 
>> This change makes ELF_SECTION_IN_SEGMENT consistent with BFD _bfd_elf_make_section_from_shdr(), which has used offsets to calculate LMA since 2008.
> 
> LMA is not VMA.  The code you referenced doesn't really support the
> change you propose, nor did you say exactly why the change is needed.


I was hoping to not need to go into too much detail (my original version
of the patch email did have a description of the Armlinker, which I removed
due to it being a large off putting info dump).

The Armlinker in Arm Compiler uses a feature of scatter-loading, in the
sense that the linker has generated an image whose first task on startup is
to copy pieces of itself from their load addresses to their execution
addresses. Specifically, the entire image is packed into a single contiguous
blob running from addresses 0x0 – 0x49d4 (you imagine this being the target
embedded device's ROM / PROM / Flash / whatever), and the first thing that
happens is that the function __scatterload copies part of that range to
0x20000000 (the device's actual RAM, where the RW data will live during the
program's run time) and also zeroes out another range in the 0x20000000 area
(the bss / ZI data section).

(Other scatter loading features include data compression: in some situations
the version of a section's data packed into the initial ROM image will not even
match its runtime contents byte for byte, because instead it will be compressed,
and the scatter loading code will call a decompressor instead of a memcpy-type
function.)
 	
In images of this type, armlink's policy – since more or less forever – has
been to use section addresses (i.e. sh_addr in the section header table entries)
to indicate where a given section is expected to end up after the initial scatter
loading pass has run; and the segment addresses (p_vaddr and p_paddr – armlink
draws no distinction between the two, and I think in fact it always sets them
identically) indicate where and how the image expects to have been installed in
memory at the point where it starts running, i.e. before scatter loading. So in
this kind of case where the image starts off packed into a single chunk of ROM
and scatters itself out into RAM at the beginning of its run, the segment
addresses will describe the starting chunk of ROM, and the section addresses
show where everything ends up at the point where the interesting part of the
runtime begins.

The idea is that this gives a debugger everything it needs to know to both load
the image into memory in the first place (you use the segment addresses for that),
and to map memory addresses back to code / data / symbols during the main
runtime of the program (you use the section addresses, on the assumption that by
the time you're concerned with an address in any given section, that section has
already gone through scatter loading).
	 	
For example, here is the readelf output of a small binary I compiled. Note the
address for ER_DATA with PROGBITS. In the existing code this will use sh_addr,
incorrectly loading it to 0xead4. What we want instead is to use (p_vaddr +
sh_offset - p_offset), which is (0x8000 + 0x2b08 - 0x34), which is 0xaad4.


Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] ER_CODE           PROGBITS        00008000 000034 002ad4 00  AX  0   0  4
  [ 2] ER_DATA           PROGBITS        0000ead4 002b08 000020 00  WA  0   0  4
  [ 3] ER_DATA           NOBITS          0000eaf4 002b28 00015c 00  WA  0   0  4
  [ 4] ARM_LIB_STACKHEAP NOBITS          00040000 002b28 001000 00  WA  0   0  1
  [ 5] .debug_abbrev     PROGBITS        00000000 002b28 000065 00      0   0  1
  [ 6] .debug_frame      PROGBITS        00000000 002b8d 000d9c 00      0   0  1
  [ 7] .debug_info       PROGBITS        00000000 003929 00006e 00      0   0  1
  [ 8] .debug_line       PROGBITS        00000000 003997 00003c 00      0   0  1
  [ 9] .debug_pubnames   PROGBITS        00000000 0039d3 000029 00      0   0  1
  [10] .debug_pubtypes   PROGBITS        00000000 0039fc 000023 00      0   0  1
  [11] .debug_str        PROGBITS        00000000 003a1f 00008b 00      0   0  1
  [12] .symtab           SYMTAB          00000000 003aac 002d60 10     13 498  4
  [13] .strtab           STRTAB          00000000 00680c 001f54 00      0   0  1
  [14] .note             NOTE            00000000 008760 000020 00      0   0  4
  [15] .comment          PROGBITS        00000000 008780 0003e0 00      0   0  1
  [16] .shstrtab         STRTAB          00000000 008b60 0000ac 00      0   0  1

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000034 0x00008000 0x00008000 0x02af4 0x03c50 RWE 0x4


All of this is still within the elf spec, but clearly different from the GNU
linker - the idea was to get a patch that allowed us the safely work for both
without any horrible “if gnu ... else if armlinker ...” code.


Alan.



More information about the Binutils mailing list