* Re:[PATCH 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard
@ 2022-11-11 15:32 shihua
2022-11-18 4:59 ` Palmer Dabbelt
0 siblings, 1 reply; 4+ messages in thread
From: shihua @ 2022-11-11 15:32 UTC (permalink / raw)
To: zengxiao; +Cc: binutils
[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]
LGTM,and I think it would be better to have a test example.
> 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): Take the lower
> 6 bits as the significant bit
>
> reviewed-by: gaofei@eswincomputing.com
> jinyanjiang@eswincomputing.com
>
> Signed-off-by: zengxiao <zengxiao@eswincomputing.com>
> ---
> bfd/elfxx-riscv.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c
> index f0c91cc97f7..0fbfedd17fe 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:
> --
> 2.34.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re:[PATCH 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard 2022-11-11 15:32 Re:[PATCH 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard shihua @ 2022-11-18 4:59 ` Palmer Dabbelt 2022-11-18 8:33 ` [PATCH " Nelson Chu 0 siblings, 1 reply; 4+ messages in thread From: Palmer Dabbelt @ 2022-11-18 4:59 UTC (permalink / raw) To: shihua; +Cc: zengxiao, binutils On Fri, 11 Nov 2022 07:32:49 PST (-0800), shihua@iscas.ac.cn wrote: > LGTM,and I think it would be better to have a test example. > > > > > >> 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): Take the lower >> 6 bits as the significant bit >> >> reviewed-by: gaofei@eswincomputing.com >> jinyanjiang@eswincomputing.com Is this trying to say that both of you reviewed it? >> Signed-off-by: zengxiao <zengxiao@eswincomputing.com> >> --- >> bfd/elfxx-riscv.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c >> index f0c91cc97f7..0fbfedd17fe 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; Unless I'm missing something here, this just just silently truncates the relocation to 6 bits because the range check still assumes an 8-bit relocation range. I'm not sure if there's a way to massage the howto entry to make bfd_reloc_offset_in_range() understand this is a 6-bit relocation, if that's not viable then we should just check for the overflow here and return bfd_reloc_outofrange. That also means there should be at least two test cases, on within range and one outside of it. >> case R_RISCV_SUB8: >> case R_RISCV_SUB16: >> case R_RISCV_SUB32: >> -- >> 2.34.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard 2022-11-18 4:59 ` Palmer Dabbelt @ 2022-11-18 8:33 ` Nelson Chu 2022-11-21 10:51 ` Xiao Zeng 0 siblings, 1 reply; 4+ messages in thread From: Nelson Chu @ 2022-11-18 8:33 UTC (permalink / raw) To: Palmer Dabbelt; +Cc: shihua, zengxiao, binutils On Fri, Nov 18, 2022 at 1:00 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Fri, 11 Nov 2022 07:32:49 PST (-0800), shihua@iscas.ac.cn wrote: > > LGTM,and I think it would be better to have a test example. > > > > > > > > > > > >> 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): Take the lower > >> 6 bits as the significant bit > >> > >> reviewed-by: gaofei@eswincomputing.com > >> jinyanjiang@eswincomputing.com > > Is this trying to say that both of you reviewed it? > > >> Signed-off-by: zengxiao <zengxiao@eswincomputing.com> > > > >> --- > >> bfd/elfxx-riscv.c | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c > >> index f0c91cc97f7..0fbfedd17fe 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; > > Unless I'm missing something here, this just just silently truncates the > relocation to 6 bits because the range check still assumes an 8-bit > relocation range. I'm not sure if there's a way to massage the howto > entry to make bfd_reloc_offset_in_range() understand this is a 6-bit > relocation, if that's not viable then we should just check for the > overflow here and return bfd_reloc_outofrange. Yeah agreed, we shouldn't call bfd_reloc_offset_in_range for R_RISCV_SUB6 since we are assuming it is an 8-bit relocation. That means if the R_RISCV_SUB6 is used to relocate the last 6-bit of the section, then ld will always report bfd_reloc_outofrange since it assumes at least 8-bit is needed, although the case seems minor. As for the overflow of relocation values, we don't have any checks of them, so I think we can just ignore them in the short-term. Thanks Nelson > That also means there should be at least two test cases, on within range > and one outside of it. > > >> case R_RISCV_SUB8: > >> case R_RISCV_SUB16: > >> case R_RISCV_SUB32: > >> -- > >> 2.34.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard 2022-11-18 8:33 ` [PATCH " Nelson Chu @ 2022-11-21 10:51 ` Xiao Zeng 0 siblings, 0 replies; 4+ messages in thread From: Xiao Zeng @ 2022-11-21 10:51 UTC (permalink / raw) To: Nelson Chu, Palmer Dabbelt; +Cc: shihua, binutils On Fri, Nov 18, 2022 at 12:00:00 AM Nelson Chu <nelson@rivosinc.com> wrote: > >On Fri, Nov 18, 2022 at 1:00 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: >> >> On Fri, 11 Nov 2022 07:32:49 PST (-0800), shihua@iscas.ac.cn wrote: >> > LGTM,and I think it would be better to have a test example. >> > >> > >> > >> > >> > >> >> 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): Take the lower >> >> 6 bits as the significant bit >> >> >> >> reviewed-by: gaofei@eswincomputing.com >> >> jinyanjiang@eswincomputing.com >> >> Is this trying to say that both of you reviewed it? >> >> >> Signed-off-by: zengxiao <zengxiao@eswincomputing.com> >> >> >> >> --- >> >> bfd/elfxx-riscv.c | 7 +++++++ >> >> 1 file changed, 7 insertions(+) >> >> >> >> diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c >> >> index f0c91cc97f7..0fbfedd17fe 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; >> >> Unless I'm missing something here, this just just silently truncates the >> relocation to 6 bits because the range check still assumes an 8-bit >> relocation range. I'm not sure if there's a way to massage the howto >> entry to make bfd_reloc_offset_in_range() understand this is a 6-bit >> relocation, if that's not viable then we should just check for the >> overflow here and return bfd_reloc_outofrange. > >Yeah agreed, we shouldn't call bfd_reloc_offset_in_range for >R_RISCV_SUB6 since we are assuming it is an 8-bit relocation. That >means if the R_RISCV_SUB6 is used to relocate the last 6-bit of the >section, then ld will always report bfd_reloc_outofrange since it >assumes at least 8-bit is needed, although the case seems minor. > Thanks Nelson for catching the point of my patch: the R_RISCV_SUB6 is incorrectly assumed an 8-bit relocation. Therefore, the overflow check of bfd_reloc_offset_in_range() may not help objdump correctly resolve the R_ RISCV_ SUB6. >As for the overflow of relocation values, we don't have any checks of >them, so I think we can just ignore them in the short-term. > >Thanks >Nelson > >> That also means there should be at least two test cases, on within range >> and one outside of it. >> Thanks Xiao >> >> case R_RISCV_SUB8: >> >> case R_RISCV_SUB16: >> >> case R_RISCV_SUB32: >> >> -- >> >> 2.34.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-21 10:51 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-11 15:32 Re:[PATCH 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard shihua 2022-11-18 4:59 ` Palmer Dabbelt 2022-11-18 8:33 ` [PATCH " Nelson Chu 2022-11-21 10:51 ` Xiao Zeng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).