Re: [PATCH 1/4] [gdb/symtab] Fix htab_find_slot call in read_call_site_scope

[Tom de Vries via Gdb-patches <gdb-patches@sourceware.org>]

On 10/4/21 5:41 PM, Simon Marchi wrote:
> On 2021-10-04 08:46, Tom de Vries wrote:
>> On 10/4/21 2:05 PM, Simon Marchi wrote:
>>>>> Ah, I had not seen this comment.  So it was on purpose.  Still, I think
>>>>> that it makes it more confusing than anything.  The patch LGTM.
>>>>
>>>> And, this follow-up commit reverts everything except the comment.
>>>>
>>>> Any comments?
>>>>
>>>> Thanks,
>>>> - Tom
>>>>
>>>
>>> Is there a problem with having the lookups done with just the pc? 
>>
>> Well, there's the problem that I describe in the commit message.  I
>> don't known of any other problem.
>>
>>> If we
>>> were to replace this with some C++ hash table, say std::unordered_map, it
>>> would be std::unordered_map<CORE_ADDR /* pc */, call_site>. 
>>
>> Right, because there's a separation between key and element.
>>
>>> So doing
>>> the lookups using just the pc in the htab makes sense to me.
>>
>> I'm sorry, I don't really understand what you're trying to say here.
>>
>> Do you agree with the patch?  Do you disagree with the patch.  Are you
>> suggesting an alternative solution?
> 
> My thinking was: why not keep core_addr_eq and core_addr_hash, and pass
> a pointer to a CORE_ADDR when calling htab_find_slot.  And drop the
> requirement for call_site::pc to be the first member of call_site.  But
> I now realize that this may not be a correct use of htab: htab_find
> passes entries to the eq func.  So if we have core_addr_eq as the eq
> func and htab passes a call_site* entry to core_addr_eq, it won't work.
> Now, we don't actually use htab_find, so it would still work for us.
> But that would be like setting up a trap for whoever tries to use
> htab_find in the future.
> 

All find variants (with the explicit exception of
find_empty_slot_for_expand) call eq_f on both an element argument and a
hash table element.  Consequently, if we break first-field-type
compatibility between the two, the element argument must be of the same
type as the hash table element, and the eq function must use hash table
element type.

This is not a future problem, this is an actual problem I ran into when
I tried it out moving the pc to the second field position, and the
problem is described in the commit log.

> So, the patche LGTM.

Ack, thanks for the review, I'll commit.

Thanks,
- Tom