[Patch] Fix PR21404 - assertion fail when calculating symbol size

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[Patch] Fix PR21404 - assertion fail when calculating symbol size

Senthil Kumar Selvaraj-2
Hi,

  The below patch fixes a host of problems related to adjustment of
  symbol values and sizes when relaxing.

  1. The patch adjusts symbol size first before adjusting symbol
  value. Otherwise, a symbol whose value just got adjusted to the
  relaxed address also ends up getting resized. See pr21404-1.s below.

  2. The patch reduces symbol sizes only if their span is below an
  alignment boundary. Otherwise, the size gets decremented once when the
  actual instruction is relaxed and padding bytes are added, and again
  when the padding bytes are deleted (if padding ends up being unnecessary).
  pr21404-2.s addresses that, and this bug is really the root cause of PR21404.

  3. The patch adjusts all symbol values before an alignment boundary.
  Previous code did not adjust symbol values if they fell in the
  would-be padded area, resulting in incorrect symbol values in some
  cases (see pr21404-3.s).

  4. The patch increases symbol sizes if alignment directives force it to.
  As pr21404-4.s shows
.global nonzero_sym
L1:
    jmp  L1
nonzero_sym:
    nop
    nop
    .p2align 2
.size nonzero_sym, .-nonzero_sym

  The two nops satisfy the 4 byte alignment at assembly time and
  therefore the size of nonzero_sym is 4. Relaxation shortens
  the 4 byte jmp to a 2 byte rjmp, and to satisfy 4 byte alignment
  the code places 2 extra padding bytes after the nops, increasing
  nonzero_sym's size by 2. This wasn't handled before.

  If the assembly code does not have any align directives, then the
  boundary is the section size, and symbol values and sizes == boundary
  should also get adjusted. To handle that case, the patch
  adds a did_pad variable and uses that to determine whether it should
  use < boundary or <= boundary.

  The patch also gets rid of reloc_toaddr, which is now redundant.
  toaddr is now not adjusted to handle the above case - the newly added
  did_pad variable does the job.

  pr21404-{5,6,7,8} are the same testcases written for local symbols, as
  the code handles them slightly differently.


  Ok for trunk and backport to 2_28-branch?

Regards
Senthil

bfd/ChangeLog:

2017-04-28  Senthil Kumar Selvaraj  <[hidden email]>

  PR ld/21404
        * elf32-avr.c (avr_should_move_sym): New function.
        (avr_should_reduce_sym_size): Likewise.
        (avr_should_increase_sym_size): Likewise.
        (elf32_avr_relax_delete_bytes): Adjust symbol values
  and sizes by calling new functions.

ld/ChangeLog:

2017-04-28  Senthil Kumar Selvaraj  <[hidden email]>

  PR ld/21404
        * testsuite/ld-avr/pr21404-1.d: New test.
        * testsuite/ld-avr/pr21404-1.s: New test.
        * testsuite/ld-avr/pr21404-2.d: New test.
        * testsuite/ld-avr/pr21404-2.s: New test.
        * testsuite/ld-avr/pr21404-3.d: New test.
        * testsuite/ld-avr/pr21404-3.s: New test.
        * testsuite/ld-avr/pr21404-4.d: New test.
        * testsuite/ld-avr/pr21404-4.s: New test.
        * testsuite/ld-avr/pr21404-5.d: New test.
        * testsuite/ld-avr/pr21404-5.s: New test.
        * testsuite/ld-avr/pr21404-6.d: New test.
        * testsuite/ld-avr/pr21404-6.s: New test.
        * testsuite/ld-avr/pr21404-7.d: New test.
        * testsuite/ld-avr/pr21404-7.s: New test.
        * testsuite/ld-avr/pr21404-8.d: New test.
        * testsuite/ld-avr/pr21404-8.s: New test.


diff --git bfd/elf32-avr.c bfd/elf32-avr.c
index 0f6c18834c6..2d0c1adde7e 100644
--- bfd/elf32-avr.c
+++ bfd/elf32-avr.c
@@ -1854,6 +1854,40 @@ elf32_avr_adjust_reloc_if_spans_insn (bfd *abfd,
     }
 }
 
