[PATCH] Refactoring of Driver and TargetInfo

[Shankar Kalpathi Easwaran]

================
Comment at: lib/Core/Resolver.cpp:313
@@ -313,5 +312,3 @@
   // error message about missing symbols
-  if (!undefinedAtoms.empty() &&
-      (!_targetInfo.getLinkerOptions()._noInhibitExec ||
-       _targetInfo.getLinkerOptions()._outputKind == OutputKind::Relocatable)) {
+  if (!undefinedAtoms.empty() && _targetInfo.printRemainingUndefines()) {
     // FIXME: need diagonstics interface for writing error messages
----------------
kledzik at apple.com wrote:
> Shankar Kalpathi Easwaran wrote:
> > kledzik at apple.com wrote:
> > > Shankar Kalpathi Easwaran wrote:
> > > > looks like printRemainingUndefines is silently performing the effect of noinhibit-exec option too, which it shouldnt. A no inhibit exec option should be provided by core. 
> > > > 
> > > > $cat x.c
> > > > extern int a;
> > > > 
> > > > int _start() { a = 10; }
> > > > $gcc -c x.c
> > > > $ld x.o -noinhibit-exec
> > > > x.o: In function `_start':
> > > > x.c:(.text+0x6): undefined reference to `a'
> > > > 
> > > I thought --noinhibit-exec meant "do not delete existing (old) file if link fails".  But after reading more man pages, it looks to mean "write output file, even if it is it contains errors".  
> > > The GnuLdDriver currently only calls setPrintRemainingUndefines(false) for -r mdoe and --allow-undefines.   I needed to invent --allow-undefines to get some elf cases to pass without writing to stderr.  What should --noinhibit-exec? Does it mean turn errors into warnings?  All errors or just undefined symbosl?
> > -noinhibit-exec is an option valid only when creating the executable.
> > 
> > If -noinhibit-exec option is passed, does the following :-
> > 
> > a) prints undefined symbols as errors
> > b) moves onto create the executable
> > 
> > If -noinhibit-exec is not passed, the linker aborts when there are undefined symbols
> Can you clarify "valid only when creating the executable" .  Does that mean it is invalid for making DSO's?    So it still prints undefines and still returns 1 for the tool status.  The big difference is that it writes the output file?  What about other errors?  This implies there is a way to encode undefined symbols in the final output?  
DSO's can have unresolved symbols, and they are usually called through the Procedure Linkage Table(GOT). The noinhibit-exec option is still valid while creating DSO's but there is no effect. 

usually when there is a undefined symbol, the linker aborts and does not output the final executable. With noinhibit exec, it does go and write the final output file. 

Yes there is a way to encode undefined symbols in the final output. They are just treated as Undefined symbols in the final ELF image.

================
Comment at: lib/Driver/CoreDriver.cpp:144-149
@@ +143,8 @@
+  
+  // Handle --add-pass xxx option
+  for (llvm::opt::arg_iterator it = parsedArgs->filtered_begin(OPT_add_pass),
+                               ie = parsedArgs->filtered_end();
+                              it != ie; ++it) {
+    info.addPassNamed((*it)->getValue());
+  }
+
----------------
kledzik at apple.com wrote:
> Shankar Kalpathi Easwaran wrote:
> > Shankar Kalpathi Easwaran wrote:
> > > I think there is a need for a --remove-pass option too, for targets that have set few options by default.
> > ?
> lld -core runs no passes by default.  You need to explicitly add the passes you want to be run for a particular test case.
The reason I thought of the remove pass option, was to just workaround bugs for specific usecases, and try out what would happen if the usecase is run with few options removed. 

================
Comment at: lib/Driver/GnuLdDriver.cpp:181-196
@@ +180,18 @@
+  
+  // Handle --allow-undefines
+  if (parsedArgs->getLastArg(OPT_allow_undefines))
+    options->setPrintRemainingUndefines(false);
+  
+  // Handle --force-load
+  if (parsedArgs->getLastArg(OPT_force_load))
+    options->setForceLoadAllArchives(true);
+  
+  // Handle --merge-strings
+  if (parsedArgs->getLastArg(OPT_merge_strings))
+    options->setMergeCommonStrings(true);
+  
+  // Handle -t
+  if (parsedArgs->getLastArg(OPT_t))
+    options->setLogInputFiles(true);
+  
+  // Handle -Lxxx
----------------
kledzik at apple.com wrote:
> Shankar Kalpathi Easwaran wrote:
> > Shankar Kalpathi Easwaran wrote:
> > > -dead-strip/ --add-pass is missing in target. Isnt it better for the targets to call the Core to parse the options and set appropriate values in the targetInfo structure, otherwise if there is an option added in core, you have to duplicate handling that option in all targets.
> > ?
> The whole patch changes the driver relationships.  "core" is just a driver used for test cases.  It is not "inherited" by any other drivers.  Each driver needs to set XXXTargetInfo settings based on its own command line language.  Since, the GnuLdDriver is supposed to be a drop in replacement for binutil's ld we need to follow that command line syntax.  If there already is an option that means to do dead stripping, we should wire it up to setDeadStrip().  We might also want to consider a naming convention for all lld extensions to the gnu command line.  For instance an "lld" prefix, like:  --lld-add-pass.
ok.


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

BRANCH
  svn

ARCANIST PROJECT
  lld