[PATCH] [ELF] Refactor File.h.

[Rui Ueyama]

  This patch failed to link a hello world program with --merge-strings. Fix has been uploaded.

  I will add a test to catch this error. Looks like there's already a test for --merge-strings but it did not catch this case.


================
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;
----------------
Shankar Kalpathi Easwaran wrote:
> why is this change from auto to const Elf_Shdr ?
Because I think this is more readable. _mergeStringSection is not defined in this file, and it's not easy to get its type at first sight. The type is the same.

================
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(),
----------------
Shankar Kalpathi Easwaran wrote:
> same here.
the same reason

================
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);
         }
----------------
Shankar Kalpathi Easwaran wrote:
> You can possibly check if the second parameter is null and return from createEdge.
Yeah, if I make such change I would rename createEdge -> maybeCreateEdge. I think this is a matter of taste, but I feel like for a function that creates an edge, the precondition is the existence of nodes. My preference is to keep this null check outside of the function and keep the function as well defined as possible.

================
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
----------------
Shankar Kalpathi Easwaran wrote:
> Is this out of indentation ?
Which line are you pointing to? This blank line?

================
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 =
----------------
Shankar Kalpathi Easwaran wrote:
> isProcessorSpecificAtom -> isTargetSpecificAtom
done

================
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
----------------
Shankar Kalpathi Easwaran wrote:
> How did the order of the reference change just for this test ?
I don't remember exactly, but while I was editing code for weak symbols the order changed, and I modified the test because the order does not matter.


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