Hi Lifang,

thanks for reporting this.  I'm really curious how this happened, so I spend some times to research how DATA_SEGMENT_ALIGN and DATA_SEGMENT_RELRO_END work in the current GNU linker.  Consider the binutils doc,

* DATA_SEGMENT_ALIGN(maxpagesize, commonpagesize) =
[maxpagesize == commonpagesize]
(ALIGN(maxpagesize) + (. & (maxpagesize - 1)))
[maxpagesize != commonpagesize]
(ALIGN(maxpagesize)
 + ((. + commonpagesize - 1) & (maxpagesize - commonpagesize)))

* DATA_SEGMENT_RELRO_END(offset, exp)
When ‘-z relro’ option is not present, DATA_SEGMENT_RELRO_END does nothing, otherwise DATA_SEGMENT_ALIGN is padded so that exp + offset is aligned to the commonpagesize argument given to DATA_SEGMENT_ALIGN.


I only consider the default linker script here.  Besides, I do some minor changes to your original example, to see how the DATA_*_ALIGN work.

nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ cat tmp.s
        .option nopic
        .section        .rodata
        .align  10
        .globl hello_rodata
hello_rodata:
        .zero   0x10
        .section .init_array
        .globl padding_init_array
padding_init_array:
        .zero 0xfc
        .text
        .align  1
        .globl  main
main:
        lui     a5,%hi(hello_rodata)
        addi    a5,a5,%lo(hello_rodata)
nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ ~/binutils-dev/build-linux64-upstream/build-install/bin/riscv64-unknown-linux-gnu-as tmp.s -o tmp.o

Therefore, consider four cases as follows,

1. norelro, maxpagesize=0x1000, commonpagesize=0x1000

nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ ~/binutils-dev/build-linux64-upstream/build-install/bin/riscv64-unknown-linux-gnu-ld -z norelro -z max-page-size=0x1000 -z common-page-size=0x1000 tmp.o -M > map1
nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ cat map1
...
.rodata         0x0000000000010400       0x10
 *(.rodata .rodata.* .gnu.linkonce.r.*)
 .rodata        0x0000000000010400       0x10 tmp.o
                0x0000000000010400                hello_rodata
...
0x0000000000011410                . = DATA_SEGMENT_ALIGN (CONSTANT (MAXPAGESIZE), CONSTANT (COMMONPAGESIZE))
...
.init_array     0x0000000000011410       0xfc
...
0x000000000001150c                . = DATA_SEGMENT_RELRO_END (., 0x0)
.data           0x000000000001150c        0x0
...

DATA_SEGMENT_ALIGN should be
(ALIGN(0x1000) + (0x10410 & (0x1000 - 1)))
= 0x11000 + (0x10410 & 0xfff)
= 0x11410
And DATA_SEGMENT_RELRO_END = 0x11410 + 0xfc = 0x1150c.
So we need to reserve at most "MAXPAGESIZE" size.

2. relro, maxpagesize=0x1000, commonpagesize=0x1000

nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ ~/binutils-dev/build-linux64-upstream/build-install/bin/riscv64-unknown-linux-gnu-ld -z relro -z max-page-size=0x1000 -z common-page-size=0x1000 tmp.o -M > map2
nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ cat map2
...
.rodata         0x0000000000010400       0x10
 *(.rodata .rodata.* .gnu.linkonce.r.*)
 .rodata        0x0000000000010400       0x10 tmp.o
                0x0000000000010400                hello_rodata
...
0x0000000000011f04                . = DATA_SEGMENT_ALIGN (CONSTANT (MAXPAGESIZE), CONSTANT (COMMONPAGESIZE))
...
.init_array     0x0000000000011f04       0xfc
...
0x0000000000012000                . = DATA_SEGMENT_RELRO_END (., 0x0)
...

This is same as above, but since "-z relro",
DATA_SEGMENT_RELRO_END = ALIGN(0x1150c, 0x1000) = 0x12000
DATA_SEGMENT_ALIGN = 0x12000 - 0xfc = 0x11f04
So we need to reserve at most "MAXPAGESIZE + COMMONPAGESIZE" size = "2*MAXPAGESIZE".

