diff mbox series

as: fixed internal error when immediate value of relocation overflow.

Message ID 20231009021109.2980562-1-cailulu@loongson.cn
State New
Headers
Series as: fixed internal error when immediate value of relocation overflow. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed

Commit Message

Lulu Cai Oct. 9, 2023, 2:11 a.m. UTC 
  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

Comments

WANG Xuerui Oct. 10, 2023, 12:25 a.m. UTC | #1 
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.
Lulu Cai Oct. 10, 2023, 9:55 a.m. UTC | #2 
在 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.
diff mbox series

Patch

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;
     }
 
diff --git a/gas/testsuite/gas/loongarch/imm_overflow.d b/gas/testsuite/gas/loongarch/imm_overflow.d
new file mode 100644
index 00000000000..1ead2c9a74f
--- /dev/null
+++ b/gas/testsuite/gas/loongarch/imm_overflow.d
@@ -0,0 +1,3 @@ 
+#as:
+#source: imm_overflow.s
+#warning_output: imm_overflow.l
diff --git a/gas/testsuite/gas/loongarch/imm_overflow.l b/gas/testsuite/gas/loongarch/imm_overflow.l
new file mode 100644
index 00000000000..3dd4d90ffa1
--- /dev/null
+++ b/gas/testsuite/gas/loongarch/imm_overflow.l
@@ -0,0 +1,2 @@ 
+.*Assembler messages:
+.*Warning: Reloc overflow
diff --git a/gas/testsuite/gas/loongarch/imm_overflow.s b/gas/testsuite/gas/loongarch/imm_overflow.s
new file mode 100644
index 00000000000..2be688bd25a
--- /dev/null
+++ b/gas/testsuite/gas/loongarch/imm_overflow.s
@@ -0,0 +1,4 @@ 
+.L1:
+  nop
+  .fill 0x12345, 4, 0
+  blt $r3, $r4, .L1
diff --git a/gas/testsuite/gas/loongarch/imm_unalign.d b/gas/testsuite/gas/loongarch/imm_unalign.d
new file mode 100644
index 00000000000..df1a8cf6041
--- /dev/null
+++ b/gas/testsuite/gas/loongarch/imm_unalign.d
@@ -0,0 +1,3 @@ 
+#as:
+#source: imm_unalign.s
+#warning_output: imm_unalign.l
diff --git a/gas/testsuite/gas/loongarch/imm_unalign.l b/gas/testsuite/gas/loongarch/imm_unalign.l
new file mode 100644
index 00000000000..3dd4d90ffa1
--- /dev/null
+++ b/gas/testsuite/gas/loongarch/imm_unalign.l
@@ -0,0 +1,2 @@ 
+.*Assembler messages:
+.*Warning: Reloc overflow
diff --git a/gas/testsuite/gas/loongarch/imm_unalign.s b/gas/testsuite/gas/loongarch/imm_unalign.s
new file mode 100644
index 00000000000..d97ada04d9f
--- /dev/null
+++ b/gas/testsuite/gas/loongarch/imm_unalign.s
@@ -0,0 +1,6 @@ 
+.L1:
+  .2byte 0x12
+
+.L2:
+  .fill 1, 4, 0
+  blt $r3, $r4, .L1