+static bfd_boolean
+avr_should_move_sym (symvalue symval,
+                     bfd_vma start,
+                     bfd_vma end,
+                     bfd_boolean did_pad)
+{
+  bfd_boolean sym_within_boundary =
+          did_pad ? symval < end : symval <= end;
+  return (symval > start && sym_within_boundary);
+}
+
+static bfd_boolean
+avr_should_reduce_sym_size (symvalue symval,
+                            symvalue symend,
+                            bfd_vma start,
+                            bfd_vma end,
+                            bfd_boolean did_pad)
+{
+  bfd_boolean sym_end_within_boundary =
+          did_pad ? symend < end : symend <= end;
+  return (symval <= start && symend > start && sym_end_within_boundary);
+}
+
+static bfd_boolean
+avr_should_increase_sym_size (symvalue symval,
+                              symvalue symend,
+                              bfd_vma start,
+                              bfd_vma end,
+                              bfd_boolean did_pad)
+{
+  return avr_should_move_sym (symval, start, end, did_pad)
+          && symend >= end && did_pad;
+}
+
 /* Delete some bytes from a section while changing the size of an instruction.
    The parameter "addr" denotes the section-relative offset pointing just
    behind the shrinked instruction. "addr+count" point at the first
@@ -1875,13 +1909,14 @@ elf32_avr_relax_delete_bytes (bfd *abfd,
   Elf_Internal_Rela *irel, *irelend;
   Elf_Internal_Sym *isym;
   Elf_Internal_Sym *isymbuf = NULL;
-  bfd_vma toaddr, reloc_toaddr;
+  bfd_vma toaddr;
   struct elf_link_hash_entry **sym_hashes;
   struct elf_link_hash_entry **end_hashes;
   unsigned int symcount;
   struct avr_relax_info *relax_info;
   struct avr_property_record *prop_record = NULL;
   bfd_boolean did_shrink = FALSE;
+  bfd_boolean did_pad = FALSE;
 
   symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
   sec_shndx = _bfd_elf_section_from_bfd_section (abfd, sec);
@@ -1912,17 +1947,6 @@ elf32_avr_relax_delete_bytes (bfd *abfd,
         }
     }
 
-  /* We need to look at all relocs with offsets less than toaddr. prop
-     records handling adjusts toaddr downwards to avoid moving syms at the
-     address of the property record, but all relocs with offsets between addr
-     and the current value of toaddr need to have their offsets adjusted.
-     Assume addr = 0, toaddr = 4 and count = 2. After prop records handling,
-     toaddr becomes 2, but relocs with offsets 2 and 3 still need to be
-     adjusted (to 0 and 1 respectively), as the first 2 bytes are now gone.
-     So record the current value of toaddr here, and use it when adjusting
-     reloc offsets. */
-  reloc_toaddr = toaddr;
-
   irel = elf_section_data (sec)->relocs;
   irelend = irel + sec->reloc_count;
 
@@ -1962,10 +1986,7 @@ elf32_avr_relax_delete_bytes (bfd *abfd,
          to remember we didn't delete anything i.e. don't set did_shrink,
          so that we don't corrupt reloc offsets or symbol values.*/
       memset (contents + toaddr - count, fill, count);
-
-      /* Adjust the TOADDR to avoid moving symbols located at the address
-         of the property record, which has not moved.  */
-      toaddr -= count;
+      did_pad = TRUE;
     }
 
   if (!did_shrink)