3. norelro, maxpagesize=0x10000, commonpagesize=0x2000

nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ ~/binutils-dev/build-linux64-upstream/build-install/bin/riscv64-unknown-linux-gnu-ld -z norelro -z max-page-size=0x10000 -z common-page-size=0x2000 tmp.o -M > map3
nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ cat map3
...
.rodata         0x0000000000010400       0x10
 *(.rodata .rodata.* .gnu.linkonce.r.*)
 .rodata        0x0000000000010400       0x10 tmp.o
                0x0000000000010400                hello_rodata
...
0x0000000000020410                . = DATA_SEGMENT_ALIGN (CONSTANT (MAXPAGESIZE), CONSTANT (COMMONPAGESIZE))
...
.init_array     0x0000000000020410       0xfc
...
0x000000000002050c                . = DATA_SEGMENT_RELRO_END (., 0x0)
...

DATA_SEGMENT_ALIGN =
(ALIGN(0x10000) + (0x10410 & (0x2000 - 1)))
= 0x20000 + (0x10410 & 0x1fff)
= 0x20410
DATA_SEGMENT_RELRO_END = 0x20410 + 0xfc = 0x2050c.
So we need to reserve at most "MAXPAGESIZE" size.

4. relro, maxpagesize=0x10000, commonpagesize=0x2000

nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ ~/binutils-dev/build-linux64-upstream/build-install/bin/riscv64-unknown-linux-gnu-ld -z relro -z max-page-size=0x10000 -z common-page-size=0x2000 tmp.o -M > map4
nelson@LAPTOP-QFSGI1F2:~/test/pr27566$ cat map4
...
.rodata         0x0000000000010400       0x10
 *(.rodata .rodata.* .gnu.linkonce.r.*)
 .rodata        0x0000000000010400       0x10 tmp.o
                0x0000000000010400                hello_rodata
...
0x0000000000021f04                . = DATA_SEGMENT_ALIGN (CONSTANT (MAXPAGESIZE), CONSTANT (COMMONPAGESIZE))
...
.init_array     0x0000000000021f04       0xfc
...
0x0000000000022000                . = DATA_SEGMENT_RELRO_END (., 0x0)
...

DATA_SEGMENT_RELRO_END = ALIGN(0x20410, 0x2000) = 0x22000
DATA_SEGMENT_ALIGN = 0x22000 - 0xfc = 0x21f04
So we need to reserve at most "MAXPAGESIZE + COMMONPAGESIZE" size.

Therefore, when we are doing the LUI and PCREL relaxations (GP to symbol or c.lui to symbol, must cross the DATA_SEGMENT),

* If "-z relro" isn't set, then we need to reserve at most "MAXPAGESIZE" for the padding of DATA_SEGMENT_ALIGN.

* If "-z relro" is set, then we need to reserve at most "MAXPAGESIZE + COMMONPAGESIZE" for the padding of DATA_SEGMENT_ALIGN and DATA_SEGMENT_RELRO_END.

I only see the related checks when we are doing the LUI to C.LUI relaxation.  If we want to ensure safety, then we need to do the above checks both for the LUI and PCREL relaxations, when the range crosses the DATA_SEGMENT.  But consider the code size improvement, we might also need to consider the "MAXPAGESIZE" for noreloc (or "MAXPAGESIZE + COMMONPAGESIZE" for relro) when relaxing LUI/PCREL RO symbol to GPREL, that will cause the current code size worst.

I'm not sure if the safest approach is needed at the moment, because the embedded target (norelro) is currently not reporting any related errors.  Therefore, I would suggest that - for the "-z relro", consider the "MAXPAGESIZE + COMMONPAGESIZE" size when doing the LUI and PCREL relaxations, and the target symbol is placed in the RO-data section.  And maybe we could also change the size of "2*MAXPAGESIZE" to "MAXPAGESIZE + COMMONPAGESIZE" for the C.LUI relaxation check.  Though Lifang's patch, which disable the LUI relaxation when relro (only for linux toolchain), should works, but I prefer to use a unified method (consider the PAGESIZE) to check all LUI, C.LUI and PCREL relaxations.

