[PATCH v1 0/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard

[PATCH v1 0/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard

[zengxiao]

From: zengxiao <zengxiao@eswincomputing.com>

Hi all RISC-V folks:

When riscv-objdump is used to generate dwarf information, problems are found, like:
DW_CFA_??? (User defined call frame op: 0x3c)

This error is related to that riscv-objdump does not follow the riscv R_RISCV_SUB6 standard. 
Riscv-readelf is correct because it follows the R_RISCV_SUB6 standard.

There are test cases in https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769
that describe the error in detail. 

zengxiao (1):
  RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard

 bfd/elfxx-riscv.c                             |  7 +++++
 .../testsuite/binutils-all/riscv/dwarf-SUB6.d | 31 +++++++++++++++++++
 .../testsuite/binutils-all/riscv/dwarf-SUB6.s | 12 +++++++
 3 files changed, 50 insertions(+)
 create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
 create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s

-- 
2.34.1

[PATCH v1 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard

[zengxiao]

From: zengxiao <zengxiao@eswincomputing.com>

This patch makes R_RISCV_SUB6 conforms to riscv abi standard.
R_RISCV_SUB6 only the lower 6 bits of the code are valid.
The proposed specification which can be found in 8.5. Relocations of,
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/download/v1.0-rc4/riscv-abi.pdf

bfd/ChangeLog:

        * elfxx-riscv.c (riscv_elf_add_sub_reloc):

binutils/ChangeLog:

        * testsuite/binutils-all/riscv/dwarf-SUB6.d: New test.
        * testsuite/binutils-all/riscv/dwarf-SUB6.s: New test.

reviewed-by: gaofei@eswincomputing.com
             jinyanjiang@eswincomputing.com

Signed-off-by: zengxiao <zengxiao@eswincomputing.com>
---
 bfd/elfxx-riscv.c                             |  7 +++++
 .../testsuite/binutils-all/riscv/dwarf-SUB6.d | 31 +++++++++++++++++++
 .../testsuite/binutils-all/riscv/dwarf-SUB6.s | 12 +++++++
 3 files changed, 50 insertions(+)
 create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
 create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s

diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
index 300ccf49534..e71d4a456f2 100644
--- a/bfd/elfxx-riscv.c
+++ b/bfd/elfxx-riscv.c
@@ -994,6 +994,13 @@ riscv_elf_add_sub_reloc (bfd *abfd,
       relocation = old_value + relocation;
       break;
     case R_RISCV_SUB6:
+      {
+        bfd_vma six_bit_valid_value = old_value & howto->dst_mask;
+        six_bit_valid_value -= relocation;
+        relocation = (six_bit_valid_value & howto->dst_mask) |
+	              (old_value & ~howto->dst_mask);
+      }
+      break;
     case R_RISCV_SUB8:
     case R_RISCV_SUB16:
     case R_RISCV_SUB32:
diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
new file mode 100644
index 00000000000..47d5ae570d7
--- /dev/null
+++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
@@ -0,0 +1,31 @@
+#PROG: objcopy
+#objdump: --dwarf=frames
+
+tmpdir/riscvcopy.o:     file format elf32-littleriscv
+
+Contents of the .eh_frame section:
+
+
+00000000 00000020 00000000 CIE
+  Version:               3
+  Augmentation:          "zR"
+  Code alignment factor: 1
+  Data alignment factor: -4
+  Return address column: 1
+  Augmentation data:     1b
+  DW_CFA_def_cfa_register: r2 \(sp\)
+  DW_CFA_def_cfa_offset: 48
+  DW_CFA_offset: r1 \(ra\) at cfa-4
+  DW_CFA_offset: r8 \(s0\) at cfa-8
+  DW_CFA_def_cfa: r8 \(s0\) ofs 0
+  DW_CFA_restore: r1 \(ra\)
+  DW_CFA_restore: r8 \(s0\)
+  DW_CFA_def_cfa: r2 \(sp\) ofs 48
+  DW_CFA_def_cfa_offset: 0
+  DW_CFA_nop
+
+00000024 00000010 00000028 FDE cie=00000000 pc=0000002c..0000002c
+  DW_CFA_nop
+  DW_CFA_nop
+  DW_CFA_nop
+
diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
new file mode 100644
index 00000000000..fe959f59d9b
--- /dev/null
+++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
@@ -0,0 +1,12 @@
+        .attribute arch, "rv32i2p0_m2p0_a2p0_f2p0_c2p0"
+        .cfi_startproc
+        .cfi_def_cfa_offset 48
+        .cfi_offset 1, -4
+        .cfi_offset 8, -8
+        .cfi_def_cfa 8, 0
+        .cfi_restore 1
+        .cfi_restore 8
+        .cfi_def_cfa 2, 48
+        .cfi_def_cfa_offset 0
+        .cfi_endproc
+        
\ No newline at end of file
-- 
2.34.1

Re: [PATCH v1 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard

[Nelson Chu]

On Tue, Nov 15, 2022 at 4:15 PM <zengxiao@eswincomputing.com> wrote:
>
> From: zengxiao <zengxiao@eswincomputing.com>
>
> This patch makes R_RISCV_SUB6 conforms to riscv abi standard.
> R_RISCV_SUB6 only the lower 6 bits of the code are valid.
> The proposed specification which can be found in 8.5. Relocations of,
> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/download/v1.0-rc4/riscv-abi.pdf
>
> bfd/ChangeLog:
>
>         * elfxx-riscv.c (riscv_elf_add_sub_reloc):
>
> binutils/ChangeLog:
>
>         * testsuite/binutils-all/riscv/dwarf-SUB6.d: New test.
>         * testsuite/binutils-all/riscv/dwarf-SUB6.s: New test.

I tried but it seems like the testcases won't generate any
R_RISCV_SET6/SUB6, so it is useless in fact.  Maybe reducing your test
case here is the right thing to do,
https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769, I can
reproduce the problem when testing the link.

> reviewed-by: gaofei@eswincomputing.com
>              jinyanjiang@eswincomputing.com
>
> Signed-off-by: zengxiao <zengxiao@eswincomputing.com>
> ---
>  bfd/elfxx-riscv.c                             |  7 +++++
>  .../testsuite/binutils-all/riscv/dwarf-SUB6.d | 31 +++++++++++++++++++
>  .../testsuite/binutils-all/riscv/dwarf-SUB6.s | 12 +++++++
>  3 files changed, 50 insertions(+)
>  create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
>  create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index 300ccf49534..e71d4a456f2 100644
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -994,6 +994,13 @@ riscv_elf_add_sub_reloc (bfd *abfd,
>        relocation = old_value + relocation;
>        break;
>      case R_RISCV_SUB6:
> +      {
> +        bfd_vma six_bit_valid_value = old_value & howto->dst_mask;
> +        six_bit_valid_value -= relocation;
> +        relocation = (six_bit_valid_value & howto->dst_mask) |
> +                     (old_value & ~howto->dst_mask);
> +      }
> +      break;

Seems like it will cause problems if we are dumping the debug
information of the object by the objdump, before the final link.
There are two issues here, one is the overflow checking, and another
is to have the same calculations for both riscv_elf_add_sub_reloc and
riscv_elf_relocate_section.

For the former, I just talked to Palmer today, and talked to Kito a
few months ago, we all agree that we should have overflow checks for
these kinds of relocations, including ADD/SUB/SET, even if we don't
have any of them for now.  I don't remember the details when I tried
to add the overflow checks before, but that caused the debug
information broken for the rv64 toolchains when running the gcc
regressions.  Since sometimes we still generate the ADD32/SUB32
relocations in rv64, but that seems dangerous though.  Anyway, I'm not
the expert for that, so I don't have any useful thoughts for now.

For the latter, I think llvm did the things right,
https://llvm.org/doxygen/RelocationResolver_8cpp_source.html#l00469.
So,

1. relocation = (old_value & ~howto->dst_mask)
                         | (((old_value & howto->dst_mask) - relocation)
                            & howto->dst_mask);

2. We should have the same behavior in riscv_elf_relocate_section -
filter to get the valid least 6-bit address for the old value,
https://github.com/bminor/binutils-gdb/blob/master/bfd/elfnn-riscv.c#L2429.
Btw, we don't need to do anything else when encoding, since
perform_relocation already did the right thing.

It would be great if we have the testcase, but the testcase in this
patch doesn't seems useful, so we need a reduced case from here,
https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769, to show
something like "DW_ CFA_??? (User defined call frame op: 0x3c)" before
the fix.  Or if it is too difficult to reduce, then it's okay for me
to ignore the testcase.  Just remember to pass the riscv-gnu-toolchain
regressions, that should be enough to prove we won't break something
basically.

Please see the regressions here,
https://github.com/riscv-collab/riscv-gnu-toolchain/tree/master/regression

Thanks
Nelson

>      case R_RISCV_SUB8:
>      case R_RISCV_SUB16:
>      case R_RISCV_SUB32:
> diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
> new file mode 100644
> index 00000000000..47d5ae570d7
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
> @@ -0,0 +1,31 @@
> +#PROG: objcopy
> +#objdump: --dwarf=frames
> +
> +tmpdir/riscvcopy.o:     file format elf32-littleriscv
> +
> +Contents of the .eh_frame section:
> +
> +
> +00000000 00000020 00000000 CIE
> +  Version:               3
> +  Augmentation:          "zR"
> +  Code alignment factor: 1
> +  Data alignment factor: -4
> +  Return address column: 1
> +  Augmentation data:     1b
> +  DW_CFA_def_cfa_register: r2 \(sp\)
> +  DW_CFA_def_cfa_offset: 48
> +  DW_CFA_offset: r1 \(ra\) at cfa-4
> +  DW_CFA_offset: r8 \(s0\) at cfa-8
> +  DW_CFA_def_cfa: r8 \(s0\) ofs 0
> +  DW_CFA_restore: r1 \(ra\)
> +  DW_CFA_restore: r8 \(s0\)
> +  DW_CFA_def_cfa: r2 \(sp\) ofs 48
> +  DW_CFA_def_cfa_offset: 0
> +  DW_CFA_nop
> +
> +00000024 00000010 00000028 FDE cie=00000000 pc=0000002c..0000002c
> +  DW_CFA_nop
> +  DW_CFA_nop
> +  DW_CFA_nop
> +
> diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
> new file mode 100644
> index 00000000000..fe959f59d9b
> --- /dev/null
> +++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
> @@ -0,0 +1,12 @@
> +        .attribute arch, "rv32i2p0_m2p0_a2p0_f2p0_c2p0"
> +        .cfi_startproc
> +        .cfi_def_cfa_offset 48
> +        .cfi_offset 1, -4
> +        .cfi_offset 8, -8
> +        .cfi_def_cfa 8, 0
> +        .cfi_restore 1
> +        .cfi_restore 8
> +        .cfi_def_cfa 2, 48
> +        .cfi_def_cfa_offset 0
> +        .cfi_endproc
> +
> \ No newline at end of file
> --
> 2.34.1
>

Re: [PATCH v1 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard

[Xiao Zeng]

On Wed, Nov 16, 2022 at 12:00:00 AM  Nelson Chu <nelson@rivosinc.com> wrote:
>
>On Tue, Nov 15, 2022 at 4:15 PM <zengxiao@eswincomputing.com> wrote:
>>
>> From: zengxiao <zengxiao@eswincomputing.com>
>>
>> This patch makes R_RISCV_SUB6 conforms to riscv abi standard.
>> R_RISCV_SUB6 only the lower 6 bits of the code are valid.
>> The proposed specification which can be found in 8.5. Relocations of,
>> https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/download/v1.0-rc4/riscv-abi.pdf
>>
>> bfd/ChangeLog:
>>
>>         * elfxx-riscv.c (riscv_elf_add_sub_reloc):
>>
>> binutils/ChangeLog:
>>
>>         * testsuite/binutils-all/riscv/dwarf-SUB6.d: New test.
>>         * testsuite/binutils-all/riscv/dwarf-SUB6.s: New test.
>
>I tried but it seems like the testcases won't generate any
>R_RISCV_SET6/SUB6, so it is useless in fact.  Maybe reducing your test
>case here is the right thing to do,
>https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769, I can
>reproduce the problem when testing the link. 

Yes, I am not familiar with how to construct the dwarf of R_RISCV_SET6/SUB6.
The dwarf-SUB6.s only constructs a failed test case, which does not reflect 
the R_RISCV_SET6/SUB6.
I would be grateful if anyone could provide such a construction use case. 
Or can there be no test case in this place temporarily?
Because the test case is invalid, I will delete it in v2 patch.

>
>> reviewed-by: gaofei@eswincomputing.com
>>              jinyanjiang@eswincomputing.com
>>
>> Signed-off-by: zengxiao <zengxiao@eswincomputing.com>
>> ---
>>  bfd/elfxx-riscv.c                             |  7 +++++
>>  .../testsuite/binutils-all/riscv/dwarf-SUB6.d | 31 +++++++++++++++++++
>>  .../testsuite/binutils-all/riscv/dwarf-SUB6.s | 12 +++++++
>>  3 files changed, 50 insertions(+)
>>  create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
>>  create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
>>
>> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
>> index 300ccf49534..e71d4a456f2 100644
>> --- a/bfd/elfxx-riscv.c
>> +++ b/bfd/elfxx-riscv.c
>> @@ -994,6 +994,13 @@ riscv_elf_add_sub_reloc (bfd *abfd,
>>        relocation = old_value + relocation;
>>        break;
>>      case R_RISCV_SUB6:
>> +      {
>> +        bfd_vma six_bit_valid_value = old_value & howto->dst_mask;
>> +        six_bit_valid_value -= relocation;
>> +        relocation = (six_bit_valid_value & howto->dst_mask) |
>> +                     (old_value & ~howto->dst_mask);
>> +      }
>> +      break;
>
>Seems like it will cause problems if we are dumping the debug
>information of the object by the objdump, before the final link.
>There are two issues here, one is the overflow checking, and another
>is to have the same calculations for both riscv_elf_add_sub_reloc and
>riscv_elf_relocate_section.
>
>For the former, I just talked to Palmer today, and talked to Kito a
>few months ago, we all agree that we should have overflow checks for
>these kinds of relocations, including ADD/SUB/SET, even if we don't
>have any of them for now.  I don't remember the details when I tried
>to add the overflow checks before, but that caused the debug
>information broken for the rv64 toolchains when running the gcc
>regressions.  Since sometimes we still generate the ADD32/SUB32
>relocations in rv64, but that seems dangerous though.  Anyway, I'm not
>the expert for that, so I don't have any useful thoughts for now.
>
>For the latter, I think llvm did the things right,
>https://llvm.org/doxygen/RelocationResolver_8cpp_source.html#l00469.
>So,
>
>1. relocation = (old_value & ~howto->dst_mask)
>                         | (((old_value & howto->dst_mask) - relocation)
>                            & howto->dst_mask);
> 

fix

>2. We should have the same behavior in riscv_elf_relocate_section -
>filter to get the valid least 6-bit address for the old value,
>https://github.com/bminor/binutils-gdb/blob/master/bfd/elfnn-riscv.c#L2429. 

fix

>Btw, we don't need to do anything else when encoding, since
>perform_relocation already did the right thing.
>
>It would be great if we have the testcase, but the testcase in this
>patch doesn't seems useful, so we need a reduced case from here,
>https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769, to show
>something like "DW_ CFA_??? (User defined call frame op: 0x3c)" before
>the fix.  Or if it is too difficult to reduce, then it's okay for me
>to ignore the testcase.  Just remember to pass the riscv-gnu-toolchain
>regressions, that should be enough to prove we won't break something
>basically.
>
>Please see the regressions here,
>https://github.com/riscv-collab/riscv-gnu-toolchain/tree/master/regression

The link given seems to be a Makefile for testing. I don't understand what you mean.
Can you give more detailed steps for testing?
How can I merge my patch into the code base and report the test results of the 
regression to the community?

>
>Thanks
>Nelson 

I will put my v2 patch in https://sourceware.org/pipermail/binutils/2022-November/124547.html. 
Please review the code on it later.

Thanks
Xiao
>
>>      case R_RISCV_SUB8:
>>      case R_RISCV_SUB16:
>>      case R_RISCV_SUB32:
>> diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
>> new file mode 100644
>> index 00000000000..47d5ae570d7
>> --- /dev/null
>> +++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d
>> @@ -0,0 +1,31 @@
>> +#PROG: objcopy
>> +#objdump: --dwarf=frames
>> +
>> +tmpdir/riscvcopy.o:     file format elf32-littleriscv
>> +
>> +Contents of the .eh_frame section:
>> +
>> +
>> +00000000 00000020 00000000 CIE
>> +  Version:               3
>> +  Augmentation:          "zR"
>> +  Code alignment factor: 1
>> +  Data alignment factor: -4
>> +  Return address column: 1
>> +  Augmentation data:     1b
>> +  DW_CFA_def_cfa_register: r2 \(sp\)
>> +  DW_CFA_def_cfa_offset: 48
>> +  DW_CFA_offset: r1 \(ra\) at cfa-4
>> +  DW_CFA_offset: r8 \(s0\) at cfa-8
>> +  DW_CFA_def_cfa: r8 \(s0\) ofs 0
>> +  DW_CFA_restore: r1 \(ra\)
>> +  DW_CFA_restore: r8 \(s0\)
>> +  DW_CFA_def_cfa: r2 \(sp\) ofs 48
>> +  DW_CFA_def_cfa_offset: 0
>> +  DW_CFA_nop
>> +
>> +00000024 00000010 00000028 FDE cie=00000000 pc=0000002c..0000002c
>> +  DW_CFA_nop
>> +  DW_CFA_nop
>> +  DW_CFA_nop
>> +
>> diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
>> new file mode 100644
>> index 00000000000..fe959f59d9b
>> --- /dev/null
>> +++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s
>> @@ -0,0 +1,12 @@
>> +        .attribute arch, "rv32i2p0_m2p0_a2p0_f2p0_c2p0"
>> +        .cfi_startproc
>> +        .cfi_def_cfa_offset 48
>> +        .cfi_offset 1, -4
>> +        .cfi_offset 8, -8
>> +        .cfi_def_cfa 8, 0
>> +        .cfi_restore 1
>> +        .cfi_restore 8
>> +        .cfi_def_cfa 2, 48
>> +        .cfi_def_cfa_offset 0
>> +        .cfi_endproc
>> +
>> \ No newline at end of file
>> --
>> 2.34.1
>>