diff mbox series

fix Score absolute relocation on elf32 score

Message ID LV3P220MB1855643913899911AF4B2757D4582@LV3P220MB1855.NAMP220.PROD.OUTLOOK.COM
State New
Headers
Series fix Score absolute relocation on elf32 score |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 fail Patch failed to apply

Commit Message

Danish Reyjavik Nov. 11, 2024, 6:43 p.m. UTC

Comments

Jan Beulich Nov. 12, 2024, 9:18 a.m. UTC | #1 
On 11.11.2024 19:43, Danish Reyjavik wrote:
> diff --git a/bfd/elf32-score.c b/bfd/elf32-score.c
> index d1a910f279..eb93c7cfa1 100644

I'm not a maintainer of the given target (and sadly there also is no-one
named to play that role), so I can't say anything about the correctness.
In particular as long as such a change comes with no description at all.

Jan

> --- bfd/elf32-score.c
> +++ bfd/elf32-score.c
> @@ -2165,7 +2165,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>        if ((offset & 0x1000000) != 0)
>       offset |= 0xfe000000;
>        value += offset;
> -      abs_value = value - rel_addr;
> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>        if ((abs_value & 0xfe000000) != 0)
>       return bfd_reloc_overflow;
>        addend = (addend & ~howto->src_mask)
> @@ -2241,7 +2241,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>        if ((offset & 0x800) != 0)   /* Offset is negative.  */
>       offset |= 0xfffff000;
>        value += offset;
> -      abs_value = value - rel_addr;
> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>        if ((abs_value & 0xfffff000) != 0)
>       return bfd_reloc_overflow;
>        addend = (addend & ~howto->src_mask) | (value & howto->src_mask);
> diff --git a/bfd/elf32-score7.c b/bfd/elf32-score7.c
> index ab5e32a29a..3bf4c30465 100644
> --- bfd/elf32-score7.c
> +++ bfd/elf32-score7.c
> @@ -2066,7 +2066,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>        if ((offset & 0x1000000) != 0)
>       offset |= 0xfe000000;
>        value += offset;
> -      abs_value = value - rel_addr;
> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>        if ((abs_value & 0xfe000000) != 0)
>       return bfd_reloc_overflow;
>        addend = (addend & ~howto->src_mask)
> @@ -2096,7 +2096,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>        if ((offset & 0x800) != 0)   /* Offset is negative.  */
>       offset |= 0xfffff000;
>        value += offset;
> -      abs_value = value - rel_addr;
> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>        if ((abs_value & 0xfffff000) != 0)
>       return bfd_reloc_overflow;
>        addend = (addend & ~howto->src_mask) | (value & howto->src_mask);
Danish Reyjavik Nov. 13, 2024, 4:49 p.m. UTC | #2 
There's a bug with binutils creating a jump instruction with 24bit jumps: there are many j instructions, one of which is 24bit offsets, so note what the fix does:

-      abs_value = value - rel_addr;
+      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;

that instruction is bugged when jumping backwards but most short j's will be converted to j!, so it went unnoticed
________________________________
From: Jan Beulich <jbeulich@suse.com>
Sent: 12 November 2024 09:18
To: Danish Reyjavik <danishreyjavik@outlook.com>
Cc: binutils@sourceware.org <binutils@sourceware.org>
Subject: Re: [PATCH] fix Score absolute relocation on elf32 score

On 11.11.2024 19:43, Danish Reyjavik wrote:
> diff --git a/bfd/elf32-score.c b/bfd/elf32-score.c
> index d1a910f279..eb93c7cfa1 100644

I'm not a maintainer of the given target (and sadly there also is no-one
named to play that role), so I can't say anything about the correctness.
In particular as long as such a change comes with no description at all.

Jan

