* 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).