[PATCH] D48247: lld: add experimental support for SHT_RELR sections.

[Rahul Chaudhry via llvm-commits]

On Fri, Jul 6, 2018 at 11:22 AM Rui Ueyama <ruiu at google.com> wrote:

> On Fri, Jul 6, 2018 at 10:45 AM Rahul Chaudhry via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> rahulchaudhry added inline comments.
>>
>>
>> ================
>> Comment at: ELF/Relocations.cpp:717-723
>> +                             RelType Type) {
>> +  // Add a relative relocation. If RelrDyn section is enabled, and the
>> +  // relocation offset is guaranteed to be even, add the relocation to
>> +  // the RelrDyn section, otherwise add it to the RelaDyn section.
>> +  // RelrDyn sections don't support odd offsets. Also, RelrDyn sections
>> +  // don't store the addend values, so we must write it to the relocated
>> +  // address.
>> ----------------
>> ruiu wrote:
>> > 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.
>> >
>> > Now that I understood the entire patch, I'll rewrite this patch after
>> you commit this change so that we can proceed, though.
>> I cannot commit this change as I do not have write access.
>> I won't ask you to commit it either, since it is clear that you are not
>> happy with this change.
>> You've indicated a couple of times that you would like to rewrite this
>> patch.
>> I would suggest that you re-implement the functionality to your liking,
>> and simply commit that instead.
>> Unfortunately, I will not be able to spend any more time on this.
>> Thanks for your time,
>> Rahul
>>
>
> 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.
>

Okay.. that's reasonable.
Let's move past the misunderstanding and land it then?

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.
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!!)
All this clearly demonstrates great design, as well as good taste when it
comes to implementation details.
Thanks, and keep up the good work,
Rahul
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180706/f35b84f3/attachment.html>