[PATCH] D21995: [ELF] Implement minimal PHDRS parser and section-to-segment assignment

[Rui Ueyama via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM with these changes. Thanks!


================
Comment at: ELF/LinkerScript.cpp:290
@@ +289,3 @@
+
+  for (const PhdrsCommand &Hdr : Opt.PhdrsCommands) {
+    Phdrs.emplace_back(Hdr.HeaderType, PF_R);
----------------
Again, it is hard to read if you call both ELF program headers and linker script's PHDRS command "headers". Rename Hdr -> Cmd.

================
Comment at: ELF/LinkerScript.cpp:306
@@ +305,3 @@
+      if (!isOutputDynamic<ELFT>())
+        continue;
+      Added.H.p_flags = toPhdrFlags(Out<ELFT>::Dynamic->getFlags());
----------------
`continue` and `break` are interchangeable in this context, but mixing them is confusing. Please be consistent -- I'd use `break` here.

================
Comment at: ELF/LinkerScript.cpp:321
@@ +320,3 @@
+      if (Out<ELFT>::EhFrame->empty() || !Out<ELFT>::EhFrameHdr)
+        continue;
+      Added.H.p_flags = toPhdrFlags(Out<ELFT>::EhFrameHdr->getFlags());
----------------
Ditto

================
Comment at: ELF/LinkerScript.cpp:339
@@ +338,3 @@
+    std::vector<size_t> HeaderIds =
+        Script<ELFT>::X->getPhdrIndicesForSection(Sec->getName());
+
----------------
You can remove `Script<ELFT>::X->`.

================
Comment at: ELF/LinkerScript.cpp:732
@@ -588,1 +731,3 @@
 
+void ScriptParser::readOutputSectionHeaders() {
+  while (!Error && peek().startswith(":")) {    
----------------
This should be readOutputSectionPhdrs.

================
Comment at: ELF/LinkerScript.cpp:739
@@ +738,3 @@
+    else
+      Opt.Commands.back().Headers.push_back(Tok); 
+  } 
----------------
Return a vector of StringRefs from this function instead of mutating a member.

================
Comment at: ELF/LinkerScript.cpp:752
@@ +751,3 @@
+
+  PhdrsCommand &Cmd = Opt.PhdrsCommands.back();
+  StringRef Tok = next();
----------------
Mutating the last phdrs command here is too subtle. You want to make this function return a phdr type instead of setting it to a member and returning nothing.

================
Comment at: ELF/LinkerScript.h:50
@@ -48,1 +49,3 @@
   StringRef Name;
+  std::vector<StringRef> Headers;
+};
----------------
We are handling various types of headers, so "Headers" is confusing. Rename Phdrs.


https://reviews.llvm.org/D21995