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

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/LinkerScript.cpp:286
@@ +285,3 @@
+  ssize_t RelroNum = -1;
+  Phdr *Load = nullptr;
+  uintX_t Flags = PF_R;
----------------
grimar wrote:
> evgeny777 wrote:
> > grimar wrote:
> > > evgeny777 wrote:
> > > > Well, ErrorOr<T> looks a way too heavy to represent valid/invalid array index, doesn't it?
> > > > 
> > > Right. That is why I suggested to use int32_t/uint32_t depending on what you need here. It is what we generally use in lld for such cases.
> > > But we do not use int and never used ssize_t yet I think.
> > There is plenty of int and plenty of size_t. Unfortunately I don't know any windows analog for 'grep -w', so just beleive me :)
> > Yes, there isn't any ssize_t, right, but on the other hand - why not use it?
> Well, I think there should be a reason for using something new, like new type in you code. What is the reason for you ?
> We used other types all the way before, so at least there is a reason not to use ssize_t - and that readon is consistency. 
> At the same time that is just my opinion here, I would not intoduce using of new type, but lets wait for another reviewers comments then.
I have a concern about `ssize_t` because (unlike `size_t`) it is not in the C standard. It is defined in POSIX, so it may be unavailable on non-Unix systems. So I'd prefer `int32_t` or just `int`. (I don't have a strong preference here because it is unrealistic we have more than 2^31 elements.)

================
Comment at: ELF/LinkerScript.cpp:628-633
@@ +627,8 @@
+      Tok = next();
+      if (Tok == "FILEHDR")
+        PhdrCmd.HasFilehdr = true;
+      else if (Tok == "PHDRS")
+        PhdrCmd.HasPhdrs = true;
+      else if (Tok == ";")
+        break;
+      else
----------------
Can you break as early as possible?

  if (Tok == ";")
    break;
  if (Tok == "FILEHDR")
    ...

================
Comment at: ELF/LinkerScript.h:101
@@ -86,2 +100,3 @@
+  bool hasPhdrsCommands();
 private:
   // "ScriptConfig" is a bit too long, so define a short name for it.
----------------
Add a blank line before `private:`.

================
Comment at: ELF/Writer.cpp:1303-1311
@@ +1302,10 @@
+
+template bool elf::isRelroSection<ELF32LE>(OutputSectionBase<ELF32LE> *Sec);
+template bool elf::isRelroSection<ELF32BE>(OutputSectionBase<ELF32BE> *Sec);
+template bool elf::isRelroSection<ELF64LE>(OutputSectionBase<ELF64LE> *Sec);
+template bool elf::isRelroSection<ELF64BE>(OutputSectionBase<ELF64BE> *Sec);
+
+template bool elf::needsPtLoad<ELF32LE>(OutputSectionBase<ELF32LE> *Sec);
+template bool elf::needsPtLoad<ELF32BE>(OutputSectionBase<ELF32BE> *Sec);
+template bool elf::needsPtLoad<ELF64LE>(OutputSectionBase<ELF64LE> *Sec);
+template bool elf::needsPtLoad<ELF64BE>(OutputSectionBase<ELF64BE> *Sec);
----------------
`* Sec` -> `*` (we usually omit parameter names here)


http://reviews.llvm.org/D21995