[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

Tue May 24 23:45:48 PDT 2022

llunak added a comment.

In D122974#3535669 <https://reviews.llvm.org/D122974#3535669>, @dblaikie wrote:

>> It doesn't make sense to require a stable hash algorithm for an internal cache file.
>
> It's at least a stronger stability requirement than may be provided before - like: stable across process boundaries, at least, by the sounds of it? yeah?

It's meant to be the same for the same library binary.

> & then still raises the question for me what about minor version updates, is it expected to be stable across those?

Is anything expected to be stable across those? If so, that doesn't seem to be the reality of it (a random quick check finds two ABI changes just between 14.0.4 and 14.0.5, e6de9ed37308e46560243229dd78e84542f37ead <https://reviews.llvm.org/rGe6de9ed37308e46560243229dd78e84542f37ead> and 53433dd0b5034681e1866e74651e8527a29e9364 <https://reviews.llvm.org/rG53433dd0b5034681e1866e74651e8527a29e9364>).

> It'd be a bit subtle to have to know when to go and update the lldb cache version number because this hash function has changed, for instance. It might be more suitable to have lldb explicitly request a known hash function rather than the generic one (even if they're identical at the moment) so updates to LLVM's core libraries don't subtly break the hashing/cache system here. (I would guess there's no cross-version hash testing? So it seems like such a change could produce fairly subtle breakage that would slip under the radar fairly easily?)

D124704 <https://reviews.llvm.org/D124704> adds a unittest that compares StringMap::hash() to a known hardcoded value, so whenever the hash implementation changes, it won't be possible to unittest LLDB with that change, and that will be the time to change the lldb cache version. If the theory is that this should keep working even with the library changing without LLDB rebuild, then as I wrote above that theory needs double-checking first. And additionally a good question to ask would be if it's really a good idea to do incompatible implementation changes to a class that has half of its functionality inline. Finally, there have been already attempts to change the hash function to use the better non-zero seed (D97396 <https://reviews.llvm.org/D97396>), and they were reverted because something depended on the hash function not changing, so I don't see it that likely for the hash function to change just like that in a minor update.

But if after all this that's still the theory, I guess StringMap could get an additional template parameter specifying the hash function, if it's actually worth the effort.

@clayborg : BTW, this is exactly the reason why I wanted the cache files header to include a hash value for a known string.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122974/new/

https://reviews.llvm.org/D122974