[PATCH] bfd/RISC-V: Prevent region check failures when relaxation is not final

[PATCH] bfd/RISC-V: Prevent region check failures when relaxation is not final

[Lewis Revill]

In some circumstances for RISC-V, such as using a .align directive in a
section with a very small output memory region, we currently fail to
link. The following is an analysis of the problem:

Since the restart_relax flag was implemented (commit
ebdcad3fddf6ec21f6d4dcc702379a12718cf0c4), we have the capability to
take another complete round trip through the lang_relax_sections process
rather than simply using the `again` flag. Doing this, we delay the
relaxation of alignment directives until no more round trips are
required.

However, the final part of the lang_relax_sections process also involves
a sanity check that addresses fit within regions (os_region_check). In
the case where alignment directives - implemented as nop sleds which can
be reduced as needed - are not yet relaxed, these checks may fail
prematurely.

To fix this, this patch adds the delay_region_check flag to
bfd_link_info, which allows lang_relax_sections to prevent the sanity
check from occurring if set to true. Then, in _bfd_riscv_relax_section,
instead of simply skipping the final phase of relaxation when we are
restarting relaxation, we also set this flag to true, preventing
premature errors.

bfd/

* elfnn-riscv.c (_bfd_riscv_relax_section): Set flag to delay
region check when restarting relaxation.

ld/

* ldlang.c (lang_relax_sections): Account for delay_region_check
during final sizing of sections.
---
 bfd/ChangeLog                                   |  5 +++++
 bfd/elfnn-riscv.c                               | 13 +++++++++++--
 include/bfdlink.h                               |  6 ++++++
 ld/ChangeLog                                    |  5 +++++
 ld/ldlang.c                                     |  4 +++-
 ld/testsuite/ld-riscv-elf/align-small-region.d  | 12 ++++++++++++
 ld/testsuite/ld-riscv-elf/align-small-region.ld | 12 ++++++++++++
 ld/testsuite/ld-riscv-elf/align-small-region.s  |  7 +++++++
 8 files changed, 61 insertions(+), 3 deletions(-)
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.d
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.ld
 create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.s

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 2a08ff7cfb4..892eb7a322b 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,8 @@
+2021-10-01  Lewis Revill  <lewis.revill@embecosm.com>
+
+ * elfnn-riscv.c (_bfd_riscv_relax_section): Set flag to delay
+ region check when restarting relaxation.
+
 2021-09-27  Nick Alcock  <nick.alcock@oracle.com>

  * configure: Regenerate.
diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 2e8df72fa2a..e3f247a1c39 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -4711,13 +4711,22 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
       || sec->reloc_count == 0
       || (info->disable_target_specific_optimizations
   && info->relax_pass < 2)