> --- bfd/elf32-score.c
> +++ bfd/elf32-score.c
> @@ -2165,7 +2165,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>        if ((offset & 0x1000000) != 0)
>       offset |= 0xfe000000;
>        value += offset;
> -      abs_value = value - rel_addr;
> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>        if ((abs_value & 0xfe000000) != 0)
>       return bfd_reloc_overflow;
>        addend = (addend & ~howto->src_mask)
> @@ -2241,7 +2241,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>        if ((offset & 0x800) != 0)   /* Offset is negative.  */
>       offset |= 0xfffff000;
>        value += offset;
> -      abs_value = value - rel_addr;
> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>        if ((abs_value & 0xfffff000) != 0)
>       return bfd_reloc_overflow;
>        addend = (addend & ~howto->src_mask) | (value & howto->src_mask);
> diff --git a/bfd/elf32-score7.c b/bfd/elf32-score7.c
> index ab5e32a29a..3bf4c30465 100644
> --- bfd/elf32-score7.c
> +++ bfd/elf32-score7.c
> @@ -2066,7 +2066,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>        if ((offset & 0x1000000) != 0)
>       offset |= 0xfe000000;
>        value += offset;
> -      abs_value = value - rel_addr;
> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>        if ((abs_value & 0xfe000000) != 0)
>       return bfd_reloc_overflow;
>        addend = (addend & ~howto->src_mask)
> @@ -2096,7 +2096,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>        if ((offset & 0x800) != 0)   /* Offset is negative.  */
>       offset |= 0xfffff000;
>        value += offset;
> -      abs_value = value - rel_addr;
> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>        if ((abs_value & 0xfffff000) != 0)
>       return bfd_reloc_overflow;
>        addend = (addend & ~howto->src_mask) | (value & howto->src_mask);
Jan Beulich Nov. 14, 2024, 9:28 a.m. UTC | #3 
On 13.11.2024 17:49, Danish Reyjavik wrote:
> There's a bug with binutils creating a jump instruction with 24bit jumps: there are many j instructions, one of which is 24bit offsets, so note what the fix does:
> 
> -      abs_value = value - rel_addr;
> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
> 
> that instruction is bugged when jumping backwards but most short j's will be converted to j!, so it went unnoticed

I'm afraid your response is confusing to me, as I know nothing about the
Score architecture. Looking at the opcode table I see quite a few insns
with a ! at the end. Yet there's no "j!" there.

In any event you will want to re-submit your bugfix with an explanation
added. The better the explanation, the more likely that someone like me
(not knowing the target) can approve the change.

A quick search didn't turn up any hits for an ELF psABI for the target,
so a pointer there may help, too.

Jan

> ________________________________
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 12 November 2024 09:18
> 
> On 11.11.2024 19:43, Danish Reyjavik wrote:
>> diff --git a/bfd/elf32-score.c b/bfd/elf32-score.c
>> index d1a910f279..eb93c7cfa1 100644
> 
> I'm not a maintainer of the given target (and sadly there also is no-one
> named to play that role), so I can't say anything about the correctness.
> In particular as long as such a change comes with no description at all.
> 
> Jan
> 
>> --- bfd/elf32-score.c
>> +++ bfd/elf32-score.c
>> @@ -2165,7 +2165,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>>        if ((offset & 0x1000000) != 0)
>>       offset |= 0xfe000000;
>>        value += offset;
>> -      abs_value = value - rel_addr;
>> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>>        if ((abs_value & 0xfe000000) != 0)
>>       return bfd_reloc_overflow;
>>        addend = (addend & ~howto->src_mask)
>> @@ -2241,7 +2241,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>>        if ((offset & 0x800) != 0)   /* Offset is negative.  */
>>       offset |= 0xfffff000;
>>        value += offset;
>> -      abs_value = value - rel_addr;
>> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>>        if ((abs_value & 0xfffff000) != 0)
>>       return bfd_reloc_overflow;
>>        addend = (addend & ~howto->src_mask) | (value & howto->src_mask);
>> diff --git a/bfd/elf32-score7.c b/bfd/elf32-score7.c
>> index ab5e32a29a..3bf4c30465 100644
>> --- bfd/elf32-score7.c
>> +++ bfd/elf32-score7.c
>> @@ -2066,7 +2066,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>>        if ((offset & 0x1000000) != 0)
>>       offset |= 0xfe000000;
>>        value += offset;
>> -      abs_value = value - rel_addr;
>> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>>        if ((abs_value & 0xfe000000) != 0)
>>       return bfd_reloc_overflow;
>>        addend = (addend & ~howto->src_mask)
>> @@ -2096,7 +2096,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
>>        if ((offset & 0x800) != 0)   /* Offset is negative.  */
>>       offset |= 0xfffff000;
>>        value += offset;
>> -      abs_value = value - rel_addr;
>> +      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
>>        if ((abs_value & 0xfffff000) != 0)
>>       return bfd_reloc_overflow;
>>        addend = (addend & ~howto->src_mask) | (value & howto->src_mask);
>
Hans-Peter Nilsson Nov. 14, 2024, 4:38 p.m. UTC | #4 
On Wed, 13 Nov 2024, Danish Reyjavik wrote:
> There's a bug with binutils creating a jump instruction with 24bit jumps: there are many j
> instructions, one of which is 24bit offsets, so note what the fix does:
> - abs_value = value - rel_addr;
> + abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
> 
> that instruction is bugged when jumping backwards but most short j's will be converted to j!, so
> it went unnoticed

Just some random advice from the sideline: a testcase, even 
better, one in the form of an added regression test, to the 
score part of the (gas or ld as appropriate) testsuite, says 
more than any number of words.  It also helps agains accidental 
misedits of the code.

brgds, H-P

