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

[Rui Ueyama via llvm-commits]

ruiu added a comment.

It is generally looking good.


================
Comment at: ELF/Driver.cpp:354
@@ -353,2 +353,3 @@
 
+  Config->ZCombReloc = !hasZOption(Args, "nocombreloc");
   Config->ZExecStack = hasZOption(Args, "execstack");
----------------
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.

================
Comment at: ELF/OutputSections.cpp:306
@@ -305,2 +305,3 @@
     : OutputSectionBase<ELFT>(Name, Config->Rela ? SHT_RELA : SHT_REL,
                               SHF_ALLOC) {
+  this->Sort = Sort;
----------------
  SHF_ALLOC), Sort(Sort) {

================
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;
----------------
Please use uint8_t instead of unsigned char.

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

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

================
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)
----------------
I'd use X and Y (or whatever) rather than IdxA and IdxB because they have very narrow scopes.

================
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)
----------------
You don't need these temporary variables because you can use A.r_offset and B.r_offset directly.

================
Comment at: ELF/OutputSections.cpp:364-365
@@ +363,4 @@
+
+  if (!Sort)
+    return;
+
----------------
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

================
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});
----------------
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?

================
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);
----------------
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.

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

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


http://reviews.llvm.org/D19528