[PATCH] D16771: [ELF] PHDRS linker script command implemented.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/LinkerScript.cpp:195-197
@@ +194,5 @@
+StringRef ScriptParser::peek() {
+  if (atEOF())
+    error("unexpected EOF");
+  return Tokens[Pos];
+}
----------------
ruiu wrote:
> error() no longer call exit(), so you have to handle errors gracefully. In this case, if atEOF(), Token[Pos] is invalid.
yep, I know, just missed that place, sorry.

================
Comment at: ELF/LinkerScript.cpp:358
@@ +357,3 @@
+    error("Integer can't be parsed: " + Tok);
+  return Val;
+}
----------------
ruiu wrote:
> Do not return an undefined value.
That was done intentionally. My point was that using of the return value in case of error is also a error (at least for that method), error flag should be checked, value can be accessedm but should not be used in any results because of invalid nature. Undefined value accepts to this requirements.
When you look the value of undefined variable it is usually clear it is the one, but defining it to something can hide the actual error.

================
Comment at: ELF/LinkerScript.cpp:380
@@ +379,3 @@
+      else
+        error("Unknown header attribute " + Tok);
+    }
----------------
ruiu wrote:
> Handle errors gracefully.
Sorry, not sure I see something wrong here also. skip() has a check for error flag, so if something wrong here, it will just exit finally from both of while loops. There is no need of excessive error flags here and that is why I think it is gracefully already.

================
Comment at: ELF/Writer.cpp:1209
@@ +1208,3 @@
+template <class ELFT> void Writer<ELFT>::createPhdrsFromScript() {
+  // Reserve for total PHDRS amount + 1 for "NONE" synthetic PHDR.
+  Phdrs.reserve(Script->Phdrs.size() + 1);
----------------
ruiu wrote:
> grimar wrote:
> > Btw, is it supposed that createPhdrsFromScript() stuff should go to LinkerScript.cpp ?
> > Problem is that here the field of Writer is changed (Phdrs) and also methods and <ELFT> is used which I am not sure would look good in linkerscript files.
> I think we probably want to move this to LinkerScript.cpp. Also this function is too long. Is there any way to reduce the amount of code and split into smaller pieces?
I`ll try and update the patch later. Please clarify my comments above.


http://reviews.llvm.org/D16771