[PATCH] D53596: [ThinLTO] Fix a crash in lazy loading of Metadata

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D53596#1287638, @dexonsmith wrote:

> In https://reviews.llvm.org/D53596#1287633, @dexonsmith wrote:
>
> > Hmm.  I remember writing this code.  I have a faint recollection of proving at the time that these could never be forward refs, and that it was important for bitcode reading performance.
>
>
> Sorry, I neglected to say anything actionable.
>
> - If any bitcode writer in a released Clang has written forward refs for scope/inline-at, then clearly we need to handle this in the reader.  But if that's not the case, perhaps we can instead change the writer to make it impossible again.  (I think I might have intentionally used `dyn_cast` instead of `dyn_cast_or_null` as an assertion in the reader.)


It looks like this code was already handling forward refs for the scope/inline-at, since it was calling getMDNodeFwdRefOrNull - so is it possible that changed at some point?

> - Either way, you might look at a bitcode-reading profile and see if this is still something to be careful about for compile-time.
> - Note: I wrote that code way back before we had lazy loading of metadata and I don't remember if/how things changed there.

For lazy loading of metadata, I don't think it matters whether it is a forward reference originally, we won't load it until we need it.

I don't think the patch changes whether it handles forward references, or am I misunderstanding how the code currently works?


Repository:
  rL LLVM

https://reviews.llvm.org/D53596