[PATCH] D105328: [Frontend] Only compile modules if not already finalized

Tue Jul 6 20:57:27 PDT 2021

bnbarham added a comment.

In D105328#2861028 <https://reviews.llvm.org/D105328#2861028>, @vsapsai wrote:

> This problem shouldn't happen based on module-level name hashing but for some reason it is happening. If I'm not mistaken, InMemoryModuleCache uses .pcm file name as the key. But that file name should contain a hash part that is based on the modulemap path. But frameworks M in "frameworks" and "frameworks2" should have different paths for modulemaps and should have different .pcm names, therefore no conflicts in InMemoryModuleCache.

Ah sorry, looks like I had my modules slightly confused. The underlying problem is that `B` is considered out of date due to the check on line 2920, ie. the one that creates the `err_imported_module_relocated` diagnostic. That causes `Top` to be re-compiled as a module it depends on is out of date. The location of `Top` hasn't changed, so it keeps the same .pcm path.

`B` is also re-compiled, but as you point out that's perfectly fine since it'll get a different path in the cache.

I'll update the comments in the test + the review summary. Sorry for the confusion.

> I see the value in not crashing but looks like there might be a problem somewhere else.

For what it's worth, even if there was another cause I'd still like to get this change in - writing over a module in the cache causes all sorts of different crashes that are hard to diagnose. I'd be *very* surprised if there weren't more issues like this, so at least we'll get an actual error rather than random crashes.

> Also it is more of my personal pet peeve. InMemoryModuleCache should simplify memory management for modules but in practice it simplifies creating dangling references. I'm not asking to address that, so if you feel like my requests get too close to that direction, it's good to push back.

Heh, I find this a nice summary of how I feel about it after trying to find the root cause of these crashes!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105328