回复:[PATCH] RISC-V: PR27566, Do not relax when data segment phase is exp_seg_relro_adjust.

Message ID 174adcbc-0903-42d9-980e-35114db62678.lifang_xia@c-sky.com
State New
Headers show
Series
  • 回复:[PATCH] RISC-V: PR27566, Do not relax when data segment phase is exp_seg_relro_adjust.
Related show

Commit Message

=?UTF-8?B?5aSP56uL5pa5?= May 17, 2021, 3:04 a.m.
Looks good to me. Thanks!

Brs,
Lifang

夏立方 / 软件工具
Lifang Xia / Tools
T: +86 13186997717
M:lifang_xia@c-sky.com
杭州西湖区西斗门路3号天堂软件园A座15层
3 Xidoumen Rd,BldgA,15F,Hangzhou,China   
P.C.: 310012


------------------------------------------------------------------
发件人:Nelson Chu <nelson.chu@sifive.com>
发送时间:2021年5月12日(星期三) 23:26
收件人:binutils@sourceware.org <binutils@sourceware.org>
抄 送:lifang_xia <lifang_xia@linux.alibaba.com>
主 题:[PATCH] RISC-V: PR27566, Do not relax when data segment phase is exp_seg_relro_adjust.

2021-05-12  Nelson Chu  <nelson.chu@sifive.com>
            Lifang Xia  <lifang_xia@c-sky.com>

The data segment phase exp_seg_relro_adjust means we are still adjusting the
relro segments, so we will get the symbol values which havn't consider the
relro.  It is dangerous and we shouldn't do the relaxations at this stage.
Otherwise, we may get the truncated fails when the relax range crossing the
data segment.

One of the solution is that, we use a pointer to monitor the data segment
phase while relaxing, to know whether the relro has been handled or not.
Once we check the phase is exp_seg_relro_adjust, we should skip this round
of relaxations, since the incorrect symbol values will affect the correctness
of relaxations.  I think we probably need to record more information about
data segment or alignments in the future, to make sure it is safe to doing
relaxations.

For the two new testcases, relro-relax-lui and relro-relax-pcrel, we get
the following truncated errors when using toolchains, which enable relro:

