Re: [PATCH] [PowerPC] VLE update

[Alexander Fedotov <alfedotov at gmail dot com>]

>>Looks to me like you've been give the job of sweeping out the
>>stables..

Well I have the same feelings, but that's is always part of our job :)
And thats why I appreciate your feedback on all this stuff.

>> +  /* PowerPC VLE code.  */
>> +#define SEC_PPC_VLE 0x100000000
>
> How can this possibly work?  The field is an unsigned int.

You are right. In original patch it was just reuse:
+#define SEC_PPC_VLE SEC_TIC54X_BLOCK

Well what the best way to deal with this ? Extend flagword to long ?
Or keep reuse ?


>> +     (R_PPC_VLE_PLTREL24): New relocation.
>> +     (R_PPC_VLE_ADDR20): Likewise.
>
>How are these generated?  I see code that processes them, but nothing
> to create them.

Regarding to R_PPC_VLE_PLTREL24 I forgot to apply patch:

@@ -9686,8 +9845,17 @@ ppc_elf_relocate_section (bfd *output_bfd,
            }
        }
       else
-       r = _bfd_final_link_relocate (howto, input_bfd, input_section, contents,
+       {
+         if ((elf_section_flags (input_section) & SHF_PPC_VLE) != 0)
+           {
+             if (howto == ppc_elf_howto_table[R_PPC_PLTREL24])
+               howto = ppc_elf_howto_table[R_PPC_VLE_PLTREL24];
+             else if (howto == ppc_elf_howto_table[R_PPC_LOCAL24PC])
+               howto = ppc_elf_howto_table[R_PPC_VLE_REL24];
+           }
+         r = _bfd_final_link_relocate (howto, input_bfd,
input_section, contents,
                                      rel->r_offset, relocation, addend);
+       }


Yes, R_PPC_VLE_ADDR20 is not used yet. But it is defined by Power
Architecture 32-bit Application Binary Interface Supplement 1.0 -
Linux & Embedded. So it looks like just missing.


>> @@ -253,12 +253,13 @@ get_powerpc_dialect (struct disassemble_info *info)
>>    dialect = POWERPC_DIALECT (info);
>>
>>    /* Disassemble according to the section headers flags for VLE-mode.  */
>> -  if (dialect & PPC_OPCODE_VLE
>> -      && info->section != NULL && info->section->owner != NULL
>> +  if (dialect & PPC_OPCODE_VLE)
>> +    return dialect;
>> +  else if (info->section != NULL && info->section->owner != NULL
>>        && bfd_get_flavour (info->section->owner) == bfd_target_elf_flavour
>>        && elf_object_id (info->section->owner) == PPC32_ELF_DATA
>>        && (elf_section_flags (info->section) & SHF_PPC_VLE) != 0)
>> -    return dialect;
>> +    return PPC_OPCODE_VLE;
>>    else
>>      return dialect & ~ PPC_OPCODE_VLE;
>
>Can you explain why this change is necessary?

As I can see it resolves problem with LSP disassembling on lsp.s test.
Unfortunately I have no more history for this one. Drop it from the
patch ?

Best regards,
Alex


On Mon, Aug 28, 2017 at 1:34 AM, Alan Modra <amodra@gmail.com> wrote:
> On Sat, Aug 26, 2017 at 08:37:22PM +0300, Alexander Fedotov wrote:
>> This patch adds support PLT for VLE ISA. Also adds new relocations and
>> section headers handling.
>
> Looks to me like you've been give the job of sweeping out the
> stables..
>
>> +  /* PowerPC VLE code.  */
>> +#define SEC_PPC_VLE 0x100000000
>
> How can this possibly work?  The field is an unsigned int.
>
>> @@ -7371,10 +7474,21 @@ ppc_elf_relax_section (bfd *abfd,
>>           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)
>> +             {
>> +               t0 = bfd_get_32 (abfd, hit_addr);
>> +               t0 &= ~0x01fffffe;
>> +               t0 |= val & 0x01fffffe;
>> +               bfd_put_32 (abfd, t0, hit_addr);
>> +             }
>> +           else
>> +             {
>> +               t0 = bfd_get_32 (abfd, hit_addr);
>> +               t0 &= ~0x3fffffc;
>> +               t0 |= val & 0x3fffffc;
>> +               bfd_put_32 (abfd, t0, hit_addr);
>> +             }
>
> What is going on here?
>
>> +     (R_PPC_VLE_PLTREL24): New relocation.
>> +     (R_PPC_VLE_ADDR20): Likewise.
>
> How are these generated?  I see code that processes them, but nothing
> to create them.
>
>> @@ -253,12 +253,13 @@ get_powerpc_dialect (struct disassemble_info *info)
>>    dialect = POWERPC_DIALECT (info);
>>
>>    /* Disassemble according to the section headers flags for VLE-mode.  */
>> -  if (dialect & PPC_OPCODE_VLE
>> -      && info->section != NULL && info->section->owner != NULL
>> +  if (dialect & PPC_OPCODE_VLE)
>> +    return dialect;
>> +  else if (info->section != NULL && info->section->owner != NULL
>>        && bfd_get_flavour (info->section->owner) == bfd_target_elf_flavour
>>        && elf_object_id (info->section->owner) == PPC32_ELF_DATA
>>        && (elf_section_flags (info->section) & SHF_PPC_VLE) != 0)
>> -    return dialect;
>> +    return PPC_OPCODE_VLE;
>>    else
>>      return dialect & ~ PPC_OPCODE_VLE;
>
> Can you explain why this change is necessary?
>
> --
> Alan Modra
> Australia Development Lab, IBM



-- 
Best regards,
AF