diff mbox

[V2] ARM: plt_size functions need to read instructions in right byte order

Message ID 1413955513-4923-2-git-send-email-victor.kamensky@linaro.org
State New
Headers show

Commit Message

vkamensky Oct. 22, 2014, 5:25 a.m. UTC 
elf32_arm_plt0_size and elf32_arm_plt_size read instructions
to determine what is size of PLT entry. However it does not
read instruction correctly in case of ARM big endian V7 case.
In this case instructions are still kept in little endian
order (BE8).

Because of that in armv7b case gdb.base/dprintf-pending.exp
test is failing - It cannot find 'pendfunc@plt' symbol.
And that symbol is not created because elf32_arm_get_synthetic_symtab
function does not create 'pendfunc@plt' symbol for symbols
from PLT after elf32_arm_plt0_size returns -1.

Fix is to introduce code reading functions read_code32,
read_code16 which would read code content in little endian
mode when it is armv7b executabe (i.e e_flags has EF_ARM_BE8)
set. elf32_arm_plt0_size and elf32_arm_plt_size to use these
functions in place where H_GET_32, H_GET_16 were used before.

bfd/ChangeLog:

2014-10-21  Victor Kamensky  <victor.kamensky@linaro.org>

	* elf32-arm.c (read_code32): New function to read 32 bit
	arm instruction.
	(read_code16): New function to read 16 bit thumb instrution.
	(elf32_arm_plt0_size, elf32_arm_plt_size) change code to use
	read_code32, read_code16 to read instruction to deal with
	BE8 arm case.
---
 bfd/elf32-arm.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

Comments

Alan Modra Oct. 22, 2014, 10:50 p.m. UTC | #1 
On Tue, Oct 21, 2014 at 10:25:13PM -0700, Victor Kamensky wrote:
> +  if ((elf_elfheader (abfd)->e_flags) & EF_ARM_BE8)
                                       ^ this parenthesis is in the
wrong place.  gcc will warn about "if (x & y)", breaking -Werror
builds.  Writing "if ((x & y))" silences the gcc warning, so you
should write

  if ((elf_elfheader (abfd)->e_flags & EF_ARM_BE8))

or as I suggested in the previous email (without explaining why)

  if ((elf_elfheader (abfd)->e_flags & EF_ARM_BE8) != 0)

The patch is OK to commit with that change.
vkamensky Oct. 23, 2014, 1:01 a.m. UTC | #2 
On 22 October 2014 15:50, Alan Modra <amodra@gmail.com> wrote:
> On Tue, Oct 21, 2014 at 10:25:13PM -0700, Victor Kamensky wrote:
>> +  if ((elf_elfheader (abfd)->e_flags) & EF_ARM_BE8)
>                                        ^ this parenthesis is in the
> wrong place.  gcc will warn about "if (x & y)", breaking -Werror
> builds.  Writing "if ((x & y))" silences the gcc warning, so you
> should write
>
>   if ((elf_elfheader (abfd)->e_flags & EF_ARM_BE8))
>
> or as I suggested in the previous email (without explaining why)
>
>   if ((elf_elfheader (abfd)->e_flags & EF_ARM_BE8) != 0)

Thanks, Alan.

Sorry, did not catch that in your previous email I will fix that
and repost patch shortly.

> The patch is OK to commit with that change.

Besides reposting updated patch is there any other actions
on my side? I am bit a new to this. My assumption is that once
patch is reviewed and approved, some of maintainers will pick it
and commit. Is it not correct?

Thanks,
Victor

> --
> Alan Modra
> Australia Development Lab, IBM
Alan Modra Oct. 23, 2014, 1:12 a.m. UTC | #3 
On Wed, Oct 22, 2014 at 06:01:12PM -0700, Victor Kamensky wrote:
> On 22 October 2014 15:50, Alan Modra <amodra@gmail.com> wrote:
> > On Tue, Oct 21, 2014 at 10:25:13PM -0700, Victor Kamensky wrote:
> >> +  if ((elf_elfheader (abfd)->e_flags) & EF_ARM_BE8)
> >                                        ^ this parenthesis is in the
> > wrong place.  gcc will warn about "if (x & y)", breaking -Werror
> > builds.  Writing "if ((x & y))" silences the gcc warning, so you
> > should write
> >
> >   if ((elf_elfheader (abfd)->e_flags & EF_ARM_BE8))
> >
> > or as I suggested in the previous email (without explaining why)
> >
> >   if ((elf_elfheader (abfd)->e_flags & EF_ARM_BE8) != 0)
> 
> Thanks, Alan.
> 
> Sorry, did not catch that in your previous email I will fix that
> and repost patch shortly.
> 
> > The patch is OK to commit with that change.
> 
> Besides reposting updated patch is there any other actions
> on my side? I am bit a new to this. My assumption is that once
> patch is reviewed and approved, some of maintainers will pick it
> and commit. Is it not correct?