@@ -1981,7 +2002,7 @@ elf32_avr_relax_delete_bytes (bfd *abfd,
 
       /* Get the new reloc address.  */
       if ((irel->r_offset > addr
-           && irel->r_offset < reloc_toaddr))
+           && irel->r_offset < toaddr))
         {
           if (debug_relax)
             printf ("Relocation at address 0x%x needs to be moved.\n"
@@ -2059,7 +2080,7 @@ elf32_avr_relax_delete_bytes (bfd *abfd,
                  {
                    /* If there is an alignment boundary, we only need to
                       adjust addends that end up below the boundary. */
-                   bfd_vma shrink_boundary = (reloc_toaddr
+                   bfd_vma shrink_boundary = (toaddr
                                               + sec->output_section->vma
                                               + sec->output_offset);
 
@@ -2102,12 +2123,10 @@ elf32_avr_relax_delete_bytes (bfd *abfd,
  {
   if (isym->st_shndx == sec_shndx)
             {
-      if (isym->st_value > addr
-                  && isym->st_value <= toaddr)
-                isym->st_value -= count;
-
-              if (isym->st_value <= addr
-                  && isym->st_value + isym->st_size > addr)
+              symvalue symval = isym->st_value;
+              symvalue symend = symval + isym->st_size;
+              if (avr_should_reduce_sym_size (symval, symend,
+                                      addr, toaddr, did_pad))
                 {
                   /* If this assert fires then we have a symbol that ends
                      part way through an instruction.  Does that make
@@ -2115,6 +2134,12 @@ elf32_avr_relax_delete_bytes (bfd *abfd,
                   BFD_ASSERT (isym->st_value + isym->st_size >= addr + count);
                   isym->st_size -= count;
                 }
+              else if (avr_should_increase_sym_size (symval, symend,
+                                      addr, toaddr, did_pad))
+                isym->st_size += count;
+
+              if (avr_should_move_sym (symval, addr, toaddr, did_pad))
+                isym->st_value -= count;
             }
  }
     }
@@ -2131,20 +2156,24 @@ elf32_avr_relax_delete_bytes (bfd *abfd,
            || sym_hash->root.type == bfd_link_hash_defweak)
           && sym_hash->root.u.def.section == sec)
         {
-          if (sym_hash->root.u.def.value > addr
-              && sym_hash->root.u.def.value <= toaddr)
-            sym_hash->root.u.def.value -= count;
+          symvalue symval = sym_hash->root.u.def.value;
+          symvalue symend = symval + sym_hash->size;
 
-          if (sym_hash->root.u.def.value <= addr
-              && (sym_hash->root.u.def.value + sym_hash->size > addr))
+          if (avr_should_reduce_sym_size (symval, symend,
+                                  addr, toaddr, did_pad))
             {
               /* If this assert fires then we have a symbol that ends
                  part way through an instruction.  Does that make
                  sense?  */
-              BFD_ASSERT (sym_hash->root.u.def.value + sym_hash->size
-                          >= addr + count);
+              BFD_ASSERT (symend >= addr + count);
               sym_hash->size -= count;
             }
+          else if (avr_should_increase_sym_size (symval, symend,
+                                  addr, toaddr, did_pad))
+              sym_hash->size += count;
+
+          if (avr_should_move_sym (symval, addr, toaddr, did_pad))
+            sym_hash->root.u.def.value -= count;
         }
     }
 
diff --git ld/testsuite/ld-avr/pr21404-1.d ld/testsuite/ld-avr/pr21404-1.d
new file mode 100644
index 00000000000..fc7103fc9f2
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-1.d
@@ -0,0 +1,12 @@
+#name: AVR symbol size adjustment with non zero symbol value
+#as: -mmcu=avrxmega2 -mlink-relax
+#ld:  -mavrxmega2 --relax
+#source: pr21404-1.s
+#nm: -n -S
+#target: avr-*-*
+
+#...
+00000000 00000004 T main
+#...
+00000002 00000002 T nonzero_sym
+#...
diff --git ld/testsuite/ld-avr/pr21404-1.s ld/testsuite/ld-avr/pr21404-1.s
new file mode 100644
index 00000000000..66ddb7d4a31
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-1.s
@@ -0,0 +1,11 @@
+    .file "pr21404-1.s"
+.section .text,"ax",@progbits
+.global nonzero_sym
+.global main
+main:
+L1:
+    jmp  L1
+nonzero_sym:
+    nop
+.size main, .-main
+.size nonzero_sym, .-nonzero_sym
diff --git ld/testsuite/ld-avr/pr21404-2.d ld/testsuite/ld-avr/pr21404-2.d
new file mode 100644
index 00000000000..e85735cd7d2
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-2.d
@@ -0,0 +1,16 @@
+#name: AVR symbol size adjustment across alignment boundary
+#as: -mmcu=avrxmega2 -mlink-relax
+#ld:  -mavrxmega2 --relax
+#source: pr21404-2.s
+#nm: -n -S
+#target: avr-*-*
+
+#...
+00000000 00000006 T main
+00000000 00000004 T size_after_align
+00000000 00000004 T size_before_align
+#...
+00000002 00000002 T nonzero_sym_after_align
+00000002 00000004 T nonzero_sym_after_end
+00000002 00000002 T nonzero_sym_before_align
+#...
diff --git ld/testsuite/ld-avr/pr21404-2.s ld/testsuite/ld-avr/pr21404-2.s
new file mode 100644
index 00000000000..e1da1ae4b3b
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-2.s
@@ -0,0 +1,25 @@
+    .file "pr21404.s"
+.section .text,"ax",@progbits
+.global size_before_align
+.global size_after_align
+.global main
+.global nonzero_sym_before_align
+.global nonzero_sym_after_align
+.global nonzero_sym_after_end
+main:
+size_before_align:
+size_after_align:
+L1:
+    jmp  L1
+nonzero_sym_before_align:
+nonzero_sym_after_align:
+nonzero_sym_after_end:
+    jmp  L1
+.size size_before_align, .-size_before_align
+.size nonzero_sym_before_align, .-nonzero_sym_before_align
+    .p2align 1
+.size size_after_align, .-size_after_align
+.size nonzero_sym_after_align, .-nonzero_sym_after_align
+.word L1
+.size main, .-main
+.size nonzero_sym_after_end, .-nonzero_sym_after_end
diff --git ld/testsuite/ld-avr/pr21404-3.d ld/testsuite/ld-avr/pr21404-3.d
new file mode 100644
index 00000000000..145b48fff25
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-3.d
@@ -0,0 +1,10 @@
+#name: AVR symbol value adjustment with non zero symbol value
+#as: -mmcu=avrxmega2 -mlink-relax
+#ld:  -mavrxmega2 --relax
+#source: pr21404-3.s
+#nm: -n -S
+#target: avr-*-*
+
+#...
+00000006 T nonzero_sym
+#...
diff --git ld/testsuite/ld-avr/pr21404-3.s ld/testsuite/ld-avr/pr21404-3.s
new file mode 100644
index 00000000000..d62ecd99ed4
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-3.s
@@ -0,0 +1,10 @@
+    .file "pr21404-3.s"
+.section .text,"ax",@progbits
+.global nonzero_sym
+L1:
+    jmp  L1
+    jmp  L1
+    jmp L1
+    .p2align 1
+nonzero_sym:
+    jmp L1
diff --git ld/testsuite/ld-avr/pr21404-4.d ld/testsuite/ld-avr/pr21404-4.d
new file mode 100644
index 00000000000..b80dfcffbeb
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-4.d
@@ -0,0 +1,10 @@
+#name: AVR symbol size increase for alignment
+#as: -mmcu=avrxmega2 -mlink-relax
+#ld:  -mavrxmega2 --relax
+#source: pr21404-4.s
+#nm: -n -S
+#target: avr-*-*
+
+#...
+00000002 00000006 T nonzero_sym
+#...
diff --git ld/testsuite/ld-avr/pr21404-4.s ld/testsuite/ld-avr/pr21404-4.s
new file mode 100644
index 00000000000..3e957bb43b4
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-4.s
@@ -0,0 +1,10 @@
+    .file "pr21404-4.s"
+.section .text,"ax",@progbits
+.global nonzero_sym
+L1:
+    jmp  L1
+nonzero_sym:
+    nop
+    nop
+    .p2align 2
+.size nonzero_sym, .-nonzero_sym
diff --git ld/testsuite/ld-avr/pr21404-5.d ld/testsuite/ld-avr/pr21404-5.d
new file mode 100644
index 00000000000..519c50fdd6c
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-5.d
@@ -0,0 +1,11 @@
+#name: AVR local symbol size adjustment with non zero symbol value
+#as: -mmcu=avrxmega2 -mlink-relax
+#ld:  -mavrxmega2 --relax
+#source: pr21404-5.s
+#nm: -n -S
+#target: avr-*-*
+
+#...
+00000000 00000004 t _main
+00000002 00000002 t _nonzero_sym
+#...
diff --git ld/testsuite/ld-avr/pr21404-5.s ld/testsuite/ld-avr/pr21404-5.s
new file mode 100644
index 00000000000..ccf2394e0ea
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-5.s
@@ -0,0 +1,9 @@
+    .file "pr21404-1.s"
+.section .text,"ax",@progbits
+_main:
+L1:
+    jmp  L1
+_nonzero_sym:
+    nop
+.size _main, .-_main
+.size _nonzero_sym, .-_nonzero_sym
diff --git ld/testsuite/ld-avr/pr21404-6.d ld/testsuite/ld-avr/pr21404-6.d
new file mode 100644
index 00000000000..784fc7c3262
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-6.d
@@ -0,0 +1,16 @@
+#name: AVR local symbol size adjustment across alignment boundary
+#as: -mmcu=avrxmega2 -mlink-relax
+#ld:  -mavrxmega2 --relax
+#source: pr21404-6.s
+#nm: -n -S
+#target: avr-*-*
+
+#...
+00000000 00000006 t main
+00000000 00000004 t size_after_align
+00000000 00000004 t size_before_align
+#...
+00000002 00000002 t nonzero_sym_after_align
+00000002 00000004 t nonzero_sym_after_end
+00000002 00000002 t nonzero_sym_before_align
+#...
diff --git ld/testsuite/ld-avr/pr21404-6.s ld/testsuite/ld-avr/pr21404-6.s
new file mode 100644
index 00000000000..36f2301ba86
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-6.s
@@ -0,0 +1,19 @@
+    .file "pr21404-6.s"
+.section .text,"ax",@progbits
+main:
+size_before_align:
+size_after_align:
+L1:
+    jmp  L1
+nonzero_sym_before_align:
+nonzero_sym_after_align:
+nonzero_sym_after_end:
+    jmp  L1
+.size size_before_align, .-size_before_align
+.size nonzero_sym_before_align, .-nonzero_sym_before_align
+    .p2align 1
+.size size_after_align, .-size_after_align
+.size nonzero_sym_after_align, .-nonzero_sym_after_align
+.word L1
+.size main, .-main
+.size nonzero_sym_after_end, .-nonzero_sym_after_end
diff --git ld/testsuite/ld-avr/pr21404-7.d ld/testsuite/ld-avr/pr21404-7.d
new file mode 100644
index 00000000000..1360acb3e5e
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-7.d
@@ -0,0 +1,10 @@
+#name: AVR local symbol value adjustment with non zero symbol value
+#as: -mmcu=avrxmega2 -mlink-relax
+#ld:  -mavrxmega2 --relax
+#source: pr21404-7.s
+#nm: -n -S
+#target: avr-*-*
+
+#...
+00000006 t nonzero_sym
+#...
diff --git ld/testsuite/ld-avr/pr21404-7.s ld/testsuite/ld-avr/pr21404-7.s
new file mode 100644
index 00000000000..b7e4e354dd6
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-7.s
@@ -0,0 +1,9 @@
+    .file "pr21404-7.s"
+.section .text,"ax",@progbits
+L1:
+    jmp  L1
+    jmp  L1
+    jmp L1
+    .p2align 1
+nonzero_sym:
+    jmp L1
diff --git ld/testsuite/ld-avr/pr21404-8.d ld/testsuite/ld-avr/pr21404-8.d
new file mode 100644
index 00000000000..d70ba8fc3a5
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-8.d
@@ -0,0 +1,10 @@
+#name: AVR local symbol size increase for alignment
+#as: -mmcu=avrxmega2 -mlink-relax
+#ld:  -mavrxmega2 --relax
+#source: pr21404-8.s
+#nm: -n -S
+#target: avr-*-*
+
+#...
+00000002 00000006 t nonzero_sym
+#...
diff --git ld/testsuite/ld-avr/pr21404-8.s ld/testsuite/ld-avr/pr21404-8.s
new file mode 100644
index 00000000000..6b00df541a6
--- /dev/null
+++ ld/testsuite/ld-avr/pr21404-8.s
@@ -0,0 +1,9 @@
+    .file "pr21404-8.s"
+.section .text,"ax",@progbits
+L1:
+    jmp  L1
+nonzero_sym:
+    nop
+    nop
+    .p2align 2
+.size nonzero_sym, .-nonzero_sym
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix PR21404 - assertion fail when calculating symbol size

Nick Clifton
Hi Senthil,

  Nice - thanks for looking into this.

>    Ok for trunk and backport to 2_28-branch?

Approved for trunk.  Ping Tristan for the branch.

Cheers
  Nick
 
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix PR21404 - assertion fail when calculating symbol size

Senthil Kumar Selvaraj-2

Nick Clifton writes:

> Hi Senthil,
>
>   Nice - thanks for looking into this.
>
>>    Ok for trunk and backport to 2_28-branch?
>
> Approved for trunk.  Ping Tristan for the branch.

Tristan, ok to backport this patch to 2.28 branch?
(https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=b660e9eb7a45cfe99e719c5d16af35913a2fdc96)

Regards
Senthil
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] Fix PR21404 - assertion fail when calculating symbol size

Tristan Gingold-2
In reply to this post by Nick Clifton
On 02/05/2017 13:05, Nick Clifton wrote:
> Hi Senthil,
>
>   Nice - thanks for looking into this.
>
>>    Ok for trunk and backport to 2_28-branch?
>
> Approved for trunk.  Ping Tristan for the branch.

Ok for 2.28