> 
> ________________________________________________________________________________________________
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 12 November 2024 09:18
> To: Danish Reyjavik <danishreyjavik@outlook.com>
> Cc: binutils@sourceware.org <binutils@sourceware.org>
> Subject: Re: [PATCH] fix Score absolute relocation on elf32 score
> On 11.11.2024 19:43, Danish Reyjavik wrote:
> > diff --git a/bfd/elf32-score.c b/bfd/elf32-score.c
> > index d1a910f279..eb93c7cfa1 100644
> 
> I'm not a maintainer of the given target (and sadly there also is no-one
> named to play that role), so I can't say anything about the correctness.
> In particular as long as such a change comes with no description at all.
> 
> Jan
> 
> > --- bfd/elf32-score.c
> > +++ bfd/elf32-score.c
> > @@ -2165,7 +2165,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
> > if ((offset & 0x1000000) != 0)
> > ?????offset |= 0xfe000000;
> > value += offset;
> > - abs_value = value - rel_addr;
> > + abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
> > if ((abs_value & 0xfe000000) != 0)
> > ?????return bfd_reloc_overflow;
> > addend = (addend & ~howto->src_mask)
> > @@ -2241,7 +2241,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
> > if ((offset & 0x800) != 0)???/* Offset is negative. */
> > ?????offset |= 0xfffff000;
> > value += offset;
> > - abs_value = value - rel_addr;
> > + abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
> > if ((abs_value & 0xfffff000) != 0)
> > ?????return bfd_reloc_overflow;
> > addend = (addend & ~howto->src_mask) | (value & howto->src_mask);
> > diff --git a/bfd/elf32-score7.c b/bfd/elf32-score7.c
> > index ab5e32a29a..3bf4c30465 100644
> > --- bfd/elf32-score7.c
> > +++ bfd/elf32-score7.c
> > @@ -2066,7 +2066,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
> > if ((offset & 0x1000000) != 0)
> > ?????offset |= 0xfe000000;
> > value += offset;
> > - abs_value = value - rel_addr;
> > + abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
> > if ((abs_value & 0xfe000000) != 0)
> > ?????return bfd_reloc_overflow;
> > addend = (addend & ~howto->src_mask)
> > @@ -2096,7 +2096,7 @@ score_elf_final_link_relocate (reloc_howto_type *howto,
> > if ((offset & 0x800) != 0)???/* Offset is negative. */
> > ?????offset |= 0xfffff000;
> > value += offset;
> > - abs_value = value - rel_addr;
> > + abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
> > if ((abs_value & 0xfffff000) != 0)
> > ?????return bfd_reloc_overflow;
> > addend = (addend & ~howto->src_mask) | (value & howto->src_mask);
> 
> 
>
diff mbox series

Patch

diff --git a/bfd/elf32-score.c b/bfd/elf32-score.c
index d1a910f279..eb93c7cfa1 100644
--- bfd/elf32-score.c
+++ bfd/elf32-score.c
@@ -2165,7 +2165,7 @@  score_elf_final_link_relocate (reloc_howto_type *howto,
       if ((offset & 0x1000000) != 0)
      offset |= 0xfe000000;
       value += offset;
-      abs_value = value - rel_addr;
+      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
       if ((abs_value & 0xfe000000) != 0)
      return bfd_reloc_overflow;
       addend = (addend & ~howto->src_mask)
@@ -2241,7 +2241,7 @@  score_elf_final_link_relocate (reloc_howto_type *howto,
       if ((offset & 0x800) != 0)   /* Offset is negative.  */
      offset |= 0xfffff000;
       value += offset;
-      abs_value = value - rel_addr;
+      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
       if ((abs_value & 0xfffff000) != 0)
      return bfd_reloc_overflow;
       addend = (addend & ~howto->src_mask) | (value & howto->src_mask);
diff --git a/bfd/elf32-score7.c b/bfd/elf32-score7.c
index ab5e32a29a..3bf4c30465 100644
--- bfd/elf32-score7.c
+++ bfd/elf32-score7.c
@@ -2066,7 +2066,7 @@  score_elf_final_link_relocate (reloc_howto_type *howto,
       if ((offset & 0x1000000) != 0)
      offset |= 0xfe000000;
       value += offset;
-      abs_value = value - rel_addr;
+      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
       if ((abs_value & 0xfe000000) != 0)
      return bfd_reloc_overflow;
       addend = (addend & ~howto->src_mask)
@@ -2096,7 +2096,7 @@  score_elf_final_link_relocate (reloc_howto_type *howto,
       if ((offset & 0x800) != 0)   /* Offset is negative.  */
      offset |= 0xfffff000;
       value += offset;
-      abs_value = value - rel_addr;
+      abs_value = (value < rel_addr) ? rel_addr - value : value - rel_addr;
       if ((abs_value & 0xfffff000) != 0)
      return bfd_reloc_overflow;
       addend = (addend & ~howto->src_mask) | (value & howto->src_mask);