[PATCH] D19528: [ELF] - Implemented -z combrelocs/nocombreloc.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/Driver.cpp:354
@@ -353,2 +353,3 @@
 
+  Config->ZCombReloc = !hasZOption(Args, "nocombreloc");
   Config->ZExecStack = hasZOption(Args, "execstack");
----------------
ruiu wrote:
> Let's name ZCombreloc as this is a one-word option (if it were -z no-comb-reloc, then I'd name it ZCombReloc). Maybe we want to change ZExecStack to ZExecstack too.
Done.

================
Comment at: ELF/OutputSections.cpp:306
@@ -305,2 +305,3 @@
     : OutputSectionBase<ELFT>(Name, Config->Rela ? SHT_RELA : SHT_REL,
                               SHF_ALLOC) {
+  this->Sort = Sort;
----------------
ruiu wrote:
>   SHF_ALLOC), Sort(Sort) {
Done, but I dont sure I like how it looks after clang-format.

================
Comment at: ELF/OutputSections.cpp:323-324
@@ +322,4 @@
+
+  unsigned char AType = A.getType(Config->Mips64EL);
+  unsigned char BType = B.getType(Config->Mips64EL);
+  bool ARel = AType == Target->RelativeRel;
----------------
ruiu wrote:
> Please use uint8_t instead of unsigned char.
Done.

================
Comment at: ELF/OutputSections.cpp:329
@@ +328,3 @@
+    return true;
+  else if (!ARel && BRel)
+    return false;
----------------
ruiu wrote:
> Remove `else` after return.
Done.

================
Comment at: ELF/OutputSections.cpp:331
@@ +330,3 @@
+    return false;
+  else if (!ARel && !BRel) {
+    // Both are not R_*_RELATIVE, sort by symbol index.
----------------
ruiu wrote:
> Ditto.
Done.

================
Comment at: ELF/OutputSections.cpp:333-334
@@ +332,4 @@
+    // Both are not R_*_RELATIVE, sort by symbol index.
+    uint32_t IdxA = A.getSymbol(Config->Mips64EL);
+    uint32_t IdxB = B.getSymbol(Config->Mips64EL);
+    if (IdxA != IdxB)
----------------
ruiu wrote:
> I'd use X and Y (or whatever) rather than IdxA and IdxB because they have very narrow scopes.
Done.

================
Comment at: ELF/OutputSections.cpp:341-342
@@ +340,4 @@
+  // both relative relocations. Sort by address in this case.
+  uintX_t OffA = A.r_offset;
+  uintX_t OffB = B.r_offset;
+  if (OffA != OffB)
----------------
ruiu wrote:
> You don't need these temporary variables because you can use A.r_offset and B.r_offset directly.
Done.

================
Comment at: ELF/OutputSections.cpp:364-365
@@ +363,4 @@
+
+  if (!Sort)
+    return;
+
----------------
ruiu wrote:
> Let's invert this condition and add comment before `if` about `-z combreloc` option.
> 
>   // Sort relocations by r_offset, breaking ties with other conditions.
>   // Doing this does not change the semantics nor the file format, but
>   // some dynamic linker can load executables/DSOs faster when the
>   // relocation table is sorted. The fact that we have sorted relocations 
>   // is recorded using DT_RELACOUNT tag.
>   // http://people.redhat.com/jakub/prelink.pdf
Done, but:
1) "The fact that we have sorted relocations is recorded using DT_RELACOUNT tag." was not quite correct, I removed it. DT_RELACOUNT just keeps amount of Relative relocations at start of rela.dyn. We can have zero of such relocations and DT_RELACOUNT entry will absent. But relocations will still be sorted.  That how golds works, and it seems reasonable: there is no point in zero filled tag about relative relocations, we will still have some boost after sorting others one.
2) Changed link to Ian Lance Tailor notes about it.

================
Comment at: ELF/OutputSections.cpp:643-644
@@ -597,1 +642,4 @@
          uintX_t(IsRela ? sizeof(Elf_Rela) : sizeof(Elf_Rel))});
+    if (uint64_t N = Out<ELFT>::RelaDyn->getRelativeCount())
+      if (Config->ZCombReloc)
+        Add({IsRela ? DT_RELACOUNT : DT_RELCOUNT, N});
----------------
ruiu wrote:
> Swap the two `if`s so that we don't call getRelativeCount if the feature is disabled.
> 
> Also, does it make sense to not emit DT_RELACOUNT if there's no relative relocations?
DT_RELACOUNT == 0 just wastes file space and does not affect anything I think.
It looks like excessive job for lld to generate it and for dynamic linker to proccess the
empty record.
gold do the same, btw.

================
Comment at: ELF/OutputSections.h:232
@@ -231,3 +231,3 @@
 public:
-  RelocationSection(StringRef Name);
+  RelocationSection(StringRef Name, bool Sort = false);
   void addReloc(const DynamicReloc<ELFT> &Reloc);
----------------
ruiu wrote:
> I wouldn't use default argument lightly because it is virtually defining two different functions, with and without the parameter. I prefer passing the argument explicitly.
Done.

================
Comment at: ELF/OutputSections.h:244
@@ +243,3 @@
+  bool Sort;
+  uint64_t Relatives = 0;
+
----------------
ruiu wrote:
> NumRelativeRels may be better.
Done.

================
Comment at: ELF/Writer.cpp:132
@@ +131,3 @@
+
+  StringRef RelDynName = Config->Rela ? ".rela.dyn" : ".rel.dyn";
+  RelocationSection<ELFT> RelaDyn(RelDynName, Config->ZCombReloc);
----------------
ruiu wrote:
> I wouldn't define this because it is used only once.
Done.


http://reviews.llvm.org/D19528