Ah, I thought you had git commit privilege.  Don't worry about
reposting the patch.  I'll commit a fixed version for you.
vkamensky Oct. 23, 2014, 1:18 a.m. UTC | #4 
On 22 October 2014 18:12, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Oct 22, 2014 at 06:01:12PM -0700, Victor Kamensky wrote:
>> On 22 October 2014 15:50, Alan Modra <amodra@gmail.com> wrote:
>> > On Tue, Oct 21, 2014 at 10:25:13PM -0700, Victor Kamensky wrote:
>> >> +  if ((elf_elfheader (abfd)->e_flags) & EF_ARM_BE8)
>> >                                        ^ this parenthesis is in the
>> > wrong place.  gcc will warn about "if (x & y)", breaking -Werror
>> > builds.  Writing "if ((x & y))" silences the gcc warning, so you
>> > should write
>> >
>> >   if ((elf_elfheader (abfd)->e_flags & EF_ARM_BE8))
>> >
>> > or as I suggested in the previous email (without explaining why)
>> >
>> >   if ((elf_elfheader (abfd)->e_flags & EF_ARM_BE8) != 0)
>>
>> Thanks, Alan.
>>
>> Sorry, did not catch that in your previous email I will fix that
>> and repost patch shortly.
>>
>> > The patch is OK to commit with that change.
>>
>> Besides reposting updated patch is there any other actions
>> on my side? I am bit a new to this. My assumption is that once
>> patch is reviewed and approved, some of maintainers will pick it
>> and commit. Is it not correct?
>
> Ah, I thought you had git commit privilege.  Don't worry about
> reposting the patch.  I'll commit a fixed version for you.

Emails crossing. It is already out. Thank you. I appreciate your
time and help.

Thanks,
Victor

> --
> Alan Modra
> Australia Development Lab, IBM
diff mbox

Patch

diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index 08aa3f9..450cabd 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -15953,6 +15953,34 @@  const struct elf_size_info elf32_arm_size_info =
   bfd_elf32_swap_reloca_out
 };
 
+static bfd_vma
+read_code32 (const bfd *abfd, const bfd_byte *addr)
+{
+  bfd_vma retval;
+
+  if ((elf_elfheader (abfd)->e_flags) & EF_ARM_BE8)
+    /* V7 BE8 code is always little endian */
+    retval = bfd_getl32 (addr);
+  else
+    retval = bfd_get_32 (abfd, addr);
+  return retval;
+}
+
+
+static bfd_vma
+read_code16 (const bfd *abfd, const bfd_byte *addr)
+{
+  bfd_vma retval;
+
+  if ((elf_elfheader (abfd)->e_flags) & EF_ARM_BE8)
+    /* V7 BE8 code is always little endian */
+    retval = bfd_getl16 (addr);
+  else
+    retval = bfd_get_16 (abfd, addr);
+  return retval;
+}
+
+
 /* Return size of plt0 entry starting at ADDR
    or (bfd_vma) -1 if size can not be determined.  */
 
@@ -15962,7 +15990,7 @@  elf32_arm_plt0_size (const bfd *abfd, const bfd_byte *addr)
   bfd_vma first_word;
   bfd_vma plt0_size;
 
-  first_word = H_GET_32 (abfd, addr);
+  first_word = read_code32 (abfd, addr);
 
   if (first_word == elf32_arm_plt0_entry[0])
     plt0_size = 4 * ARRAY_SIZE (elf32_arm_plt0_entry);
@@ -15987,17 +16015,17 @@  elf32_arm_plt_size (const bfd *abfd, const bfd_byte *start, bfd_vma offset)
   const bfd_byte *addr = start + offset;
 
   /* PLT entry size if fixed on Thumb-only platforms.  */
-  if (H_GET_32(abfd, start) == elf32_thumb2_plt0_entry[0])
+  if (read_code32 (abfd, start) == elf32_thumb2_plt0_entry[0])
       return 4 * ARRAY_SIZE (elf32_thumb2_plt_entry);
 
   /* Respect Thumb stub if necessary.  */
-  if (H_GET_16(abfd, addr) == elf32_arm_plt_thumb_stub[0])
+  if (read_code16 (abfd, addr) == elf32_arm_plt_thumb_stub[0])
     {
       plt_size += 2 * ARRAY_SIZE(elf32_arm_plt_thumb_stub);
     }
 
   /* Strip immediate from first add.  */
-  first_insn = H_GET_32(abfd, addr + plt_size) & 0xffffff00;
+  first_insn = read_code32 (abfd, addr + plt_size) & 0xffffff00;
 
 #ifdef FOUR_WORD_PLT
   if (first_insn == elf32_arm_plt_entry[0])