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

Message ID CAJd+PRNLzn-cEt5P6sK8pmL_ZBg4sXc8t+7knO+cedtiBHgKhw@mail.gmail.com
State New
Headers show
Series
  • bfd/RISC-V: Prevent region check failures when relaxation is not final
Related show

Commit Message

Lewis Revill Oct. 1, 2021, 3:35 p.m.
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

-- 
2.25.1

Comments

Nelson Chu Oct. 2, 2021, 2:54 a.m. | #1
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
H.J. Lu via Binutils Oct. 5, 2021, 2:36 p.m. | #2
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
Nelson Chu Oct. 6, 2021, 4:41 a.m. | #3
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

Patch

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