(.text+0x0): relocation truncated to fit: R_RISCV_GPREL_I against symbol `SymbolRodata' defined in .rodata section in test1.o

After applying this patch, the truncated errors should be resolved.
However, only linux toolchains support -z relro, so we only test these
two testcases when supporting shared library.

bfd/
    PR 27566
    * elfnn-riscv.c (struct riscv_elf_link_hash_table): New integer pointer
    to monitor the data segment phase.
    (bfd_elfNN_riscv_set_data_segment_info): New function called by
    after_allocation, to set the data_segment_phase from expld.dataseg.
    (_bfd_riscv_relax_section): Don't relax when data_segment_phase is
    exp_seg_relro_adjust (0x4).
    * elfxx-riscv.h (bfd_elf32_riscv_set_data_segment_info): New extern.
    (bfd_elf64_riscv_set_data_segment_info): Likewise.
ld/
    PR 27566
    * emultempl/riscvelf.em (after_allocation): Call
    riscv_set_data_segment_info to set data segment phase before relaxing.
    * testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated.
    * testsuite/ld-riscv-elf/relro-relax-lui.d: New testcase.
    * testsuite/ld-riscv-elf/relro-relax-lui.s: Likewise.
    * testsuite/ld-riscv-elf/relro-relax-pcrel.d: Likewise.
    * testsuite/ld-riscv-elf/relro-relax-pcrel.s: Likewise.
---
 bfd/elfnn-riscv.c                             | 20 +++++++++++++++++++-
 bfd/elfxx-riscv.h                             |  6 +++++-
 ld/emultempl/riscvelf.em                      | 14 ++++++++++++++
 ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp    |  3 +++
 ld/testsuite/ld-riscv-elf/relro-relax-lui.d   | 12 ++++++++++++
 ld/testsuite/ld-riscv-elf/relro-relax-lui.s   | 15 +++++++++++++++
 ld/testsuite/ld-riscv-elf/relro-relax-pcrel.d | 12 ++++++++++++
 ld/testsuite/ld-riscv-elf/relro-relax-pcrel.s | 14 ++++++++++++++
 8 files changed, 94 insertions(+), 2 deletions(-)
 create mode 100644 ld/testsuite/ld-riscv-elf/relro-relax-lui.d
 create mode 100644 ld/testsuite/ld-riscv-elf/relro-relax-lui.s
 create mode 100644 ld/testsuite/ld-riscv-elf/relro-relax-pcrel.d
 create mode 100644 ld/testsuite/ld-riscv-elf/relro-relax-pcrel.s

Comments

Nelson Chu May 20, 2021, 9 a.m. | #1
On Mon, May 17, 2021 at 11:04 AM 夏立方 <lifang_xia@c-sky.com> wrote:
>

> Looks good to me. Thanks!


Thanks, Lifang.  I will wait for a while to avoid missing other suggestions :)

Thanks
Nelson
Nelson Chu May 31, 2021, 3:31 a.m. | #2
On Thu, May 20, 2021 at 5:00 PM Nelson Chu <nelson.chu@sifive.com> wrote:
>

> On Mon, May 17, 2021 at 11:04 AM 夏立方 <lifang_xia@c-sky.com> wrote:

> >

> > Looks good to me. Thanks!

>

> Thanks, Lifang.  I will wait for a while to avoid missing other suggestions :)


Committed.

Thanks
Nelson

Patch

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index a944b33..906da09 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -132,6 +132,10 @@  struct riscv_elf_link_hash_table

   /* Re-run the relaxations from relax pass 0 if TRUE.  */
   bool restart_relax;
+
+  /* The data segment phase, don't relax the section
+     when it is exp_seg_relro_adjust.  */
+  int *data_segment_phase;
 };

 /* Instruction access functions. */
@@ -4608,6 +4612,17 @@  _bfd_riscv_relax_delete (bfd *abfd,
   return true;
 }

+/* Called by after_allocation to set the information of data segment
+   before relaxing.  */
+
+void
+bfd_elfNN_riscv_set_data_segment_info (struct bfd_link_info *info,
+                                       int *data_segment_phase)
+{
+  struct riscv_elf_link_hash_table *htab = riscv_elf_hash_table (info);
+  htab->data_segment_phase = data_segment_phase;
+}
+
 /* Called by after_allocation to check if we need to run the whole
    relaxations again.  */

@@ -4660,7 +4675,10 @@  _bfd_riscv_relax_section (bfd *abfd, asection *sec,
       || (info->disable_target_specific_optimizations
    && info->relax_pass < 2)
       || (htab->restart_relax
-   && info->relax_pass == 3))
+   && info->relax_pass == 3)
+      /* The exp_seg_relro_adjust is enum phase_enum (0x4),
+  and defined in ld/ldexp.h.  */
+      || *(htab->data_segment_phase) == 4)
     return true;

   riscv_init_pcgp_relocs (&pcgp_relocs);
diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
index ff18ff6..4f91928 100644
--- a/bfd/elfxx-riscv.h
+++ b/bfd/elfxx-riscv.h
@@ -97,6 +97,10 @@  riscv_compare_subsets (const char *, const char *);

 extern bool
 bfd_elf32_riscv_restart_relax_sections (struct bfd_link_info *);
-
 extern bool
 bfd_elf64_riscv_restart_relax_sections (struct bfd_link_info *);
+
+extern void
+bfd_elf32_riscv_set_data_segment_info (struct bfd_link_info *, int *);
+extern void
+bfd_elf64_riscv_set_data_segment_info (struct bfd_link_info *, int *);
diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em
index 5fa7c77..c625a63 100644
--- a/ld/emultempl/riscvelf.em
+++ b/ld/emultempl/riscvelf.em
@@ -62,6 +62,20 @@  gld${EMULATION_NAME}_after_allocation (void)
  }
     }

+  /* PR 27566, if the phase of data segment is exp_seg_relro_adjust,
+     that means we are still adjusting the relro, and shouldn't do the
+     relaxations at this stage.  Otherwise, we will get the symbol
+     values beofore handling the relro, and may cause truncated fails
+     when the relax range crossing the data segment.  One of the solution
+     is to monitor the data segment phase while relaxing, to know whether
+     the relro has been handled or not.
+
+     I think we probably need to record more information about data
+     segment or alignments in the future, to make sure it is safe
+     to doing relaxations.  */
+  enum phase_enum *phase = &(expld.dataseg.phase);
+  bfd_elf${ELFSIZE}_riscv_set_data_segment_info (&link_info, (int *) phase);
+
   do
     {
       ldelf_map_segments (need_layout);
diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
index f3ff95c..e6f8c0d 100644
--- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
+++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
@@ -121,6 +121,9 @@  if [istarget "riscv*-*-*"] {
  return
     }

+    run_dump_test "relro-relax-lui"
+    run_dump_test "relro-relax-pcrel"
+
     set abis [list rv32gc ilp32 [riscv_choose_ilp32_emul] rv64gc lp64 [riscv_choose_lp64_emul]]
     foreach { arch abi emul } $abis {
  # This checks whether our linker scripts handle __global_pointer$
diff --git a/ld/testsuite/ld-riscv-elf/relro-relax-lui.d b/ld/testsuite/ld-riscv-elf/relro-relax-lui.d
new file mode 100644
index 0000000..d29aa9d
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/relro-relax-lui.d
@@ -0,0 +1,12 @@ 
+#source: relro-relax-lui.s
+#ld: -zrelro --relax
+#objdump: -d -Mno-aliases
+
+.*:[  ]+file format .*
+
+
+Disassembly of section .text:
+
+0+[0-9a-f]+ <_start>:
+.*:[  ]+[0-9a-f]+[  ]+lui[  ]+.*
+.*:[  ]+[0-9a-f]+[  ]+addi[  ]+.*<SymbolRodata>
diff --git a/ld/testsuite/ld-riscv-elf/relro-relax-lui.s b/ld/testsuite/ld-riscv-elf/relro-relax-lui.s
new file mode 100644
index 0000000..f284187
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/relro-relax-lui.s
@@ -0,0 +1,15 @@ 
+ .section .rodata
+ .align 10
+ .globl SymbolRodata
+ .set SymbolRodata, . + 0x1800
+ .word 0x0
+
+
+ .section .init_array
+        .word 0x0
+
+ .text
+ .globl _start
+_start:
+ lui a0, %hi (SymbolRodata)
+ addi a0, a0, %lo (SymbolRodata)
diff --git a/ld/testsuite/ld-riscv-elf/relro-relax-pcrel.d b/ld/testsuite/ld-riscv-elf/relro-relax-pcrel.d
new file mode 100644
index 0000000..28e11b3
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/relro-relax-pcrel.d
@@ -0,0 +1,12 @@ 
+#source: relro-relax-pcrel.s
+#ld: -zrelro --relax
+#objdump: -d -Mno-aliases
+
+.*:[  ]+file format .*
+
+
+Disassembly of section .text:
+
+0+[0-9a-f]+ <_start>:
+.*:[  ]+[0-9a-f]+[  ]+auipc[  ]+.*
+.*:[  ]+[0-9a-f]+[  ]+addi[  ]+.*<SymbolRodata>
diff --git a/ld/testsuite/ld-riscv-elf/relro-relax-pcrel.s b/ld/testsuite/ld-riscv-elf/relro-relax-pcrel.s
new file mode 100644
index 0000000..4a88287
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/relro-relax-pcrel.s
@@ -0,0 +1,14 @@ 
+ .section .rodata
+ .align 10
+ .globl SymbolRodata
+ .set SymbolRodata, . + 0x1800
+ .word 0x0
+
+
+ .section .init_array
+        .word 0x0
+
+ .text
+ .globl _start
+_start:
+ lla a0, SymbolRodata