[PATCH] [ELF] Refactor File.h.

[Shankar Kalpathi Easwaran]

  You might want to run the below tests before checkin :-

  1) x86_64, static using C
  2) x86_64, static with --merge-strings option using C
  3) x86_64 dynamic using C
  4) x86_64 dynamic with --merge-string using C code
  5) x86_64 static c++

  For (C) what I use is to : print Hello world
  For C++ what I use is to print a message from constructor and destructor.


================
Comment at: lib/ReaderWriter/ELF/File.h:235
@@ -235,3 +234,3 @@
     std::vector<MergeString *> tokens;
-    for (auto msi : _mergeStringSections) {
+    for (const Elf_Shdr *msi : _mergeStringSections) {
       StringRef sectionContents;
----------------
why is this change from auto to const Elf_Shdr ?

================
Comment at: lib/ReaderWriter/ELF/File.h:255
@@ -255,3 +254,3 @@
     // Create Mergeable atoms
-    for (auto tai : tokens) {
+    for (const MergeString *tai : tokens) {
       ArrayRef<uint8_t> content((const uint8_t *)tai->_string.data(),
----------------
same here.

================
Comment at: lib/ReaderWriter/ELF/File.h:431-432
@@ +430,4 @@
+        // the anonymous atom that we created.
+        if (anonAtom)
+          createEdge(newAtom, anonAtom, lld::Reference::kindLayoutAfter);
+
----------------
You can possibly make the check in createEdge on the second parameter, makes it cleaner.

================
Comment at: lib/ReaderWriter/ELF/File.h:422-428
@@ -482,10 +421,9 @@
 
-        // If the atom was a weak symbol, lets create a followon
-        // reference to the anonymous atom that we created
-        if ((*si)->getBinding() == llvm::ELF::STB_WEAK && anonAtom) {
-          ELFReference<ELFT> *wFollowedBy = new (_readerStorage)
+        // If this is the last atom, lets not create a followon reference.
+        if (anonAtom && (si + 1) != se) {
+          anonFollowedBy = new (_readerStorage)
               ELFReference<ELFT>(lld::Reference::kindLayoutAfter);
-          wFollowedBy->setTarget(anonAtom);
-          newAtom->addReference(wFollowedBy);
+          anonAtom->addReference(anonFollowedBy);
         }
 
+        // If the atom was a weak symbol, lets create a followon reference to
----------------
Is this out of indentation ?

================
Comment at: lib/ReaderWriter/ELF/File.h:413-415
@@ -444,33 +412,5 @@
 
-          // If this is the last atom, lets not create a followon
-          // reference
-          if ((si + 1) != se)
-            anonFollowedBy = new (_readerStorage)
-               ELFReference<ELFT>(lld::Reference::kindLayoutAfter);
-          anonPrecededBy = new (_readerStorage)
-              ELFReference<ELFT>(lld::Reference::kindLayoutBefore);
-          // Add the references to the anonymous atom that we created
-          if (anonFollowedBy)
-            anonAtom->addReference(anonFollowedBy);
-          anonAtom->addReference(anonPrecededBy);
-          if (previous_atom)
-            anonPrecededBy->setTarget(previous_atom);
-          contentSize = 0;
-        }
-
-        ArrayRef<uint8_t> symbolData = ArrayRef<uint8_t>(
-            (uint8_t *)sectionContents.data() + (*si)->st_value, contentSize);
-
-        // If the linker finds that a section has global atoms that are in a
-        // mergeable section, treat them as defined atoms as they shouldnt be
-        // merged away as well as these symbols have to be part of symbol
-        // resolution
-        if (isMergeableSection(section)) {
-          if ((*si)->getBinding() == llvm::ELF::STB_GLOBAL) {
-            auto definedMergeAtom = new (_readerStorage) ELFDefinedAtom<ELFT>(
-                *this, symbolName, sectionName, (*si), section, symbolData,
-                _references.size(), _references.size(), _references);
-            _definedAtoms._atoms.push_back(definedMergeAtom);
-          }
-          continue;
+          if (previousAtom)
+            createEdge(anonAtom, previousAtom,
+                       lld::Reference::kindLayoutBefore);
         }
----------------
You can possibly check if the second parameter is null and return from createEdge.

================
Comment at: lib/ReaderWriter/ELF/File.h:655
@@ +654,3 @@
+    // not. Let the TargetHandler to make a decision if that's the case.
+    if (isProcessorSpecificAtom(section, symbol)) {
+      TargetHandler<ELFT> &targetHandler =
----------------
isProcessorSpecificAtom -> isTargetSpecificAtom

================
Comment at: test/elf/X86_64/weak-override.test:28-30
@@ -30,2 +27,5 @@
 WEAKATOMSORDER:        target:          fn
+WEAKATOMSORDER:      - kind:            layout-after
+WEAKATOMSORDER:        offset:          0
+WEAKATOMSORDER:        target:          main
 WEAKATOMSORDER:  - name:            main
----------------
How did the order of the reference change just for this test ?


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