[PATCH] add support for merging common strings

[Shankar Kalpathi Easwaran]

================
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;
+};
+
----------------
Michael Spencer wrote:
> 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.
Will remove this class.

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

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

================
Comment at: lib/ReaderWriter/ELF/Atoms.h:574
@@ +573,3 @@
+  unsigned int _referenceEndIndex;
+  int32_t _offset;
+};
----------------
Michael Spencer wrote:
> What is this for? It's currently dead.
I am planning to use that to search for a mergeable atom, where in the relocation offset is not the start of the merge atom.

================
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;
+    }
----------------
Michael Spencer wrote:
> Use hash_combine.
hash_combine takes a string, integer, .... I initially did that and reverted to this.

================
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));
+    }
+  };
+
----------------
Michael Spencer wrote:
> You can merge these two.
Ok.

================
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:
> Michael Spencer wrote:
> > if (sectionContents[i] == '\0')
> This is also ignoring sh_entsize.
The resolver doesnot support multibyte characters currently I think. 

================
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));
+    }
----------------
Michael Spencer wrote:
> Spec says that all strings are null terminated.
ah ok, cool.

================
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]);
----------------
Michael Spencer wrote:
> Spaces around +.
> 
> Also, the spec doesn't require that relocations point to the beginning of of strings.
Will that not be a relocation to the symbol that holds the data, than the data itself ?

for example :

const char *foo = "foo";

char fn() {
return foo+3;
}

relocation is here is relative to foo.

(or)

const char *foo1 = "foo";
const char *bar = (char *)&foo1+1;

The relocation here is also to foo1


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