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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
