[PATCH] D47969: [Verifier] Check that ValueAsMetadata belongs to the module

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D47969#1131661, @vlad.tsyrklevich wrote:

> In https://reviews.llvm.org/D47969#1129699, @dexonsmith wrote:
>
> > IIRC, sharing uniqued metadata across modules should only happen as a temporary state.  It's a trick for when loading modules that will be merged together, to avoid unnecessarily creating multiple copies of the graphs.
> >
> > I feel like the verifier should reject this state, unless it's specifically being called in a context where modules-to-be-merged are still being loaded.
>
>
> The crash noted in the test occurs during cross-module importing, does that count as merging? Both of the modules are independently codegened, not merged together, but there is importing. To me it seems like cross-module references are a temporary state while merging for monolithic LTO, but not a temporary state under ThinLTO.


Yes this is essentially merging.  For ThinLTO importing we still IRMove in the source module, but pass in the set of globals to move so that it is selective. The assert shown in the patch description here is happening after the source module has been loaded by the importer, but just before we have done the IRMove. So this is very much in a temporary state. Note that the caller of the function importer (crossImportIntoModule) does call the verifier again right after all importing is complete. It would be interesting to see if the assert is hit there if we can somehow skip it during the call from UpgradeDebugInfo.

The source modules being imported from are loaded separately from the memory buffers when they are codegened by a different thread.


Repository:
  rL LLVM

https://reviews.llvm.org/D47969