Hi Nelson,

I agree with you, the previous patch is not a best choice.

I have do some debug today.
I add a print here:

===========================================
diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 364d67b..e774cff 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -4247,14 +4247,23 @@ _bfd_riscv_relax_lui (bfd *abfd,
        max_alignment = (bfd_vma) 1 << sym_sec->output_section->alignment_power;
     }

+  printf ("gp: %lx, sym val: %lx\n", gp, symval);
   /* Is the reference in range of x0 or gp?
      Valid gp range conservatively because of alignment issue.  */
   if (undefined_weak
       || (VALID_ITYPE_IMM (symval)
          || (symval >= gp
==============================================

And I got a result:
-----------------------------------------------
xialf@magics:build-linux$ ./ld/ld-new -o a a.o -e main  -z norelro
gp: 11d90, sym val: 11c10
gp: 11d90, sym val: 11c10
xialf@magics:build-linux$ readelf -s a |grep global
     5: 0000000000011d90     0 NOTYPE  GLOBAL DEFAULT  ABS __global_pointer$
-----------------------------------------------

From this, GP is not changed, that means DATA_SEGMENT_ALIGN  has been finished while relaxing.Therefore we can just consider about COMMON_PAGESIZE, not MAX_PAGESIZE+COMMON_PAGESIZE.

=====================================================
For -z relro

xialf@magics:build-linux$ ./ld/ld-new -o a a.o -e main  -z relro
gp: 12800, sym val: 11c10
gp: 12800, sym val: 11c10
gp: 11d90, sym val: 11c10
gp: 11d90, sym val: 11c10
xialf@magics:build-linux$ readelf -s a |grep global
     5: 0000000000012800     0 NOTYPE  GLOBAL DEFAULT  ABS __global_pointer$
=======================================================
DATA_SEGMENT_RELRO_END has been aligned to 0x12000, and gp is 0x12800.
=======================================================
common-page-size=0x4000

xialf@magics:build-linux$ ./ld/ld-new -o a a.o -e main -z common-page-size=0x4000 -z relro
gp: 14800, sym val: 11c10
gp: 14800, sym val: 11c10
gp: 11d90, sym val: 11c10
gp: 11d90, sym val: 11c10
xialf@magics:build-linux$ readelf -s a |grep global
     5: 0000000000014800     0 NOTYPE  GLOBAL DEFAULT  ABS __global_pointer$
========================================================
DATA_SEGMENT_RELRO_END has been aligned to 0x14000, gp is 0x14800.

It is not check the imm with (MAX_PAGESIZE + COMMON_PAGE_SIZE) or (COMMON_PAGESIZE); It need to align to COMMON_PAGE_SIZE.

Can we add a symbol in the default link file:
================================================
.jcr            : { KEEP (*(.jcr)) }
  .data.rel.ro : { *(.data.rel.ro.local* .gnu.linkonce.d.rel.ro.local.*) *(.data.rel.ro .data.rel.ro.* .gnu.linkonce.d.rel.ro.*) }
  .dynamic        : { *(.dynamic) }
  . = DATA_SEGMENT_RELRO_END (0, .);
  __data_segment_relro_end = .; // IT IS NEW FROM LIFANG
  .data :
  {
================================================

checking __data_segment_relro_end in relaxing:
"-z relro": __data_segment_relro_end  must be aligned to COMMON_PAGESIZE;
"-z norelro": ignore __data_segment_relro_end

The c.lui case was fixed because a bug turned up in regression testing, and Ilya Diachkov debugged it and wrote a patch.  The tail end of the thread is here
https://sourceware.org/pipermail/binutils/2019-July/107702.html
Looks like I missed the fact that other cases needed a similar fix.  And I didn't notice the MAXPAGESIZE versus COMMONPAGESIZE issue.  it does look like fixing all 3 places to use MAXPAGESIZE+COMMONPAGESIZE is the correct fix.

GP is computed so that __DATA_BEGIN__+0x800 is the minimum value, so that it can never overlap rodata section, but apparently that isn't enough in this case.  Maybe gp or __DATA__BEGIN__ aren't being updated until after we use them because of how relaxation works.

Yes, we can add symbols to the linker script if necessary, but __DATA_BEGIN__ is alraedy in the right place and we should be able to use that.  Except it looks like it might not work in this case.

Jim's assumption is right, the gp won't overlap the rodata.  But it could overlap the symbol defined in the rodata, and it's value plus a constant.

    .align  3
    .globl hello_rodata
    .set hello_rodata, . + 0x1800
.Lpadding:
    .zero   128

Lifang's assumption is also correct.  I had traced the source code and did some experiments, we should get the symbol values that have considered the DATA_SEGMENT_ALIGN when relaxing.  But sometimes, they don't consider the DATA_SEGMENT_RELRO_END.  It causes that we may get the unexpected symbol values when relaxing.  After the following change, Lifang's example is fixed,


 ld/ldlang.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 629be32..53513de 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -6378,7 +6378,8 @@ lang_size_relro_segment_1 (seg_align_type *seg)
 }

 static bfd_boolean
-lang_size_relro_segment (bfd_boolean *relax, bfd_boolean check_regions)
+lang_size_relro_segment (bfd_boolean *relax ATTRIBUTE_UNUSED,
+                        bfd_boolean check_regions)
 {
   bfd_boolean do_reset = FALSE;
   bfd_boolean do_data_relro;
@@ -6399,7 +6400,7 @@ lang_size_relro_segment (bfd_boolean *relax, bfd_boolean check_regions)
   if (do_data_relro)
     {
       lang_reset_memory_regions ();
-      one_lang_size_sections_pass (relax, check_regions);
+      one_lang_size_sections_pass (FALSE, check_regions);

       /* Assignments to dot, or to output section address in a user
         script have increased padding over the original.  Revert.  */
       if (do_data_relro && expld.dataseg.relro_end > data_relro_end)
        {
          expld.dataseg.base = data_initial_base;;
          do_reset = TRUE;
        }
    }

  if (!do_data_relro && lang_size_segment (&expld.dataseg))
    do_reset = TRUE;


An interesting thing is that - although we have update the expld.dataseg in the lang_size_relro_segment_1, but the symbol values are updated until the lang_size_segment, and we will do the relaxations again in the one_lang_size_sections_pass before that point.  But is looks strange to me, why we will do the same relax pass again by the while loop in the lang_relax_sections, but we still do it again here, with the symbols that don't consider the relro_end?

However, I think we still need to consider the maxpagesize+commonpagesize when the gp (or pc of c.lui) and target symbols cross the DATA_SEGMENT_ALIGN.  Since relaxation might cause the data segment gap larger, but we should create another new testcase to describe the problem.  Not sure how to check if the symbols cross the DATA_SEGMENT_ALIGN, we probably need to get the value of expdl.dataseg.base, and store it to the link_info.

The master branch has been updated by Nelson Chu <nelsonc1225@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=ef9d25656226c46406293a70a81e564640973662

commit ef9d25656226c46406293a70a81e564640973662
Author: Nelson Chu <nelson.chu@sifive.com>
Date:   Wed May 12 08:26:33 2021 -0700

    RISC-V: PR27566, Do not relax when data segment phase is exp_seg_relro_adjust.
    
    2021-05-31  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.

Marked as resolved and fixed.

Hi we get this error a again.....
-------------
.text

hello:
        .rept 6000
        lla a0, ARCHOR0
        .endr


.section .rodata
        .set ARCHOR0, . + 4598
        .fill 100, 4, 0x12345678

.data
.align 3
world:
    .rept 860
    .long 0x1000
    .endr
-------------
build command:

binutils/build/gas/as-new -o 1.o 1.s -march=rv32gc -mabi=ilp32d
binutils/build/ld/ld-new -o 1 1.o -e hello -m elf32lriscv

we can get the error message:

1.o: in function `hello':
(.text+0x0): relocation truncated to fit: R_RISCV_GPREL_I against `ARCHOR0'
---------------------------------
It looks like the max alignment should include page size if the symbol is not defined in the same section of GP.

Any idea about this?

Reopend since it happens again...