[PATCH] D15191: [ELF] Support PHDRS command

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 16 12:45:34 PST 2015


ruiu added inline comments.

================
Comment at: ELF/Writer.cpp:57
@@ +56,3 @@
+                             StringRef SecName) const;
+  IndicesVector getIndicesForPhdrs(const std::vector<StringRef> &Phdrs,
+                                   StringRef SecName) const;
----------------
Since IndicesVector is now just a std::vector, I'd write std::vector<size_t> instead of IndicesVector.

================
Comment at: ELF/Writer.cpp:106
@@ -95,1 +105,3 @@
+  llvm::StringMap<std::vector<llvm::StringRef>> OutputSectionToPhdrs;
+  llvm::SmallMapVector<llvm::StringRef, IndicesVector, 8> PhdrNameToIndices;
 };
----------------
Ditto

================
Comment at: ELF/Writer.cpp:912
@@ +911,3 @@
+  for (StringRef Name : Phdrs) {
+    auto It = PhdrNameToIndices.find(Name);
+    if (It == std::end(PhdrNameToIndices))
----------------
Please do not use `auto` here as the actual type is not obvious in this local context.

================
Comment at: ELF/Writer.cpp:916-917
@@ +915,4 @@
+            Name);
+    std::copy(std::begin(It->second), std::end(It->second),
+              std::back_inserter(PhdrInds));
+  }
----------------
PhdrInds.insert(PhdrIns.end(), std::begin(It->second), std::end(It->second)) is more common?

================
Comment at: ELF/Writer.cpp:937
@@ -873,4 +936,3 @@
 
-  // Add the first PT_LOAD segment for regular output sections.
-  setPhdr(&Phdrs[++PhdrIdx], PT_LOAD, PF_R, 0, Target->getVAStart(), FileOff,
-          Target->getPageSize());
+// Assign output sections to headers specified in linker script's PHDRS command.
+template <class ELFT>
----------------
Can you please write more details about how you process PHDR command for those who happens to come to this file and read this code (not for me who is reading only the delta), so that they can get the whole picture what you are doing with this function (and the helper functions you added above) without reading all the code?


http://reviews.llvm.org/D15191





More information about the llvm-commits mailing list