[PATCH] add support for merging common strings

[Michael Spencer]

================
Comment at: include/lld/ReaderWriter/RelocationHelperFunctions.h:16-30
@@ +15,17 @@
+
+class RelocationError {
+
+public:
+  enum RelocationErrorKind {
+    RelocOK,
+    RelocOverflow,
+    RelocIgnore,
+    RelocError
+  };
+
+  RelocationError(int32_t k) : _kind(k) {}
+
+private:
+  int32_t _kind;
+};
+
----------------
We have enum class so there's no need to use this. Also, the value names don't follow LLVM convention. Although since this is an enum class, there's no need to have any prefix at all.

================
Comment at: lib/ReaderWriter/ELF/Atoms.h:572-573
@@ +571,4 @@
+  uint64_t _ordinal;
+  unsigned int _referenceStartIndex;
+  unsigned int _referenceEndIndex;
+  int32_t _offset;
----------------
These are unneeded.

================
Comment at: lib/ReaderWriter/ELF/Atoms.h:574
@@ +573,3 @@
+  unsigned int _referenceEndIndex;
+  int32_t _offset;
+};
----------------
What is this for? It's currently dead.

================
Comment at: lib/ReaderWriter/ELF/File.h:74
@@ +73,3 @@
+    int64_t operator()(const MergeSectionKey &k) const {
+      return (k._shdr->sh_name << 8) + k._offset;
+    }
----------------
Use hash_combine.

================
Comment at: lib/ReaderWriter/ELF/File.h:72-84
@@ +71,15 @@
+
+  struct MergeSectionKeyHash {
+    int64_t operator()(const MergeSectionKey &k) const {
+      return (k._shdr->sh_name << 8) + k._offset;
+    }
+  };
+
+  struct MergeSectionKeyEq {
+    bool operator()(const MergeSectionKey &lhs, 
+                    const MergeSectionKey &rhs) const {
+      return ((lhs._shdr->sh_name == rhs._shdr->sh_name) && 
+              (lhs._offset == rhs._offset));
+    }
+  };
+
----------------
You can merge these two.

================
Comment at: lib/ReaderWriter/ELF/File.h:225-226
@@ +224,4 @@
+      for (i = 0; i < sectionContents.size(); ++i) {
+        switch (sectionContents[i]) {
+        case '\0':
+          tokens.push_back(new (_readerStorage) 
----------------
if (sectionContents[i] == '\0')

================
Comment at: lib/ReaderWriter/ELF/File.h:236-241
@@ +235,8 @@
+
+      if (prev != sectionContents.size()) 
+        tokens.push_back(new (_readerStorage) 
+                         MergeString(prev, 
+                                     sectionContents.slice(prev, i), 
+                                     msi,
+                                     sectionName));
+    }
----------------
Spec says that all strings are null terminated.

================
Comment at: lib/ReaderWriter/ELF/File.h:225-226
@@ +224,4 @@
+      for (i = 0; i < sectionContents.size(); ++i) {
+        switch (sectionContents[i]) {
+        case '\0':
+          tokens.push_back(new (_readerStorage) 
----------------
Michael Spencer wrote:
> if (sectionContents[i] == '\0')
This is also ignoring sh_entsize.

================
Comment at: lib/ReaderWriter/ELF/File.h:210
@@ +209,3 @@
+    // TODO
+    // a) Remove the atom content to not hold \0
+    // b) Create a seperate section chunk to write mergeable atoms
----------------
Why? The null terminator is needed.

================
Comment at: lib/ReaderWriter/ELF/File.h:468
@@ +467,3 @@
+                             llvm::ELF::SHF_STRINGS)) {
+          const MergeSectionKey ms(shdr, ri->offsetInAtom()+ri->addend());
+          ri->setTarget(_mergedSectionMap[ms]);
----------------
Spaces around +.

Also, the spec doesn't require that relocations point to the beginning of of strings.

================
Comment at: lib/ReaderWriter/ELF/File.h:501
@@ -359,2 +500,3 @@
 private:
+
   ELFDefinedAtom<ELFT> *createDefinedAtomAndAssignRelocations(
----------------
Don't add whitespace here.

================
Comment at: include/lld/ReaderWriter/RelocationHelperFunctions.h:16
@@ +15,3 @@
+
+class RelocationError {
+
----------------
Michael Spencer wrote:
> We have enum class so there's no need to use this. Also, the value names don't follow LLVM convention. Although since this is an enum class, there's no need to have any prefix at all.
This isn't used anywhere.

================
Comment at: lib/ReaderWriter/ELF/Atoms.h:471
@@ +470,3 @@
+  virtual StringRef name() const {
+    return StringRef("");
+  }
----------------
  return "";


http://llvm-reviews.chandlerc.com/D387