Re: [PATCH] as: fixed internal error when immediate value of relocation overflow.

[Lulu Cai <cailulu@loongson.cn>]

在 2023/10/10 上午8:25, WANG Xuerui 写道:
> Hi,
>
> On 10/9/23 10:11, Lulu Cai wrote:
>> The as reports an internal error when the relocation
>> overflow or is misaligned. Delete _bfd_error_handler
>> can avoid it and reports Reloc overflow.
>> ---
>>   bfd/elfxx-loongarch.c                      | 8 +-------
>>   gas/testsuite/gas/loongarch/imm_overflow.d | 3 +++
>>   gas/testsuite/gas/loongarch/imm_overflow.l | 2 ++
>>   gas/testsuite/gas/loongarch/imm_overflow.s | 4 ++++
>>   gas/testsuite/gas/loongarch/imm_unalign.d  | 3 +++
>>   gas/testsuite/gas/loongarch/imm_unalign.l  | 2 ++
>>   gas/testsuite/gas/loongarch/imm_unalign.s  | 6 ++++++
>>   7 files changed, 21 insertions(+), 7 deletions(-)
>>   create mode 100644 gas/testsuite/gas/loongarch/imm_overflow.d
>>   create mode 100644 gas/testsuite/gas/loongarch/imm_overflow.l
>>   create mode 100644 gas/testsuite/gas/loongarch/imm_overflow.s
>>   create mode 100644 gas/testsuite/gas/loongarch/imm_unalign.d
>>   create mode 100644 gas/testsuite/gas/loongarch/imm_unalign.l
>>   create mode 100644 gas/testsuite/gas/loongarch/imm_unalign.s
>>
>> diff --git a/bfd/elfxx-loongarch.c b/bfd/elfxx-loongarch.c
>> index 0a595eb87d5..c20efe82f31 100644
>> --- a/bfd/elfxx-loongarch.c
>> +++ b/bfd/elfxx-loongarch.c
>> @@ -1671,7 +1671,7 @@ reloc_bits (bfd *abfd ATTRIBUTE_UNUSED,
>>   }
>>     static bool
>> -reloc_sign_bits (bfd *abfd, reloc_howto_type *howto, bfd_vma *fix_val)
>> +reloc_sign_bits (bfd *abfd ATTRIBUTE_UNUSED, reloc_howto_type 
>> *howto, bfd_vma *fix_val)
>>   {
>>     if (howto->complain_on_overflow != complain_overflow_signed)
>>       return false;
>> @@ -1682,9 +1682,6 @@ reloc_sign_bits (bfd *abfd, reloc_howto_type 
>> *howto, bfd_vma *fix_val)
>>     if (howto->rightshift
>>         && (val & ((((bfd_signed_vma) 1) << howto->rightshift) - 1)))
>>       {
>> -      (*_bfd_error_handler) (_("%pB: relocation %s right shift %d 
>> error 0x%lx"),
>> -                 abfd, howto->name, howto->rightshift, (long) val);
>> -      bfd_set_error (bfd_error_bad_value);
>>         return false;
>>       }
>>   @@ -1696,9 +1693,6 @@ reloc_sign_bits (bfd *abfd, reloc_howto_type 
>> *howto, bfd_vma *fix_val)
>>        high part: from sign bit to highest bit.  */
>>     if ((val & ~mask) && ((val & ~mask) != ~mask))
>>       {
>> -      (*_bfd_error_handler) (_("%pB: relocation %s overflow 0x%lx"),
>> -                 abfd, howto->name, (long) val);
>> -      bfd_set_error (bfd_error_bad_value);
>>         return false;
>>       }
>
> Isn't this only removing the error (downgrading it to a warning it 
> seems) but not touching anything else? This will make the programmer 
> errors slip through (apparently because no one reads warnings heh, 
> it's especially worse for many devs not comfortable with English that 
> I know of), and the problematic case you had in mind is still left 
> unfixed.
>
> I think it's better to, at least, add the previously rejected code 
> you're meaning to make acceptable, so the intent is clear; then we can 
> figure out a better way forward than simply deleting checks.

Both the as and ld call the same function and use _bfd_error_handler to 
output error messages when relocating overflow. However, as passed NULL 
to the function when it was called, resulting in an internal error. ld 
passes a non-null value when called and can output error messages 
without internal errors.
So I consider not simply deleting this function. When as passes null, 
the function is not called and reports a "Reloc overflow" error instead 
of a warning. ld can call this function to output detailed information.