Attachment 15267 Details for Bug 31179 – proposed solution v1

[patch] proposed solution v1

0001-RISC-V-PR31179-The-SET-ADD-SUB-fix-breaks-ABI-compat.patch (text/plain), 9.91 KB, created by Nelson Chu on 2023-12-20 03:36:46 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nelson Chu

Created: 2023-12-20 03:36:46 UTC

Size: 9.91 KB

patch

obsolete

>From a9eb35c3ee0eb265674d7858a8b4ec24928eae6e Mon Sep 17 00:00:00 2001
>From: Nelson Chu <nelson@rivosinc.com>
>Date: Wed, 20 Dec 2023 10:37:41 +0800
>Subject: [PATCH] RISC-V: PR31179, The SET/ADD/SUB fix breaks ABI compatibility
> with 2.41 objects
>
>* Problematic fix commit,
>2029e13917d53d2289d3ebb390c4f40bd2112d21
>RISC-V: Clarify the behaviors of SET/ADD/SUB relocations
>
>* Bugzilla,
>https://sourceware.org/bugzilla/show_bug.cgi?id=31179#c5
>
>The addend of SUB_ULEB128 should be zero if using .uleb128, but we make it
>non-zero by accident in assembler before.  This causes troubles by applying
>the above commit, since the calculation is changed to support .reloc *SUB*
>relocations with non-zero addend.
>
>We encourage people to rebuild their stuff to get the non-zero addend of
>SUB_ULEB128, but that might need some times, so report warnings to inform
>people need to rebuild their stuff if --check-uleb128 is enabled.
>
>Since the failed .reloc cases for ADD/SET/SUB/ULEB128 are rarely to use,
>it may acceptable that stop supproting them until people rebuld their stuff,
>maybe half-year or a year later.  Or maybe we should teach people that don't
>write the .reloc R_RISCV_SUB* with non-zero constant, and then report
>warnings/errors in assembler.
>
>bfd/
>	* elfnn-riscv.c (perform_relocation): Ignore the non-zero addend of
>	R_RISCV_SUB_ULEB128.
>	(riscv_elf_relocate_section): Report warnings to inform people need
>	to rebuild their stuff if --check-uleb128 is enabled.  So that can
>	get the right non-zero addend of R_RISCV_SUB_ULEB128.
>	* elfxx-riscv.h (struct riscv_elf_params): Added bool check_uleb128.
>ld/
>	* emultempl/riscvelf.em: Added linker risc-v target options,
>	--[no-]check-uleb128, to enable/disable checking if the addend of
>	uleb128 is non-zero or not.  So that people will know they need to
>	rebuild the objects with new binutils to get the right zero addend
>	of SUB_ULEB128 relocation, or they may get troubles if using .reloc.
>	* ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated.
>	* ld/testsuite/ld-riscv-elf/pr31179*: New test cases.
>---
> bfd/elfnn-riscv.c                          | 44 +++++++++++++++-------
> bfd/elfxx-riscv.h                          |  2 +
> gas/NEWS                                   |  5 +++
> ld/emultempl/riscvelf.em                   | 17 ++++++++-
> ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp |  2 +
> ld/testsuite/ld-riscv-elf/pr31179-r.d      | 10 +++++
> ld/testsuite/ld-riscv-elf/pr31179.d        | 11 ++++++
> ld/testsuite/ld-riscv-elf/pr31179.s        | 13 +++++++
> 8 files changed, 89 insertions(+), 15 deletions(-)
> create mode 100644 ld/testsuite/ld-riscv-elf/pr31179-r.d
> create mode 100644 ld/testsuite/ld-riscv-elf/pr31179.d
> create mode 100644 ld/testsuite/ld-riscv-elf/pr31179.s
>
>diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
>index 042266e791b..6a1f1d9884f 100644
>--- a/bfd/elfnn-riscv.c
>+++ b/bfd/elfnn-riscv.c
>@@ -1735,19 +1735,9 @@ perform_relocation (const reloc_howto_type *howto,
>   if (howto->pc_relative)
>     value -= sec_addr (input_section) + rel->r_offset;
> 
>-  switch (ELFNN_R_TYPE (rel->r_info))
>-    {
>-    case R_RISCV_SUB6:
>-    case R_RISCV_SUB8:
>-    case R_RISCV_SUB16:
>-    case R_RISCV_SUB32:
>-    case R_RISCV_SUB64:
>-    case R_RISCV_SUB_ULEB128:
>-      value -= rel->r_addend;
>-      break;
>-    default:
>-      value += rel->r_addend;
>-    }
>+  /* PR31179, ignore the non-zero addend of R_RISCV_SUB_ULEB128.  */
>+  if (ELFNN_R_TYPE (rel->r_info) != R_RISCV_SUB_ULEB128)
>+    value += rel->r_addend;
> 
>   switch (ELFNN_R_TYPE (rel->r_info))
>     {
>@@ -2530,9 +2520,35 @@ riscv_elf_relocate_section (bfd *output_bfd,
> 	  if (uleb128_set_rel != NULL
> 	      && uleb128_set_rel->r_offset == rel->r_offset)
> 	    {
>-	      relocation = uleb128_set_vma - relocation + uleb128_set_rel->r_addend;
>+	      relocation = uleb128_set_vma - relocation
>+			   + uleb128_set_rel->r_addend;
> 	      uleb128_set_vma = 0;
> 	      uleb128_set_rel = NULL;
>+
>+	      /* PR31179, the addend of SUB_ULEB128 should be zero if using
>+		 .uleb128, but we make it non-zero by accident in assembler,
>+		 so just ignore it in perform_relocation, and make assembler
>+		 continue doing the right thing.  Don't reset the addend of
>+		 SUB_ULEB128 to zero here since it will break the --emit-reloc,
>+		 even though the non-zero addend is unexpected.
>+
>+		 We encourage people to rebuild their stuff to get the
>+		 non-zero addend of SUB_ULEB128, but that might need some
>+		 times, so report warnings to inform people need to rebuild
>+		 if --check-uleb128 is enabled.  However, since the failed
>+		 .reloc cases for ADD/SET/SUB/ULEB128 are rarely to use, it
>+		 may acceptable that stop supproting them until people rebuld
>+		 their stuff, maybe half-year or one year later.  I believe
>+		 this might be the least harmful option that we should go.
>+
>+		 Or maybe we should teach people that don't write the
>+		 .reloc R_RISCV_SUB* with non-zero constant, and report
>+		 warnings/errors in assembler.  */
>+	      if (htab->params->check_uleb128
>+		  && rel->r_addend != 0)
>+		_bfd_error_handler (_("%pB: warning: R_RISCV_SUB_ULEB128 with"
>+				      " non-zero addend, please rebuild by new"
>+				      " binutils"), input_bfd);
> 	    }
> 	  else
> 	    {
>diff --git a/bfd/elfxx-riscv.h b/bfd/elfxx-riscv.h
>index abcb409bd78..6959ec1b61f 100644
>--- a/bfd/elfxx-riscv.h
>+++ b/bfd/elfxx-riscv.h
>@@ -31,6 +31,8 @@ struct riscv_elf_params
> {
>   /* Whether to relax code sequences to GP-relative addressing.  */
>   bool relax_gp;
>+  /* Whether to check if SUB_ULEB128 relocation with non-zero addend.  */
>+  bool check_uleb128;
> };
> 
> extern void riscv_elf32_set_options (struct bfd_link_info *,
>diff --git a/gas/NEWS b/gas/NEWS
>index 4c8d5946690..8647d9699ad 100644
>--- a/gas/NEWS
>+++ b/gas/NEWS
>@@ -3,6 +3,11 @@
> * On RISC-V macro instructions expanding to AUIPC and a load, store, or branch
>   no longer accept x0 as an intermediate and/or destination register.
> 
>+* On RISC-V, add ld target option --[no-]check-uleb128.  Should rebuild the
>+  objects by new assembler if enabling the option and get warnings, since
>+  the non-zero addend of SUB_ULEB128 shouldn't be generated from .uleb128
>+  directives.
>+
> * Add support for Reliability, Availability and Serviceability extension v2
>   (RASv2) for AArch64.
> 
>diff --git a/ld/emultempl/riscvelf.em b/ld/emultempl/riscvelf.em
>index bb6298d3e8d..7dbd264c0e8 100644
>--- a/ld/emultempl/riscvelf.em
>+++ b/ld/emultempl/riscvelf.em
>@@ -25,7 +25,8 @@ fragment <<EOF
> #include "elf/riscv.h"
> #include "elfxx-riscv.h"
> 
>-static struct riscv_elf_params params = { .relax_gp = 1 };
>+static struct riscv_elf_params params = { .relax_gp = 1,
>+					  .check_uleb128 = 0};
> EOF
> 
> # Define some shell vars to insert bits of code into the standard elf
>@@ -35,17 +36,23 @@ enum risccv_opt
> {
>   OPTION_RELAX_GP = 321,
>   OPTION_NO_RELAX_GP,
>+  OPTION_CHECK_ULEB128,
>+  OPTION_NO_CHECK_ULEB128,
> };
> '
> 
> PARSE_AND_LIST_LONGOPTS=${PARSE_AND_LIST_LONGOPTS}'
>     { "relax-gp", no_argument, NULL, OPTION_RELAX_GP },
>     { "no-relax-gp", no_argument, NULL, OPTION_NO_RELAX_GP },
>+    { "check-uleb128", no_argument, NULL, OPTION_CHECK_ULEB128 },
>+    { "no-check-uleb128", no_argument, NULL, OPTION_NO_CHECK_ULEB128 },
> '
> 
> PARSE_AND_LIST_OPTIONS=${PARSE_AND_LIST_OPTIONS}'
>   fprintf (file, _("  --relax-gp                  Perform GP relaxation\n"));
>   fprintf (file, _("  --no-relax-gp               Don'\''t perform GP relaxation\n"));
>+  fprintf (file, _("  --check-uleb128             Check if SUB_ULEB128 with non-zero addend\n"));
>+  fprintf (file, _("  --no-check-uleb128          Don'\''t check if SUB_ULEB128 with non-zero addend\n"));
> '
> 
> PARSE_AND_LIST_ARGS_CASES=${PARSE_AND_LIST_ARGS_CASES}'
>@@ -56,6 +63,14 @@ PARSE_AND_LIST_ARGS_CASES=${PARSE_AND_LIST_ARGS_CASES}'
>     case OPTION_NO_RELAX_GP:
>       params.relax_gp = 0;
>       break;
>+
>+    case OPTION_CHECK_ULEB128:
>+      params.check_uleb128 = 1;
>+      break;
>+
>+    case OPTION_NO_CHECK_ULEB128:
>+      params.check_uleb128 = 0;
>+      break;
> '
> 
> fragment <<EOF
>diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>index 947a266ba72..1d793da51e5 100644
>--- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>+++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>@@ -173,6 +173,8 @@ if [istarget "riscv*-*-*"] {
>     run_dump_test "attr-phdr"
>     run_dump_test "relax-max-align-gp"
>     run_dump_test "uleb128"
>+    run_dump_test "pr31179"
>+    run_dump_test "pr31179-r"
>     run_ld_link_tests [list \
> 	[list "Weak reference 32" "-T weakref.ld -m[riscv_choose_ilp32_emul]" "" \
> 	    "-march=rv32i -mabi=ilp32" {weakref32.s} \
>diff --git a/ld/testsuite/ld-riscv-elf/pr31179-r.d b/ld/testsuite/ld-riscv-elf/pr31179-r.d
>new file mode 100644
>index 00000000000..cd5c98e62f7
>--- /dev/null
>+++ b/ld/testsuite/ld-riscv-elf/pr31179-r.d
>@@ -0,0 +1,10 @@
>+#source: pr31179.s
>+#as:
>+#readelf: -Wr
>+
>+Relocation section '.rela.text' at .*
>+[ 	]+Offset[ 	]+Info[ 	]+Type[ 	]+.*
>+[0-9a-f]+[ 	]+[0-9a-f]+[ 	]+R_RISCV_SET_ULEB128[ 	]+[0-9a-f]+[ 	]+bar \+ 1
>+[0-9a-f]+[ 	]+[0-9a-f]+[ 	]+R_RISCV_SUB_ULEB128[ 	]+[0-9a-f]+[ 	]+foo \+ 0
>+[0-9a-f]+[ 	]+[0-9a-f]+[ 	]+R_RISCV_SET_ULEB128[ 	]+[0-9a-f]+[ 	]+bar \+ 1
>+[0-9a-f]+[ 	]+[0-9a-f]+[ 	]+R_RISCV_SUB_ULEB128[ 	]+[0-9a-f]+[ 	]+foo \+ 1
>diff --git a/ld/testsuite/ld-riscv-elf/pr31179.d b/ld/testsuite/ld-riscv-elf/pr31179.d
>new file mode 100644
>index 00000000000..366b32ea6ab
>--- /dev/null
>+++ b/ld/testsuite/ld-riscv-elf/pr31179.d
>@@ -0,0 +1,11 @@
>+#source: pr31179.s
>+#as:
>+#ld: --check-uleb128
>+#objdump: -sj .text
>+#warning: .*R_RISCV_SUB_ULEB128 with non-zero addend, please rebuild by new binutils
>+
>+.*:[ 	]+file format .*
>+
>+Contents of section .text:
>+
>+[ 	]+[0-9a-f]+[ 	]+00000303[ 	]+.*
>diff --git a/ld/testsuite/ld-riscv-elf/pr31179.s b/ld/testsuite/ld-riscv-elf/pr31179.s
>new file mode 100644
>index 00000000000..5c4b4b54230
>--- /dev/null
>+++ b/ld/testsuite/ld-riscv-elf/pr31179.s
>@@ -0,0 +1,13 @@
>+.globl _start
>+_start:
>+
>+foo:
>+.2byte 0
>+bar:
>+
>+.uleb128 bar - foo + 1
>+
>+reloc:
>+.reloc reloc, R_RISCV_SET_ULEB128, bar + 1
>+.reloc reloc, R_RISCV_SUB_ULEB128, foo + 1
>+.byte 0x0
>-- 
>2.39.3 (Apple Git-145)
>

Actions: View | Diff

Attachments on bug 31179: 15267 | 15270