[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#1288273, @tejohnson wrote:

> 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?


Ping on this comment/question - I don't think I am changing anything here on the handling of forward references in the non-lazy loading case, and I believe the need to handle them as "forward refs" is inherent in the lazy loading.


Repository:
  rL LLVM

https://reviews.llvm.org/D53596