[PATCH] D27070: Implement STARTUP linker script command.

[George Rimar via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/LinkerScript.cpp:1249
 
+void ScriptParser::readStartup() { Config->Startup = readParenLiteral(); }
+
----------------
May be better to inline this ?


================
Comment at: test/ELF/linkerscript/startup.s:4
+# RUN: mkdir -p %t.dir
+# RUN: cd %t.dir
+
----------------
This creates "startup.s.tmp.dir" folder for me inside tools\lld\test\ELF\linkerscript\Output.
And looks that is the only folder inside at this moment.
So out tests previously did not do that and that is inconsistency.

Do you plan to do that for all tests ?


================
Comment at: test/ELF/linkerscript/startup.s:13
+# RUN: ld.lld -o exe -script=script foo.o bar.o
+# RUN: llvm-objdump -s exe | FileCheck -check-prefix=FOO %s
+
----------------
llvm-objdump -s -section=.text ?


================
Comment at: test/ELF/linkerscript/startup.s:16
+# FOO: Contents of section .text:
+# FOO: 201000 aaaaaaaa aaaaaaaa ffffffff ffffffff
+
----------------
I would suggest not to check the address here. Latest changes like max page size shows
that we have lot of testcase failing for reasons they should not.


================
Comment at: test/ELF/linkerscript/startup.s:29
+
+# WARN: nosuchfile: STARTUP file not found
----------------
STARTUP file 'nosuchfile' not found ?


https://reviews.llvm.org/D27070