[PATCH] D119721: [clang][lex] Use `ConstSearchDirIterator` in lookup cache

[Stephan Bergmann via Phabricator via cfe-commits]

sberg added subscribers: rnk, sberg.
sberg added a comment.
Herald added a project: All.

I started to experience Clang crashing when building Firebird (as part of building LibreOffice) in clang-cl mode on Windows, and I think it is due to this change in combination with D2733 <https://reviews.llvm.org/D2733> by @rnk:

When `HeaderSearch::LookupFile` exits through one of the `if (checkMSVCHeaderSearch...` branches introduced in D2733 <https://reviews.llvm.org/D2733>, it does not update `CacheLookup.HitIt` (through one of the calls to `cacheLookupSuccess` or via the "Otherwise, didn't find it" step at the end of the function), even though it already may have done the "We will fill in our found location below, so prime the start point value" step of calling `CacheLookup.reset`.  That means that a later call to `HeaderSearch::LookupFile` can go into the `if (!SkipCache && CacheLookup.StartIt == NextIt)` true branch, set `It = CacheLookup.HitIt` to an invalid iterator (the default `HitIt = nullptr` value), and then assert/crash when dereferencing that invalid `It`.

Which was not an issue before this change, as the initial `HitIdx = 0` was not an invalid value, so `i = CacheLookup.HitIdx` never caused `i` to become invalid.

I locally worked around this in my build with

  -  if (!SkipCache && CacheLookup.StartIt == NextIt) {
  +  if (!SkipCache && CacheLookup.StartIt == NextIt && CacheLookup.HitIt) {

which seems to work well, but I'm not sure what the best real fix would be.  (D2733 <https://reviews.llvm.org/D2733> stated "I believe the correct behavior here is to avoid updating the cache when we find the header via MSVC's search rules.")


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119721