This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] PowerPC VLE changes
- From: Alan Modra <amodra at gmail dot com>
- To: Alexander Fedotov <alfedotov at gmail dot com>
- Cc: binutils at sourceware dot org, Edmar Wienskoski <edmarwjr at gmail dot com>
- Date: Sat, 24 Jun 2017 22:41:37 +0930
- Subject: Re: [PATCH] PowerPC VLE changes
- Authentication-results: sourceware.org; auth=none
- References: <CAN8C2CpJWr+-LM57a44id3ciw0rsa9meXkE8hAZSzAA1Hd95UQ@mail.gmail.com>
On Fri, Jun 23, 2017 at 11:51:57PM +0300, Alexander Fedotov wrote:
> Hello Alan
>
> We want to upstream our changes for VLE, LSP, SPE2 and other stuff.
> All of them are based on 2.28 release.
Please rebase against current master, and provide Changelog entries.
> - if (symaddr - reladdr + max_branch_offset
> - < 2 * max_branch_offset)
> - continue;
> +
> + /* I don't trust the relocation check using ' ... < (2 * max_branch_offset)'
I do trust it. This change is silly.
> - stub_rtype = R_PPC_RELAX;
> + stub_rtype = (target_stub_type == stub_entry_type_vle) ? R_PPC_VLE_RELAX : R_PPC_RELAX;
Overlong lines. Wrap to 80 chars or less.
> @@ -7385,10 +7503,21 @@
> case R_PPC_REL24:
> case R_PPC_LOCAL24PC:
> case R_PPC_PLTREL24:
> - t0 = bfd_get_32 (abfd, hit_addr);
> - t0 &= ~0x3fffffc;
> - t0 |= val & 0x3fffffc;
> - bfd_put_32 (abfd, t0, hit_addr);
> + if (r_type == R_PPC_PLTREL24
> + && (elf_section_flags (isec) & SHF_PPC_VLE) != 0)
Something is fishy here. Why is PLTREL24 treated differently when
VLE? You haven't changed the insn here!
> @@ -9063,8 +9214,8 @@
> }
> else
> {
> - stub = stub_entry;
> - size = ARRAY_SIZE (stub_entry);
> + stub = (r_type == R_PPC_VLE_RELAX) ? stub_entry_vle : stub_entry;
> + size = ARRAY_SIZE (stub_entry); /* stub_entry and stub_entry_vle must be same size */
Overlong line again. An assert would be better than a comment.
> --- binutils-2.28/binutils/objdump.c 2017-03-02 11:23:53.000000000 +0300
> +++ binutils-2.28-vle/binutils/objdump.c 2017-06-23 17:25:21.641299056 +0300
> @@ -481,6 +481,10 @@
> PF (SEC_NEVER_LOAD, "NEVER_LOAD");
> PF (SEC_EXCLUDE, "EXCLUDE");
> PF (SEC_SORT_ENTRIES, "SORT_ENTRIES");
> + if (bfd_get_arch(abfd) == bfd_arch_powerpc || bfd_get_arch (abfd) == bfd_mach_ppc_vle)
> + {
> + PF (SEC_TIC54X_BLOCK, "VLE"); /* hack, would have to include ppc.h */
> + }
Ick. Don't do this, define a flag in bfd/section.c.
> diff -ruN binutils-2.28/include/elf/ppc.h binutils-2.28-vle/include/elf/ppc.h
> --- binutils-2.28/include/elf/ppc.h 2017-03-02 11:23:54.000000000 +0300
> +++ binutils-2.28-vle/include/elf/ppc.h 2017-06-23 17:34:20.838930833 +0300
> @@ -79,8 +79,10 @@
> RELOC_NUMBER (R_PPC_RELAX, 48)
> RELOC_NUMBER (R_PPC_RELAX_PLT, 49)
> RELOC_NUMBER (R_PPC_RELAX_PLTREL24, 50)
> + RELOC_NUMBER (R_PPC_VLE_RELAX, 51)
> /* Reloc only used internally by gas. As above, value is unimportant. */
> - RELOC_NUMBER (R_PPC_16DX_HA, 51)
> + RELOC_NUMBER (R_PPC_16DX_HA, 52)
> + RELOC_NUMBER (R_PPC_VLE_PLTREL24, 53)
> #endif
This seems an odd place to put R_PPC_VLE_PLTREL24. Is it just an
internal relocation? (I don't see anything that would use it, nor the
new R_PPC_VLE_ADDR20. Missing patch?)
> +/* BFD section headers flag. */
> +#define SEC_PPC_VLE SEC_TIC54X_BLOCK
No, this define is in the wrong place, and if you'd defined it in
section.c you would see why you can't reuse SEC_TIC54X_BLOCK.
--
Alan Modra
Australia Development Lab, IBM