<div dir="ltr"><div>I also want to solve the problem of this patch (make ELFLinkingContext own the buffers) without mixing with future directions on cooking linkerscript AST. However, we will essentially end up with a vector of BumpPtrAllocators in ELFLinkingContext (one for each linker script), which makes the code, in my opinion, much harder to understand (it's better to have "hey, these are a bunch of linkerscripts" than "hey, here are a bunch of unnamed buffers that need to be anchored somewhere"). What are the benefits of this? We avoid including LinkerScript.h into ELFLinkingContext.h, but that is something that we will eventually need to do to avoid rewriting all classes in LinkerScript.h in their "cooked" version, don't you think?<br></div><div><br></div><div>For example, if you look specifically at the problem of mapping input sections to output sections, we already have classes that encode this same information in the script namespace (script::OutputSectionDescription in the raw script AST) and in SectionChunks.h (template class lld::OutputSection<>), used by DefaultLayout. I wanted to avoid creating yet another class such as "CookedOutputSectionInfo", and that's why I had an intent on preserving the original linker script AST structure in the LinkingContext -- mostly to avoid rewriting the same info in different data structures that stores the same information that is already exposed in the AST.</div><div><br></div><div>Let me know your thoughts and thanks for reviewing this.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Feb 1, 2015 at 9:01 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I didn't think hard enough about that, but it might be good to have a class to cook linker scripts. But if you have such class, you can instantiate that class in the driver and only set that class instance to the linking context, no? You wouldn't have to add a pointer to linker script ASTs to the linking context, I guess.<br>
<br>
I prefer a patch that does one thing; in this case, we should try to fix the unsafe memory access first, so I'd fix that without adding any new feature.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://reviews.llvm.org/D7323" target="_blank">http://reviews.llvm.org/D7323</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div><br></div>