This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Use ELF_SECTION_IN_SEGMENT to map segments
- From: Alan Hayward <Alan dot Hayward at arm dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, nd <nd at arm dot com>
- Date: Wed, 23 May 2018 10:37:00 +0000
- Subject: Re: [PATCH] Use ELF_SECTION_IN_SEGMENT to map segments
- Nodisclaimer: True
- References: <20180522110019.3839-1-alan.hayward@arm.com> <03e86682-9850-ea5d-1e4e-2bb897a55a7f@redhat.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
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.