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

[Palmer Dabbelt <palmer@dabbelt.com>]

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