[PATCH] D13814: Add support for SHF_MERGE sections

[Rafael Espíndola via llvm-commits]

On 19 October 2015 at 13:39, Rui Ueyama <ruiu at google.com> wrote:
> ruiu added a comment.
>
> I don't think splitting the loop would produce substantially duplicated code. Even if code in total gets larger, it's a good thing to do as long as each loop becomes more clear. I didn't try that myself though. You can leave it as is for now if you want. I'll take a look at that later.

OK. Note that the loop reduced a bit with r250702. If reducing it
further my suggestion would be to put the body in a helper function.


> Mix of Base and llvm::object::ElfFile<ELFT> looks a bit odd. I'd always use llvm::object::ElfFile<ELFT> because that is one less indirection.

Done.

> ================
> Comment at: ELF/OutputSections.cpp:467-468
> @@ -448,1 +466,4 @@
> +    Offset += getAddend<ELFT>(RI);
> +  Offset -= (Offset % Section->getSectionHdr()->sh_entsize);
> +  return VA + cast<MergeInputSection<ELFT>>(Section)->getOffset(Offset);
>  }
> ----------------
> Is this correct? Offset is a rounded value, and the return value does not seem to include the information you discarded using the % operator.

No. Very nice catch. I will upload a new patch in a minute.

> ================
> Comment at: ELF/OutputSections.h:208
> @@ +207,3 @@
> +private:
> +  // This map is used to find if we already have an etry for a given value and,
> +  // if so, at what offset it is.
> ----------------
> entry (typo)

Fixed.

Cheers,
Rafael