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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D122974#3536342 <https://reviews.llvm.org/D122974#3536342>, @llunak wrote:

> 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>).

My udnerstanding is that generally ABI stability is intended to be guaranteed across minor releases (& those patches don't look like ABI breaks to me - the first one changes the mangling of intended-to-be-local symbols so they don't collide, so it should cause valid programs to link when they previously failed to link. The second seems to add a new target that wasn't present - so nothing to break against?) but that's probably orthogonal to the stability of the map/cache/expectations of folks releasing and using lldb.

>> 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.

Ah, good stuff - doesn't guarantee that any hash change necessarily breaks the test, but certainly helps/seems like a good idea, thanks!

> 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.

Sorry, I wasn't meaning to discuss dynamic library versioning issues/mismatches, just static/fully matched versions.

> 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.

That something seems to have been another part of lldb - and that's the sort of change I'd like to enable/not make harder by avoiding more dependence on this ordering/hashing.

> 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.

Yeah, a little traits class or the like is roughly what I'd picture.


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

https://reviews.llvm.org/D122974