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

Simon Marchi via Gdb-patches
2021-10-04T15:42:14Z

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.

So, the patche LGTM.

Simon