Re: [PATCH 4/4] gdb: change regcache list to be a map

Simon Marchi
2020-07-30T16:26:26Z

On 2020-07-24 12:59 p.m., John Baldwin wrote:
> On 7/23/20 6:53 PM, Pedro Alves wrote:
>> On 7/20/20 9:41 PM, Simon Marchi via Gdb-patches wrote:
>>> The function regcache_thread_ptid_changed is called when a thread
>>> changes ptid. It is implemented efficiently using the map, although
>>> that's not very important: it is not called often, mostly when creating
>>> an inferior, on some specific platforms.
>>>
>>> Note: In hash_target_ptid, I am combining hash values from std::hash by
>>> summing them. I don't think it's ideal, since std::hash is just the
>>> identity function for base types. But I don't know what would be better
>>> to reduce the change of collisions. If anybody has a better idea, I'd
>>> be interested.
>>
>> I'd maybe look in some kernel sources, e.g., Linux or BSD, what they
>> use as hash function for pids.
>
> FreeBSD just does a very simple and with a power of 2 based on the
> maximum number of processes (set via a boot-time tunable).
>
> However, it has separate hash tables for pids and thread ids (tids).
> It doesn't have a single hash table of threads indexed by (pid, tid)
> because the tid namespace is independent and a tid is a fully unique
> name to a kernel thread (lwp).
>
>>> +/* Functor to hash a ptid. */
>>> +
>>> +struct hash_ptid
>>> +{
>>> + size_t operator() (const ptid_t &ptid) const
>>> + {
>>> + std::hash<long> long_hash;
>>> +
>>> + return (long_hash (ptid.pid ())
>>> + + long_hash (ptid.lwp ())
>>> + + long_hash (ptid.tid ()));
>>> + }
>>> +};
>
> I think summing the three components might be the best option.
> Presumably the low bits of the hash are what actually get used and
> so the goal would be to have more entropy there. This means you
> probably would _not_ want to do something like:
>
> pid << 32 | lwp << 16 | tid
>
> since many native backends will have tid of all zeroes. It would
> also not be ideal for backends that only do processes (so only
> pid is non-zero). The sum approach degrades to the right thing
> for those targets without needing extra complication or conditionals.

Ok, thanks for the tips. So I'll probably keep it like this for now then.

I'll still take a look at the kernel implementations like Pedro suggested.

Simon