[PATCH] RISC-V: hash with segment id and pcrel_hi address while recording pcrel_hi

[lifang_xia]

> 2024年6月25日 16:25，Jan Beulich <jbeulich@suse.com> 写道：
> 
> On 25.06.2024 06:31, Nelson Chu wrote:
>> On Fri, Jun 21, 2024 at 9:25 AM <lifang_xia@linux.alibaba.com> wrote:
>> 
>>> 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.
>>> 
>> 
>> Yeah, this makes sense.
>> 
>> 
>>> @@ -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.
>>> +     There is no hash for two integer, refer to ctf_hash_integer.  */
>>> +  return htab_hash_pointer ((void *)(uintptr_t)e->seg->id)
>>> +         + 59 * htab_hash_pointer ((void *)(uintptr_t)e->address);
>>> }
>>> 
>> 
>> The riscv_segment_info_type is used to record target stuff for special
>> works.  We used to record mapping symbol stuff there.  Maybe we can have a
>> pcrel_hi hash table with address as key, in each riscv_segment_info_type if
>> there is/are pcrel_hi instructions, and have another summarized hash table
>> with segment id as key.  I am not familiar with the ctf_hash_integer stuff,
>> so not sure which solution is better.  I think Alan, Nick and Jan know
>> these better since they are experts, so cc them, and hope they may have
>> time to give us some suggestions ;)
>> 
>> 
>> Hi Alan, Nick, Jan,
>> 
>> We met the problem that there are no internal hash tables that can use two
>> keys - one key is the segment id, the other is the riscv auipc (pcrel hi)
>> instruction address in the segment.  Do you think it's fine to refer to how
>> ctf_hash_integer works in this patch?  Or we should have multiple hash
>> tables for each segment in riscv_segment_info_type if there are auipc
>> instructions (auipc address of segment as key), and have another one hash
>> table to collect the segment (segment id as key) which has any auipc.
> 
> In principle that's fine, I think, just that the reference to
> ctf_hash_integer() seems misleading. The two similar CTF functions are
> ctf_hash_type_key() and ctf_hash_type_id_key(), afaics.
Em.. Sorry, It’s a mistake. I went to comment with “ctf_hash_type_key”.

> 
> I don't really understand, though, why htab_hash_pointer() wants / needs
> using here. Wouldn't (deriving from what was there before)
> "e->address + 59 * e->seg->id" suffice (multiplier subject to possible
> improvement), and be less of a behavioral change?
"e->address + 59 * e->seg->id” would be fine. It's just necessary 
to ensure uniqueness within a finite range. 
As for the multiplier, do you have any suggestions? I feel that 59 might be
too small. Is 127 a better choice?
> Jan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/binutils/attachments/20240702/7df32603/attachment.htm>