[v2] RISC-V: hash with segment id and pcrel_hi address while recording pcrel_hi
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Test passed
|
Commit Message
From: Lifang Xia <lifang_xia@linux.alibaba.com>
When the same address across different segments needs to be recorded, it will
overwrite the slot, leading to a memory leak. To ensure uniqueness, the
segment ID needs to be included in the hash key calculation.
gas/
* config/tc-riscv.c (riscv_pcrel_hi_fixup): New member "seg".
(riscv_pcrel_fixup_hash): make seg->id and e->adrsess as the
hash key.
(riscv_pcrel_fixup_eq): Check seg->id at first.
(riscv_record_pcrel_fixup): New member "seg".
(md_apply_fix) <case BFD_RELOC_RISCV_PCREL_HI20>: Likewise.
(md_apply_fix) <case BFD_RELOC_RISCV_PCREL_LO12_I>: Likewise.
---
gas/config/tc-riscv.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
Comments
On 04.07.2024 03:56, lifang_xia@linux.alibaba.com wrote:
> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -1784,6 +1784,7 @@ init_opcode_hash (const struct riscv_opcode *opcodes,
> help us resolve the corresponding low-part relocation later. */
> typedef struct
> {
> + segT seg;
Just one minor comment here: segT really being asection *, I think it would
be desirable to use const asection * here instead, to clarify that there's
no intent of changing the referenced section. Not using the pre-cooked segT
may not be very nice, but (a) there's precedent (obj-elf.c:match_section())
and (b) I think const-correctness is more important overall.
In the end it'll be Nelson anyway to approve the change.
Jan
The const asection * sounds reasonable and good to me, thanks for the
suggestion :-)
Nelson
On Thu, Jul 4, 2024 at 2:46 PM Jan Beulich <jbeulich@suse.com> wrote:
> On 04.07.2024 03:56, lifang_xia@linux.alibaba.com wrote:
> > --- a/gas/config/tc-riscv.c
> > +++ b/gas/config/tc-riscv.c
> > @@ -1784,6 +1784,7 @@ init_opcode_hash (const struct riscv_opcode
> *opcodes,
> > help us resolve the corresponding low-part relocation later. */
> > typedef struct
> > {
> > + segT seg;
>
> Just one minor comment here: segT really being asection *, I think it would
> be desirable to use const asection * here instead, to clarify that there's
> no intent of changing the referenced section. Not using the pre-cooked segT
> may not be very nice, but (a) there's precedent (obj-elf.c:match_section())
> and (b) I think const-correctness is more important overall.
>
> In the end it'll be Nelson anyway to approve the change.
>
> Jan
>
Committed after passing the regression of riscv-gnu-toolchain, with the
const asection change.
Thanks for all
Nelson
On Thu, Jul 4, 2024 at 5:18 PM Nelson Chu <nelson@rivosinc.com> wrote:
> The const asection * sounds reasonable and good to me, thanks for the
> suggestion :-)
>
> Nelson
>
> On Thu, Jul 4, 2024 at 2:46 PM Jan Beulich <jbeulich@suse.com> wrote:
>
>> On 04.07.2024 03:56, lifang_xia@linux.alibaba.com wrote:
>> > --- a/gas/config/tc-riscv.c
>> > +++ b/gas/config/tc-riscv.c
>> > @@ -1784,6 +1784,7 @@ init_opcode_hash (const struct riscv_opcode
>> *opcodes,
>> > help us resolve the corresponding low-part relocation later. */
>> > typedef struct
>> > {
>> > + segT seg;
>>
>> Just one minor comment here: segT really being asection *, I think it
>> would
>> be desirable to use const asection * here instead, to clarify that there's
>> no intent of changing the referenced section. Not using the pre-cooked
>> segT
>> may not be very nice, but (a) there's precedent
>> (obj-elf.c:match_section())
>> and (b) I think const-correctness is more important overall.
>>
>> In the end it'll be Nelson anyway to approve the change.
>>
>> Jan
>>
>
Thanks a lot!
Lifang
> 2024年7月4日 21:49，Nelson Chu <nelson@rivosinc.com> 写道：
>
> Committed after passing the regression of riscv-gnu-toolchain, with the const asection change.
>
> Thanks for all
> Nelson
>
> On Thu, Jul 4, 2024 at 5:18 PM Nelson Chu <nelson@rivosinc.com <mailto:nelson@rivosinc.com>> wrote:
>> The const asection * sounds reasonable and good to me, thanks for the suggestion :-)
>>
>> Nelson
>>
>> On Thu, Jul 4, 2024 at 2:46 PM Jan Beulich <jbeulich@suse.com <mailto:jbeulich@suse.com>> wrote:
>>> On 04.07.2024 03:56, lifang_xia@linux.alibaba.com <mailto:lifang_xia@linux.alibaba.com> wrote:
>>> > --- a/gas/config/tc-riscv.c
>>> > +++ b/gas/config/tc-riscv.c
>>> > @@ -1784,6 +1784,7 @@ init_opcode_hash (const struct riscv_opcode *opcodes,
>>> > help us resolve the corresponding low-part relocation later. */
>>> > typedef struct
>>> > {
>>> > + segT seg;
>>>
>>> Just one minor comment here: segT really being asection *, I think it would
>>> be desirable to use const asection * here instead, to clarify that there's
>>> no intent of changing the referenced section. Not using the pre-cooked segT
>>> may not be very nice, but (a) there's precedent (obj-elf.c:match_section())
>>> and (b) I think const-correctness is more important overall.
>>>
>>> In the end it'll be Nelson anyway to approve the change.
>>>
>>> Jan
@@ -1784,6 +1784,7 @@ init_opcode_hash (const struct riscv_opcode *opcodes,
help us resolve the corresponding low-part relocation later. */
typedef struct
{
+ segT seg;
bfd_vma address;
symbolS *symbol;
bfd_vma target;
@@ -1798,7 +1799,13 @@ static hashval_t
riscv_pcrel_fixup_hash (const void *entry)
{
const riscv_pcrel_hi_fixup *e = entry;
- return (hashval_t) (e->address);
+
+ /* the pcrel_hi with same address may reside in different segments,
+ to ensure uniqueness, the segment ID needs to be included in the
+ hash key calculation.
+ Temporarily using the prime number 499 as a multiplier, but it
+ might not be large enough. */
+ return e->address + 499 * e->seg->id;
}
/* Compare the keys between two entries fo the pcrel_hi hash table. */
@@ -1807,16 +1814,17 @@ static int
riscv_pcrel_fixup_eq (const void *entry1, const void *entry2)
{
const riscv_pcrel_hi_fixup *e1 = entry1, *e2 = entry2;
- return e1->address == e2->address;
+ return e1->seg->id == e2->seg->id
+ && e1->address == e2->address;
}
/* Record the pcrel_hi relocation. */
static bool
-riscv_record_pcrel_fixup (htab_t p, bfd_vma address, symbolS *symbol,
+riscv_record_pcrel_fixup (htab_t p, segT seg, bfd_vma address, symbolS *symbol,
bfd_vma target)
{
- riscv_pcrel_hi_fixup entry = {address, symbol, target};
+ riscv_pcrel_hi_fixup entry = {seg, address, symbol, target};
riscv_pcrel_hi_fixup **slot =
(riscv_pcrel_hi_fixup **) htab_find_slot (p, &entry, INSERT);
if (slot == NULL)
@@ -4676,11 +4684,10 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg ATTRIBUTE_UNUSED)
bfd_vma value = target - md_pcrel_from (fixP);
/* Record PCREL_HI20. */
- if (!riscv_record_pcrel_fixup (riscv_pcrel_hi_fixup_hash,
- md_pcrel_from (fixP),
- fixP->fx_addsy,
- target))
- as_warn (_("too many pcrel_hi"));
+ if (!riscv_record_pcrel_fixup (riscv_pcrel_hi_fixup_hash, seg,
+ md_pcrel_from (fixP), fixP->fx_addsy,
+ target))
+ as_warn (_("too many pcrel_hi"));
bfd_putl32 (bfd_getl32 (buf)
| ENCODE_UTYPE_IMM (RISCV_CONST_HIGH_PART (value)),
@@ -4698,7 +4705,7 @@ md_apply_fix (fixS *fixP, valueT *valP, segT seg ATTRIBUTE_UNUSED)
and set fx_done for -mno-relax. */
{
bfd_vma location_pcrel_hi = S_GET_VALUE (fixP->fx_addsy) + *valP;
- riscv_pcrel_hi_fixup search = {location_pcrel_hi, 0, 0};
+ riscv_pcrel_hi_fixup search = {seg, location_pcrel_hi, 0, 0};
riscv_pcrel_hi_fixup *entry = htab_find (riscv_pcrel_hi_fixup_hash,
&search);
if (entry && entry->symbol