This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] RISC-V: enable lui relaxation for CODE and MERGE sections.


Hi Kito,

I have investigated the fail. It is triggered by my fix but the fix is definitely correct and the problem lays in the general relaxation mechanism and its implementation for riscv.

If you look to the end of _bfd_riscv_relax_lui, you'll find comments "Alignment might move the section forward; account for this assuming page alignment at worst." and check "VALID_RVC_LUI_IMM (RISCV_CONST_HIGH_PART (symval + ELF_MAXPAGESIZE))". I.e. the range of addresses accepted by lui is reduced to avoid errors during relaxation. It's reasonable since current implementation of relaxation runs several passes and the first/second ones may give to a symbol the smallest address while the next/last one may increase it.

Briefly saying my investigation shown that the check must be more conservative in the case when we link objects with RELRO segment. That is because the code in lang_size_relro_segment_1 (ld/ldlang.c) tries to make the end of the segment aligned by page size. It achieves this (later) by increasing the addresses of sections belonging to the segment. So if you have a symbol which address is bigger then the address of the first section of RELRO segment, the symbol's address may be shifted forward by 1 page size by lang_size_relro_segment which is called during relaxation.

In the failed test case, address of .tm_clone_table section is loaded by a lui insn in .text section. At one moment during relaxation (it's pass 0, see comments for _bfd_riscv_relax_section) the section gets address 0x1e104. So it's Ok to convert that lui insn to c.lui. However later on pass 2 all sections are moved forward and the address becomes 0x1f0f4. It would be correct address for c.lui instruction. However RELRO segment is located before tm_clone_table section and later lang_size_relro_segment makes additional alignment of the segment's end by page size so tm_clone_table gets 0x20000 address which is not suitable for c.lui and breaks the relocation processing.

Thus we have to change the condition of c.lui->lui conversion. The simplest way I see is just to harden the worst assumption in the case we have RELRO segment. This patch is attached. It heals the failure. I have tested it by dejagnu with both eabi and linux toolchains and have checked freedom-u-sdk build. Also I have verified on tests (EEMBC automotive 1.1/2.0, SPEC2006 C/C++, riscv-beebs benchmarks) that the fix does not affect code size with eabi toolchain.

Since the symval is positive I'm not sure whether we really need to check this condition in elfnn-riscv.c twice:
      && VALID_RVC_LUI_IMM (RISCV_CONST_HIGH_PART (symval))
&& VALID_RVC_LUI_IMM (RISCV_CONST_HIGH_PART (symval + ELF_MAXPAGESIZE))
so I leaved the stricter check.

The more complex approach may include address comparison of RELRO segment (its first section) and relocatable symbol and if the symbol address is bigger then use stricter conditions on the symbol address.

Best regards,
Ilia.

bfd/ChangeLog:
* elfnn-riscv.c (_bfd_riscv_relax_lui): set lui relax safety area to two pages in relro presence.

Kito Cheng wrote 2019-07-23 09:45:
Hi Ilia:

Forgot to attend my binutils configuration and git hash:
../binutils-gdb/configure  --target=riscv32-unknown-linux-gnu

commit 89356123a17c27548c7e71f4f000b1f74e551c31
Refs: {origin/master}, {origin/HEAD},
users/ARM/embedded-gdb-master-2018q4-2078-g89356123a1
Author:     GDB Administrator <gdbadmin@sourceware.org>
AuthorDate: Tue Jul 23 00:00:36 2019 +0000
Commit:     GDB Admin <brobecker@adacore.com>
CommitDate: Tue Jul 23 00:00:36 2019 +0000

   Automatic date update in version.in

On Tue, Jul 23, 2019 at 2:40 PM Kito Cheng <kito.cheng@gmail.com> wrote:

Hi Illa:

I just found a case will got `relocation truncated to fit` error on
rv32 linux with this patch.

Here is the tarball for reproduce, included minimized sysroot and
necessary objects/libraries.

`make relax` can reproduce the problem and `make norelax` for disable
relax build.

https://drive.google.com/file/d/1l-IOiK3Ay7s2mnFWYXOkHYyPFNsIV58H/view?usp=sharing


On Tue, Jun 25, 2019 at 4:55 AM Jim Wilson <jimw@sifive.com> wrote:
>
> On Fri, Jun 21, 2019 at 11:59 AM Ilia Diachkov
> <ilia.diachkov@optimitech.com> wrote:
> > This patch enables lui relaxation for CODE and MERGE sections on RISC-V.
> > It helps to reduce code size.
>
> I spotted two minor style issues.  An extra blank line where we had
> the bfd_assert call during testing.  And a missing space before an
> open paren.
>
> I rewrote the ChangeLog entry.  It should describe the actual changes
> that are being made, and not the effect of the change.  Also, it
> should be sentences that start with capital letter.
>
> I did a quick test to make sure I didn't accidentally break it and
> then committed and pushed it.
>
> Jim
---
 bfd/elfnn-riscv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 003b4f8..e618477 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -3562,11 +3562,14 @@ _bfd_riscv_relax_lui (bfd *abfd,
     }
 
   /* Can we relax LUI to C.LUI?  Alignment might move the section forward;
-     account for this assuming page alignment at worst.  */
+     account for this assuming page alignment at worst. In the presence of 
+     RELRO segment the linker aligns it by one page size, therefore sections
+     after the segment can be moved more than one page. */
+  symval += link_info->relro ? 2 * ELF_MAXPAGESIZE : ELF_MAXPAGESIZE;
+
   if (use_rvc
       && ELFNN_R_TYPE (rel->r_info) == R_RISCV_HI20
-      && VALID_RVC_LUI_IMM (RISCV_CONST_HIGH_PART (symval))
-      && VALID_RVC_LUI_IMM (RISCV_CONST_HIGH_PART (symval + ELF_MAXPAGESIZE)))
+      && VALID_RVC_LUI_IMM (RISCV_CONST_HIGH_PART (symval)))
     {
       /* Replace LUI with C.LUI if legal (i.e., rd != x0 and rd != x2/sp).  */
       bfd_vma lui = bfd_get_32 (abfd, contents + rel->r_offset);
-- 
1.8.3.1


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]