<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I’m sorry, I’m out of work for a few weeks, and I won’t be able to review it correctly. However a couple of things stood out when I looked at it last week:<br></blockquote><div><br></div><div>No problem, thanks for the reply - my apologies if my reply sounded harsh, I've just been carrying some version of this patch locally for about a year and carrying LLVM patches is a pain, even if they're small.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
 - you definitely should separate the debug line fix from the RelocVisitor implementation.<br></blockquote><div><br></div><div>Ok, but the problem is that I'm not sure I can cook up a test case in ELF that will properly demonstrate this problem. Similarly, the test case for MachO fails without the bugfix. I suppose we can get the MachO RelocVisitor implementation in with an incorrect test case and then have the bugfix change that test case. Does that sound reasonable?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
 - MachO object files contain another relocation type (X86_64_RELOC_SUBTRACTOR if memory serves), and I don’t know if it really fits in the RelocVisitor model (as it needs access to the following reloc to make sense). Why is RELOC_UNSIGNED sufficient for your usecase?<br></blockquote><div><br></div><div>I haven't had LLVM generate anything else on my test cases, so I didn't need to implement it. I think it's easier to implement them one at a time with proper test cases, which I didn't have for the X86_64_RELOC_SUBTRACTOR case. I'd be happy to implement the other relocations, but I think it make sense to get the simple case settled first. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Fred<br>
<div><div><br>
><br>
> REPOSITORY<br>
>  rL LLVM<br>
><br>
> <a href="http://reviews.llvm.org/D8148" target="_blank">http://reviews.llvm.org/D8148</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>
<br>
</div></div></blockquote></div><br></div></div>