-      || (htab->restart_relax
-  && 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;

+  if (htab->restart_relax && info->relax_pass == 3)
+    {
+      /* We are restarting the entire process of mapping segments
(including
+         relaxation passes) again. This process would usually include a
sanity
+         check that addresses lie within their region, however in this
case we
+         are not yet done with relaxation and are delaying relaxation of
+         alignment directives. As such we should also delay this check.  */
+      info->delay_region_check = true;
+      return true;
+    }
+
   riscv_init_pcgp_relocs (&pcgp_relocs);

   /* Read this BFD's relocs if we haven't done so already.  */
diff --git a/include/bfdlink.h b/include/bfdlink.h
index 566529ee644..c392d3eb341 100644
--- a/include/bfdlink.h
+++ b/include/bfdlink.h
@@ -625,6 +625,12 @@ struct bfd_link_info
      relaxation returning true in *AGAIN.  */
   int relax_trip;

+  /* Whether to check that addresses fit within regions at the end of
+     the lang_relax_sections pass. Set to TRUE by bfd_relax_section if
+     section sizes are not necessarily final at the end of this
+     particular pass through lang_relax_sections.  */
+  bool delay_region_check;
+
   /* > 0 to treat protected data defined in the shared library as
      reference external.  0 to treat it as internal.  -1 to let
      backend to decide.  */
diff --git a/ld/ChangeLog b/ld/ChangeLog
index 808191bd14e..f8cf6b2de3a 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,3 +1,8 @@
+2021-10-01  Lewis Revill  <lewis.revill@embecosm.com>
+
+ * ldlang.c (lang_relax_sections): Account for delay_region_check
+ during final sizing of sections.
+
 2021-09-30  Dimitar Dimitrov  <dimitar@dinux.eu>

  * scripttempl/pru.sc (.resource_table): Align the output
diff --git a/ld/ldlang.c b/ld/ldlang.c
index bc3f8b76d35..05ad2ea7b4b 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -7681,6 +7681,8 @@ lang_find_relro_sections (void)
 void
 lang_relax_sections (bool need_layout)
 {
+  link_info.delay_region_check = false;
+
   if (RELAXATION_ENABLED)
     {
       /* We may need more than one relaxation pass.  */
@@ -7728,7 +7730,7 @@ lang_relax_sections (bool need_layout)
       /* Final extra sizing to report errors.  */
       lang_do_assignments (lang_assigning_phase_enum);
       lang_reset_memory_regions ();
-      lang_size_sections (NULL, true);
+      lang_size_sections (NULL, !link_info.delay_region_check);
     }
 }

diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.d
b/ld/testsuite/ld-riscv-elf/align-small-region.d
new file mode 100644
index 00000000000..8b9a0ab47eb
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.d
@@ -0,0 +1,12 @@
+#source: align-small-region.d
+#as: -march=rv32i
+#ld: -melf32lriscv --relax -Talign-small-region.ld --defsym=_start=0x100
+#objdump: -d
+
+.*:       file format .*
+
+Disassembly of section \.entry:
+
+00000000 <_reset>:
+       0: 6f 00 00 10   j       0x100 <_reset+0x100>
+#pass
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.ld
b/ld/testsuite/ld-riscv-elf/align-small-region.ld
new file mode 100644
index 00000000000..6a3e6638b26
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.ld
@@ -0,0 +1,12 @@
+MEMORY
+{
+ reset : ORIGIN = 0x0, LENGTH = 32
+}
+
+SECTIONS
+{
+ .entry :
+ {
+ KEEP (*(.entry))
+ } > reset
+}
diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.s
b/ld/testsuite/ld-riscv-elf/align-small-region.s
new file mode 100644
index 00000000000..ace81d640ed
--- /dev/null
+++ b/ld/testsuite/ld-riscv-elf/align-small-region.s
@@ -0,0 +1,7 @@
+ .section .entry, "xa"
+ .align 5
+ .globl _reset
+ .type _reset, @function
+_reset:
+ tail _start
+ .size _reset, . - _reset
-- 
2.25.1

Re: [PATCH] bfd/RISC-V: Prevent region check failures when relaxation is not final

[Nelson Chu]

Hi Lewis,

Thanks for reporting this, and have a proposed solution.  I cannot
find the bug report in the bugzilla, so I create one:
https://sourceware.org/bugzilla/show_bug.cgi?id=28410.  I have some
comments, but not sure how to do, maybe we can discuss the details
there.

Hi Nick, Hi Alan, Hi Jim, Hi Palmer, Hi Andrew,

I need your suggestions and helps if you are free recently.

Thank you very much
Nelson

On Fri, Oct 1, 2021 at 11:35 PM Lewis Revill <lewis.revill@embecosm.com> wrote:
>
> In some circumstances for RISC-V, such as using a .align directive in a
> section with a very small output memory region, we currently fail to
> link. The following is an analysis of the problem:
>
> Since the restart_relax flag was implemented (commit
> ebdcad3fddf6ec21f6d4dcc702379a12718cf0c4), we have the capability to
> take another complete round trip through the lang_relax_sections process
> rather than simply using the `again` flag. Doing this, we delay the
> relaxation of alignment directives until no more round trips are
> required.
>
> However, the final part of the lang_relax_sections process also involves
> a sanity check that addresses fit within regions (os_region_check). In
> the case where alignment directives - implemented as nop sleds which can
> be reduced as needed - are not yet relaxed, these checks may fail
> prematurely.
>
> To fix this, this patch adds the delay_region_check flag to
> bfd_link_info, which allows lang_relax_sections to prevent the sanity
> check from occurring if set to true. Then, in _bfd_riscv_relax_section,
> instead of simply skipping the final phase of relaxation when we are
> restarting relaxation, we also set this flag to true, preventing
> premature errors.
>
> bfd/
>
> * elfnn-riscv.c (_bfd_riscv_relax_section): Set flag to delay
> region check when restarting relaxation.
>
> ld/
>
> * ldlang.c (lang_relax_sections): Account for delay_region_check
> during final sizing of sections.
> ---
>  bfd/ChangeLog                                   |  5 +++++
>  bfd/elfnn-riscv.c                               | 13 +++++++++++--
>  include/bfdlink.h                               |  6 ++++++
>  ld/ChangeLog                                    |  5 +++++
>  ld/ldlang.c                                     |  4 +++-
>  ld/testsuite/ld-riscv-elf/align-small-region.d  | 12 ++++++++++++
>  ld/testsuite/ld-riscv-elf/align-small-region.ld | 12 ++++++++++++
>  ld/testsuite/ld-riscv-elf/align-small-region.s  |  7 +++++++
>  8 files changed, 61 insertions(+), 3 deletions(-)
>  create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.d
>  create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.ld
>  create mode 100644 ld/testsuite/ld-riscv-elf/align-small-region.s
>
> diff --git a/bfd/ChangeLog b/bfd/ChangeLog
> index 2a08ff7cfb4..892eb7a322b 100644
> --- a/bfd/ChangeLog
> +++ b/bfd/ChangeLog
> @@ -1,3 +1,8 @@
> +2021-10-01  Lewis Revill  <lewis.revill@embecosm.com>
> +
> + * elfnn-riscv.c (_bfd_riscv_relax_section): Set flag to delay
> + region check when restarting relaxation.
> +
>  2021-09-27  Nick Alcock  <nick.alcock@oracle.com>
>
>   * configure: Regenerate.
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index 2e8df72fa2a..e3f247a1c39 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -4711,13 +4711,22 @@ _bfd_riscv_relax_section (bfd *abfd, asection *sec,
>        || sec->reloc_count == 0
>        || (info->disable_target_specific_optimizations
>    && info->relax_pass < 2)
> -      || (htab->restart_relax
> -  && 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;
>
> +  if (htab->restart_relax && info->relax_pass == 3)
> +    {
> +      /* We are restarting the entire process of mapping segments
> (including
> +         relaxation passes) again. This process would usually include a
> sanity
> +         check that addresses lie within their region, however in this
> case we
> +         are not yet done with relaxation and are delaying relaxation of
> +         alignment directives. As such we should also delay this check.  */
> +      info->delay_region_check = true;
> +      return true;
> +    }
> +
>    riscv_init_pcgp_relocs (&pcgp_relocs);
>
>    /* Read this BFD's relocs if we haven't done so already.  */
> diff --git a/include/bfdlink.h b/include/bfdlink.h
> index 566529ee644..c392d3eb341 100644
> --- a/include/bfdlink.h
> +++ b/include/bfdlink.h
> @@ -625,6 +625,12 @@ struct bfd_link_info
>       relaxation returning true in *AGAIN.  */
>    int relax_trip;
>
> +  /* Whether to check that addresses fit within regions at the end of
> +     the lang_relax_sections pass. Set to TRUE by bfd_relax_section if
> +     section sizes are not necessarily final at the end of this
> +     particular pass through lang_relax_sections.  */
> +  bool delay_region_check;
> +
>    /* > 0 to treat protected data defined in the shared library as
>       reference external.  0 to treat it as internal.  -1 to let
>       backend to decide.  */
> diff --git a/ld/ChangeLog b/ld/ChangeLog
> index 808191bd14e..f8cf6b2de3a 100644
> --- a/ld/ChangeLog
> +++ b/ld/ChangeLog
> @@ -1,3 +1,8 @@
> +2021-10-01  Lewis Revill  <lewis.revill@embecosm.com>
> +
> + * ldlang.c (lang_relax_sections): Account for delay_region_check
> + during final sizing of sections.
> +
>  2021-09-30  Dimitar Dimitrov  <dimitar@dinux.eu>
>
>   * scripttempl/pru.sc (.resource_table): Align the output
> diff --git a/ld/ldlang.c b/ld/ldlang.c
> index bc3f8b76d35..05ad2ea7b4b 100644
> --- a/ld/ldlang.c
> +++ b/ld/ldlang.c
> @@ -7681,6 +7681,8 @@ lang_find_relro_sections (void)
>  void
>  lang_relax_sections (bool need_layout)
>  {
> +  link_info.delay_region_check = false;
> +
>    if (RELAXATION_ENABLED)
>      {
>        /* We may need more than one relaxation pass.  */
> @@ -7728,7 +7730,7 @@ lang_relax_sections (bool need_layout)
>        /* Final extra sizing to report errors.  */
>        lang_do_assignments (lang_assigning_phase_enum);
>        lang_reset_memory_regions ();
> -      lang_size_sections (NULL, true);
> +      lang_size_sections (NULL, !link_info.delay_region_check);
>      }
>  }
>
> diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.d
> b/ld/testsuite/ld-riscv-elf/align-small-region.d
> new file mode 100644
> index 00000000000..8b9a0ab47eb
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/align-small-region.d
> @@ -0,0 +1,12 @@
> +#source: align-small-region.d
> +#as: -march=rv32i
> +#ld: -melf32lriscv --relax -Talign-small-region.ld --defsym=_start=0x100
> +#objdump: -d
> +
> +.*:       file format .*
> +
> +Disassembly of section \.entry:
> +
> +00000000 <_reset>:
> +       0: 6f 00 00 10   j       0x100 <_reset+0x100>
> +#pass
> diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.ld
> b/ld/testsuite/ld-riscv-elf/align-small-region.ld
> new file mode 100644
> index 00000000000..6a3e6638b26
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/align-small-region.ld
> @@ -0,0 +1,12 @@
> +MEMORY
> +{
> + reset : ORIGIN = 0x0, LENGTH = 32
> +}
> +
> +SECTIONS
> +{
> + .entry :
> + {
> + KEEP (*(.entry))
> + } > reset
> +}
> diff --git a/ld/testsuite/ld-riscv-elf/align-small-region.s
> b/ld/testsuite/ld-riscv-elf/align-small-region.s
> new file mode 100644
> index 00000000000..ace81d640ed
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/align-small-region.s
> @@ -0,0 +1,7 @@
> + .section .entry, "xa"
> + .align 5
> + .globl _reset
> + .type _reset, @function
> +_reset:
> + tail _start
> + .size _reset, . - _reset
> --
> 2.25.1

Re: [PATCH] bfd/RISC-V: Prevent region check failures when relaxation is not final

[Nick Clifton]

Hi Nelson,

>> To fix this, this patch adds the delay_region_check flag to
>> bfd_link_info,

I dislike changing generic code just to fix a problem with one target.
If there is a method that only involves changing risc-v specific code
then I would prefer that.  (This is option 3 in PR 28410, right ?)

Cheers
   Nick

Re: [PATCH] bfd/RISC-V: Prevent region check failures when relaxation is not final

[Nelson Chu]

Hi Nick,

On Tue, Oct 5, 2021 at 10:36 PM Nick Clifton <nickc@redhat.com> wrote:
>
> Hi Nelson,
>
> >> To fix this, this patch adds the delay_region_check flag to
> >> bfd_link_info,
>
> I dislike changing generic code just to fix a problem with one target.
> If there is a method that only involves changing risc-v specific code
> then I would prefer that.  (This is option 3 in PR 28410, right ?)

Yes, that's option 3 in PR 28410, it is more complicated, but we don't
need to modify any generic code.  Thanks for the reply, this allows us
to know more clearly that we should try option 3 at first, as possible
as we can.

Thanks
Nelson