diff mbox

Use ELF_SECTION_IN_SEGMENT to map segments

Message ID 20180522110019.3839-1-alan.hayward@arm.com
State New, archived
Headers

Commit Message

Alan Hayward May 22, 2018, 11 a.m. UTC 
  The macro ELF_SECTION_IN_SEGMENT should be used when calculating if
a section maps to a segment.

The binutils patch "[PATCH] Use offsets instead of addresses in
ELF_SECTION_IN_SEGMENT" makes further improvements to the macro.
When the two patches are combined, this will allow GDB to be used
to debug Arm baremetal binaries produced by the Arm Compiler. See
the binutils patch for further details.

Regardless of Arm debugging, this patch reduces code/logic duplication
in gdb.

This change has been tested using make check x86 on a binutils+gdb
build. It has also been manually tested by using gdb to debug a Arm
Mbed board.

Alan.

2018-05-22  Alan Hayward  <alan.hayward@arm.com>

	* elfread.c (elf_symfile_segments): Use ELF_SECTION_IN_SEGMENT.
---
 gdb/elfread.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Pedro Alves May 22, 2018, 1:46 p.m. UTC | #1 
On 05/22/2018 12:00 PM, Alan Hayward wrote:
> The macro ELF_SECTION_IN_SEGMENT should be used when calculating if
> a section maps to a segment.
> 
> The binutils patch "[PATCH] Use offsets instead of addresses in
> ELF_SECTION_IN_SEGMENT" makes further improvements to the macro.
> When the two patches are combined, this will allow GDB to be used
> to debug Arm baremetal binaries produced by the Arm Compiler. See
> the binutils patch for further details.
> 
> Regardless of Arm debugging, this patch reduces code/logic duplication
> in gdb.

Sounds reasonable.

> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -120,17 +120,13 @@ elf_symfile_segments (bfd *abfd)
>    for (i = 0, sect = abfd->sections; sect != NULL; i++, sect = sect->next)
>      {
>        int j;
> -      CORE_ADDR vma;
> +      Elf_Internal_Shdr *this_hdr = &(elf_section_data (sect)->this_hdr);

Drop unnecessary parens.  

You could defer this call until after the SEC_ALLOC check.

>  
>        if ((bfd_get_section_flags (abfd, sect) & SEC_ALLOC) == 0)
>  	continue;
>  
> -      vma = bfd_get_section_vma (abfd, sect);
> -
>        for (j = 0; j < num_segments; j++)
> -	if (segments[j]->p_memsz > 0
> -	    && vma >= segments[j]->p_vaddr
> -	    && (vma - segments[j]->p_vaddr) < segments[j]->p_memsz)
> +	if ELF_SECTION_IN_SEGMENT (this_hdr, segments[j])

That is some odd-looking C/C++ code, for assuming the macro's
internals are wrapped in parens.  Please write the more usual:

	if (ELF_SECTION_IN_SEGMENT (this_hdr, segments[j]))

OK with those fixed.

Thanks,
Pedro Alves
Alan Hayward May 23, 2018, 10:37 a.m. UTC | #2 
Thanks for the review.

> On 22 May 2018, at 14:46, Pedro Alves <palves@redhat.com> wrote:

> 

> On 05/22/2018 12:00 PM, Alan Hayward wrote:

>> The macro ELF_SECTION_IN_SEGMENT should be used when calculating if

>> a section maps to a segment.

>> 

>> The binutils patch "[PATCH] Use offsets instead of addresses in

>> ELF_SECTION_IN_SEGMENT" makes further improvements to the macro.

>> When the two patches are combined, this will allow GDB to be used

>> to debug Arm baremetal binaries produced by the Arm Compiler. See

>> the binutils patch for further details.

>> 

>> Regardless of Arm debugging, this patch reduces code/logic duplication

>> in gdb.

> 

> Sounds reasonable.

> 

>> --- a/gdb/elfread.c

>> +++ b/gdb/elfread.c

>> @@ -120,17 +120,13 @@ elf_symfile_segments (bfd *abfd)

>>   for (i = 0, sect = abfd->sections; sect != NULL; i++, sect = sect->next)

>>     {

>>       int j;

>> -      CORE_ADDR vma;

>> +      Elf_Internal_Shdr *this_hdr = &(elf_section_data (sect)->this_hdr);

> 

> Drop unnecessary parens.  

> 

> You could defer this call until after the SEC_ALLOC check.


Ok.

> 

>> 

>>       if ((bfd_get_section_flags (abfd, sect) & SEC_ALLOC) == 0)

>> 	continue;

>> 

>> -      vma = bfd_get_section_vma (abfd, sect);

>> -

>>       for (j = 0; j < num_segments; j++)

>> -	if (segments[j]->p_memsz > 0

>> -	    && vma >= segments[j]->p_vaddr

>> -	    && (vma - segments[j]->p_vaddr) < segments[j]->p_memsz)

>> +	if ELF_SECTION_IN_SEGMENT (this_hdr, segments[j])

> 

> That is some odd-looking C/C++ code, for assuming the macro's

> internals are wrapped in parens.  Please write the more usual:

> 

> 	if (ELF_SECTION_IN_SEGMENT (this_hdr, segments[j]))


Not quite sure why I didn’t spot that when writing it.


> 

> OK with those fixed.

> 


I’ll wait until the BFD patch has been approved before I push, as there
is a small chance that review will cause me to make further changes. Will
post again if I do.


Alan.
Alan Hayward June 4, 2018, 4:30 p.m. UTC | #3 
Pushed this with changes as suggested.

The BFD patch hasn’t progressed yet (possible extra changes needed
elsewhere). Regardless, this patch can stand on it’s own.

Alan.

> On 23 May 2018, at 11:36, Alan Hayward <Alan.Hayward@arm.com> wrote:

> 

> 

> Thanks for the review.

> 

>> On 22 May 2018, at 14:46, Pedro Alves <palves@redhat.com> wrote:

>> 

>> On 05/22/2018 12:00 PM, Alan Hayward wrote:

>>> The macro ELF_SECTION_IN_SEGMENT should be used when calculating if

>>> a section maps to a segment.

>>> 

>>> The binutils patch "[PATCH] Use offsets instead of addresses in

>>> ELF_SECTION_IN_SEGMENT" makes further improvements to the macro.

>>> When the two patches are combined, this will allow GDB to be used

>>> to debug Arm baremetal binaries produced by the Arm Compiler. See

>>> the binutils patch for further details.

>>> 

>>> Regardless of Arm debugging, this patch reduces code/logic duplication

>>> in gdb.

>> 

>> Sounds reasonable.

>> 

>>> --- a/gdb/elfread.c

>>> +++ b/gdb/elfread.c

>>> @@ -120,17 +120,13 @@ elf_symfile_segments (bfd *abfd)

>>>  for (i = 0, sect = abfd->sections; sect != NULL; i++, sect = sect->next)

>>>    {

>>>      int j;

>>> -      CORE_ADDR vma;

>>> +      Elf_Internal_Shdr *this_hdr = &(elf_section_data (sect)->this_hdr);

>> 

>> Drop unnecessary parens.  

>> 

>> You could defer this call until after the SEC_ALLOC check.

> 

> Ok.

> 

>> 

>>> 

>>>      if ((bfd_get_section_flags (abfd, sect) & SEC_ALLOC) == 0)

>>> 	continue;

>>> 

>>> -      vma = bfd_get_section_vma (abfd, sect);

>>> -

>>>      for (j = 0; j < num_segments; j++)

>>> -	if (segments[j]->p_memsz > 0

>>> -	    && vma >= segments[j]->p_vaddr

>>> -	    && (vma - segments[j]->p_vaddr) < segments[j]->p_memsz)

>>> +	if ELF_SECTION_IN_SEGMENT (this_hdr, segments[j])

>> 

>> That is some odd-looking C/C++ code, for assuming the macro's

>> internals are wrapped in parens.  Please write the more usual:

>> 

>> 	if (ELF_SECTION_IN_SEGMENT (this_hdr, segments[j]))

> 

> Not quite sure why I didn’t spot that when writing it.

> 

> 

>> 

>> OK with those fixed.

>> 

> 

> I’ll wait until the BFD patch has been approved before I push, as there

> is a small chance that review will cause me to make further changes. Will

> post again if I do.

> 

> 

> Alan.
diff mbox

Patch

diff --git a/gdb/elfread.c b/gdb/elfread.c
index b4b4a1b24c..3cf9b1ec21 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -120,17 +120,13 @@  elf_symfile_segments (bfd *abfd)
   for (i = 0, sect = abfd->sections; sect != NULL; i++, sect = sect->next)
     {
       int j;
-      CORE_ADDR vma;
+      Elf_Internal_Shdr *this_hdr = &(elf_section_data (sect)->this_hdr);
 
       if ((bfd_get_section_flags (abfd, sect) & SEC_ALLOC) == 0)
 	continue;
 
-      vma = bfd_get_section_vma (abfd, sect);
-
       for (j = 0; j < num_segments; j++)
-	if (segments[j]->p_memsz > 0
-	    && vma >= segments[j]->p_vaddr
-	    && (vma - segments[j]->p_vaddr) < segments[j]->p_memsz)
+	if ELF_SECTION_IN_SEGMENT (this_hdr, segments[j])
 	  {
 	    data->segment_info[i] = j + 1;
 	    break;