[PATCH] Refactoring of Driver and TargetInfo
Shankar Kalpathi Easwaran
shankarke at gmail.com
Thu Mar 28 16:39:35 PDT 2013
Layout pass is set by default to true in the ELF target. This has to be set to true.
================
Comment at: include/lld/Core/TargetInfo.h:166-168
@@ +165,5 @@
+ /// trace different parts of LLVM and lld.
+ const std::vector<const char*> &llvmOptions() const {
+ return _llvmOptions;
+ }
+
----------------
should this be std::vector<std::string> ?
================
Comment at: include/lld/Core/TargetInfo.h:214-216
@@ +213,5 @@
+ }
+ void appendLLVMOption(const char *opt) {
+ _llvmOptions.push_back(opt);
+ }
+
----------------
this could take a std::string
================
Comment at: lib/Driver/Driver.cpp:33-36
@@ +32,6 @@
+ unsigned numArgs = targetInfo.llvmOptions().size();
+ const char **args = new const char*[numArgs + 2];
+ args[0] = "lld (LLVM option parsing)";
+ for (unsigned i = 0; i != numArgs; ++i)
+ args[i + 1] = targetInfo.llvmOptions()[i];
+ args[numArgs + 1] = 0;
----------------
A memory leak, the function doesnot free the memory on return, do you want to use a BumpAllocator ?
================
Comment at: include/lld/ReaderWriter/CoreTargetInfo.h:29-30
@@ +28,4 @@
+
+ virtual llvm::Triple getTriple() const {
+ return llvm::Triple(llvm::Triple("x86_64", "apple", "darwin"));
+ }
----------------
hexagon,x86 is missing here.
================
Comment at: include/lld/ReaderWriter/MachOTargetInfo.h:21-23
@@ -23,1 +20,5 @@
+namespace mach_o {
+ class KindHandler; // defined in lib. this header is in include.
+}
+
----------------
ELF has renamed this to RelocationHandler
================
Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:32
@@ +31,3 @@
+ , _mergeCommonStrings(false)
+ , _runLayoutPass(false) {
+}
----------------
run LayoutPass is true by default for ELF
================
Comment at: include/lld/Core/TargetInfo.h:69
@@ +68,3 @@
+ bool globalsAreDeadStripRoots() const {
+ return _globalsAreDeadStripRoots;
+ }
----------------
should this be deadStrip() && _globalsAreDeadStripRoots, to prevent an accidental use of globalsAreDeadStripRoots without checking for deadStrip ?
================
Comment at: include/lld/Core/TargetInfo.h:89-99
@@ +88,13 @@
+
+ /// Archive files (aka static libraries) are normally lazily loaded. That is,
+ /// object files within an archive are only loaded and linked in, if the
+ /// object file contains a DefinedAtom which will replace an existing
+ /// UndefinedAtom. If this method returns true, core linking will also look
+ /// for archive members to replace existing tentative definitions in addition
+ /// to replacing undefines. Note: a "tentative definition" (also called a
+ /// "common" symbols) is a C (but not C++) concept. They are modeled in lld
+ /// as a DefinedAtom with merge() of mergeAsTentative.
+ bool searchArchivesToOverrideTentativeDefinitions() const {
+ return _searchArchivesToOverrideTentativeDefinitions;
+ }
+
----------------
Does tentative definition also refer to weak symbols ? If no, then you might want to add another option to search archives for overriding weak symbols too.
================
Comment at: include/lld/Core/TargetInfo.h:122-129
@@ -52,2 +121,10 @@
- virtual uint64_t getPageSize() const = 0;
+ /// In the lld model, a SharedLibraryAtom is a proxy atom for something
+ /// that will be found in a dynamic shared library when the program runs.
+ /// A SharedLibraryAtom optionally contains the name of the shared library
+ /// in which to find the symbol name at runtime. Core linking may merge
+ /// two SharedLibraryAtom with the same name. If this method returns true,
+ /// when merging core linking will also verify that they both have the same
+ /// loadName() and if not print a warning.
+ ///
+ /// \todo This should be a method core linking calls so that drivers can
----------------
couldnt follow this.
================
Comment at: include/lld/Core/TargetInfo.h:151-154
@@ +150,6 @@
+
+ /// If true, core linking will write the path to each input file to stdout
+ /// (i.e. llvm::outs()) as it is used. This is used to implement the -t
+ /// linker option.
+ ///
+ /// \todo This should be a method core linking calls so that drivers can
----------------
There is also -y <symbol> which traces a symbol.
================
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
----------------
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'
================
Comment at: lib/Driver/Driver.cpp:49-53
@@ +48,7 @@
+ error_code ec = targetInfo.readFile(input.getPath(), files);
+ if (ec) {
+ diagnostics << "Failed to read file: " << input.getPath() << ": "
+ << ec.message() << "\n";
+ return true;
+ }
+ inputs.appendFiles(files);
----------------
extra spaces
================
Comment at: lib/Driver/GnuLdDriver.cpp:177-180
@@ +176,6 @@
+
+ // Handle --noinhibit-exec
+ if (parsedArgs->getLastArg(OPT_noinhibit_exec))
+ options->setNoInhibitExec(true);
+
+ // Handle --allow-undefines
----------------
This option is not checked now in the resolver, TargetInfo has to provide a function for noinhibit-exec.
================
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());
+ }
+
----------------
I think there is a need for a --remove-pass option too, for targets that have set few options by default.
================
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
----------------
-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.
================
Comment at: lib/ReaderWriter/ELF/Atoms.h:183-186
@@ -183,1 +182,6 @@
+ , _targetAtomHandler(nullptr) {
+ static uint64_t orderNumber = 0;
+ // llvm::errs() << "ELFDefinedAtom(" << (void*)this << ", name=" << symbolName << ")\n";
+ _ordinal = ++orderNumber;
+ }
----------------
This was recently removed by Michael :-
------------------------------------------------------------------------
r177561 | mspencer | 2013-03-20 14:25:34 -0500 (Wed, 20 Mar 2013) | 1 line
[ELF][Reader] Remove static ordinal.
------------------------------------------------------------------------
================
Comment at: lib/ReaderWriter/ELF/Atoms.h:507-511
@@ -502,3 +506,7 @@
: _owningFile(file), _sectionName(sectionName), _section(section),
- _contentData(contentData), _offset(offset) {}
+ _contentData(contentData), _offset(offset) {
+ static uint64_t orderNumber = 0;
+ //llvm::errs() << "ELFMergeAtom(this=" << (void*)this << ", sect=" << sectionName << ")\n";
+ _ordinal = ++orderNumber;
+ }
----------------
------------------------------------------------------------------------
r177561 | mspencer | 2013-03-20 14:25:34 -0500 (Wed, 20 Mar 2013) | 1 line
[ELF][Reader] Remove static ordinal.
------------------------------------------------------------------------
Recently removed.
================
Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:45-46
@@ -48,3 +44,4 @@
void ELFTargetInfo::addPasses(PassManager &pm) const {
- pm.add(std::unique_ptr<Pass>(new LayoutPass()));
+ if (_runLayoutPass)
+ pm.add(std::unique_ptr<Pass>(new LayoutPass()));
}
----------------
same here, run layout pass is true by default. This was what I was saying previously, we might need a --remove-pass option to optionally remove passes.
================
Comment at: lib/ReaderWriter/ReaderArchive.cpp:48-50
@@ -48,5 +47,5 @@
return nullptr;
- LinkerInput li(std::unique_ptr<MemoryBuffer>(buff.take()));
- if (_getReader(li)->parseFile(li.takeBuffer(), result))
+ std::unique_ptr<MemoryBuffer> mb(buff.take());
+ if (_targetInfo.parseFile(mb, result))
return nullptr;
----------------
You would need to add logInputFiles here to trace the archive file that was pulled in.
================
Comment at: lib/ReaderWriter/ReaderArchive.cpp:174-176
@@ -176,5 +173,5 @@
return ec;
- LinkerInput li(std::unique_ptr<MemoryBuffer>(buff.take()));
- if ((ec = _getReader(li)->parseFile(li.takeBuffer(), result)))
+ std::unique_ptr<MemoryBuffer> mbc(buff.take());
+ if ((ec = _targetInfo.parseFile(mbc, result)))
return ec;
}
----------------
Same here for logInputFiles.
================
Comment at: test/darwin/hello-world.objtxt:1-2
@@ -1,3 +1,3 @@
-# RUN: lld-core -writer=mach-o -stubs-pass %s -o %t && llvm-nm %t | FileCheck %s
-
+# RUN: lld -flavor darwin -arch x86_64 -macosx_version_min 10.8 %s -o %t && \
+# RUN: llvm-nm %t | FileCheck %s
#
----------------
removed -stubs-pass ?
================
Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:105-109
@@ +104,7 @@
+ }
+#if 0
+ // TODO: distinguish linkscript from yaml input files
+ if (!_linkerScriptReader)
+ _linkerScriptReader.reset(new ReaderLinkerScript(*this));
+#endif
+ return error_code::success();
----------------
How is LinkerScript parsed now, doesnt this break existing functionality ?
================
Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:100-104
@@ +99,7 @@
+ error_code ec = _elfReader->parseFile(mb,result);
+ if (ec) {
+ if (!_yamlReader)
+ _yamlReader = createReaderYAML(*this);
+ return _yamlReader->parseFile(mb,result);
+ }
+#if 0
----------------
why is yaml reader being called here, if a given ELF file doesnot parse, isnt it an error ?
================
Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:110
@@ -87,1 +109,3 @@
+#endif
+ return error_code::success();
}
----------------
it should return failure if ec if error code is set.
http://llvm-reviews.chandlerc.com/D589
BRANCH
svn
ARCANIST PROJECT
lld
More information about the llvm-commits
mailing list