<div dir="ltr">I'll land this patch for you.<br><div class="gmail_quote"><div dir="ltr"><br></div><div dir="ltr">On Fri, Jul 6, 2018 at 12:35 PM Rahul Chaudhry <<a href="mailto:rahulchaudhry@chromium.org">rahulchaudhry@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Jul 6, 2018 at 11:22 AM Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Jul 6, 2018 at 10:45 AM Rahul Chaudhry via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">rahulchaudhry added inline comments.<br>
<br>
<br>
================<br>
Comment at: ELF/Relocations.cpp:717-723<br>
+ RelType Type) {<br>
+ // Add a relative relocation. If RelrDyn section is enabled, and the<br>
+ // relocation offset is guaranteed to be even, add the relocation to<br>
+ // the RelrDyn section, otherwise add it to the RelaDyn section.<br>
+ // RelrDyn sections don't support odd offsets. Also, RelrDyn sections<br>
+ // don't store the addend values, so we must write it to the relocated<br>
+ // address.<br>
----------------<br>
ruiu wrote:<br>
> Well, this comment explains what you are doing in this function, but that's what I can find by reading this function, so I don't find this very helpful. I'd explain the intention instead. For example, this comment should answer to the question why the offset must be aligned to 2, what is RelrDyn in the first place, etc.<br>
> <br>
> Now that I understood the entire patch, I'll rewrite this patch after you commit this change so that we can proceed, though.<br>
I cannot commit this change as I do not have write access.<br>
I won't ask you to commit it either, since it is clear that you are not happy with this change.<br>
You've indicated a couple of times that you would like to rewrite this patch.<br>
I would suggest that you re-implement the functionality to your liking, and simply commit that instead.<br>
Unfortunately, I will not be able to spend any more time on this.<br>
Thanks for your time,<br>
Rahul<br></blockquote><div><br></div><div>My apologies. I meant I would update this "comment" and not the "patch". Rewriting the entire patch doesn't make sense at all, that's just a silly typo, and my intention is to save your time. My typo unfortunately made my comment arrogant. I'm sorry, I didn't mean it.</div></div></div></blockquote><div><br></div><div>Okay.. that's reasonable.</div><div>Let's move past the misunderstanding and land it then?</div><div><br></div><div>I would like to add here that it was a pleasure working on this patch for LLD. I did not expect the ELF linker code to be remotely as approachable as it was.</div><div><div style="font-size:small;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">And the speed is amazing. I saw it link chromeos-chrome in 13 seconds! (and that was a debug build of LLD linking a debug build of chrome!!)<br></div></div><div><div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial;background-color:rgb(255,255,255)">All this clearly demonstrates great design, as well as good taste when it comes to implementation details.<br></div><div style="font-size:small;text-decoration-style:initial;text-decoration-color:initial;background-color:rgb(255,255,255)">Thanks, and keep up the good work,</div></div></div></div></blockquote><div><br></div><div>Thank you for your kind words! This patch should improve lld even more, especially for reduction in output file size, and I expect resulting executables should load faster. Thank you for the improvement!</div></div></div>