[PATCH] [lld][ELF] Add linker script ENTRY command

[Shankar Kalpathi Easwaran]

  The main concern I have mirrors BigCheese concern.

  The set function should be removed from TargetInfo class anyways.

  Rather than copying the TargetInfo, lld needs to convert the options that are specified in the LinkerScript to ELFTargetInfo, and be it contained there.

  BigCheese ?


================
Comment at: lib/ReaderWriter/LinkerScript.cpp:21-34
@@ -20,26 +20,16 @@
   switch (_kind) {
-  case Token::eof:
-    os << "eof: ";
-    break;
-  case Token::identifier:
-    os << "identifier: ";
-    break;
-  case Token::kw_as_needed:
-    os << "kw_as_needed: ";
-    break;
-  case Token::kw_group:
-    os << "kw_group: ";
-    break;
-  case Token::kw_output_format:
-    os << "kw_output_format: ";
-    break;
-  case Token::l_paren:
-    os << "l_paren: ";
-    break;
-  case Token::r_paren:
-    os << "r_paren: ";
-    break;
-  case Token::unknown:
-    os << "unknown: ";
+#define CASE(name)                              \
+  case Token::name:                             \
+    os << #name ": ";                           \
     break;
+  CASE(eof)
+  CASE(identifier)
+  CASE(kw_as_needed)
+  CASE(kw_entry)
+  CASE(kw_group)
+  CASE(kw_output_format)
+  CASE(l_paren)
+  CASE(r_paren)
+  CASE(unknown)
+#undef CASE
   }
----------------
This will be hard to debug through a debugger. lets not stick to macros.

================
Comment at: include/lld/Core/TargetInfo.h:248-257
@@ -247,2 +247,12 @@
 
+  /// This method is called by core linking to fix missing options that were
+  /// not specified by linker script or object file.
+  ///
+  /// On GNU, many linker options can be specified by linker script. On
+  /// Windows, object files may contain linker options. Because of that
+  /// reason, we do not have a complete list of linker options until all input
+  /// files are parsed. This method is supposed to set missing options that
+  /// were not specified neither by command line nor linker script or object
+  /// file.
+  virtual void maybeSetDefaultOptions() { };
 
----------------
This should not be in the base TargetInfo class and the function not exposed to the user. 

================
Comment at: lib/Driver/Driver.cpp:29
@@ -28,3 +28,3 @@
 /// This is where the link is actually performed.
-bool Driver::link(const TargetInfo &targetInfo, raw_ostream &diagnostics) {
+bool Driver::link(TargetInfo &targetInfo, raw_ostream &diagnostics) {
   // Honor -mllvm
----------------
Rui Ueyama wrote:
> I tried not to remove "const" from this method, but eventually I had to, because "how and what to link" can be altered by parsing files. Not only linker script, but Windows .obj files can change targetInfo, so I thought that this change makes sense.
This should be const. We should not modify user supplied TargetInfo as this is owned by the user(When lld is used in API context).

================
Comment at: lib/Driver/Driver.cpp:58
@@ +57,3 @@
+  // Give target a chance to fix missing linker options.
+  targetInfo.maybeSetDefaultOptions();
+
----------------
This should be probably converted to ELFTargetInfo and the values set in the ELFTargetInfo class.

================
Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:163
@@ -168,1 +162,3 @@
 
+void ELFTargetInfo::maybeSetDefaultOptions() {
+  // Set the default entry point name.
----------------
Please change the name to setDefaultOptions than maybeSetDefaultOptions. 

================
Comment at: lib/ReaderWriter/ReaderLinkerScript.cpp:85
@@ -84,3 +84,3 @@
                             std::vector<std::unique_ptr<File> > &result) const {
   auto lsf = LinkerScriptFile::create(_targetInfo, std::move(mb));
   if (!lsf)
----------------
The _targetInfo should be changed to _elfTargetInfo I think.


http://llvm-reviews.chandlerc.com/D833

BRANCH
  linker-script-entry

ARCANIST PROJECT
  lld