[PATCH] Refactoring of Driver and TargetInfo
kledzik at apple.com
kledzik at apple.com
Fri Mar 29 16:36:10 PDT 2013
================
Comment at: include/lld/Core/TargetInfo.h:69
@@ +68,3 @@
+ bool globalsAreDeadStripRoots() const {
+ return _globalsAreDeadStripRoots;
+ }
----------------
Shankar Kalpathi Easwaran wrote:
> should this be deadStrip() && _globalsAreDeadStripRoots, to prevent an accidental use of globalsAreDeadStripRoots without checking for deadStrip ?
Better yet:
bool globalsAreDeadStripRoots() const {
assert(_deadStrip && "only applicable when deadstripping enabled");
return _globalsAreDeadStripRoots;
}
================
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
----------------
Shankar Kalpathi Easwaran wrote:
> couldnt follow this.
In the atom model, in addition to having undefined symbols (UndefinedAtom), you can have symbols known to be in a particular shared library. For instance, you could have a SharedLibraryAtom that says that printf is in "libc.so". But once you have that, you run into the issue of what to do if one object file says foo comes from libX.so and other says foo comes from libY.so. This is whether to warn in that case.
================
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;
+ }
+
----------------
Shankar Kalpathi Easwaran wrote:
> should this be std::vector<std::string> ?
Well, the strings come in from the command line as raw char* and are used by llvm::cl::ParseCommandLineOptions() as raw char*, so I don't see the need to wrap an object around them.
================
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"));
+ }
----------------
Shankar Kalpathi Easwaran wrote:
> hexagon,x86 is missing here.
I've deleted that whole method from CoreTargetInfo. It is not used
================
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.
+}
+
----------------
Shankar Kalpathi Easwaran wrote:
> ELF has renamed this to RelocationHandler
That may make sense for ELF where the kind field of Reference objects usually holds ELF relocation values. For mach-o, the on-disk relocation type field is only 4-bits, so the kind is encoded in crazy ways. The lld mach-o reader will translate the crazy relocations to sane internal kinds.
================
Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:32
@@ +31,3 @@
+ , _mergeCommonStrings(false)
+ , _runLayoutPass(false) {
+}
----------------
Shankar Kalpathi Easwaran wrote:
> run LayoutPass is true by default for ELF
fixed
================
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;
+ }
+
----------------
Shankar Kalpathi Easwaran wrote:
> 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.
That could be a new option. But we currently we need only this to support an existing darwin option.
================
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
----------------
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?
================
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;
+ }
----------------
Shankar Kalpathi Easwaran wrote:
> 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.
> ------------------------------------------------------------------------
>
fixed
================
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;
+ }
----------------
Shankar Kalpathi Easwaran wrote:
> ------------------------------------------------------------------------
> r177561 | mspencer | 2013-03-20 14:25:34 -0500 (Wed, 20 Mar 2013) | 1 line
>
> [ELF][Reader] Remove static ordinal.
> ------------------------------------------------------------------------
>
> Recently removed.
fixed
================
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
----------------
Shankar Kalpathi Easwaran wrote:
> why is yaml reader being called here, if a given ELF file doesnot parse, isnt it an error ?
We want to be able to pass yaml encoded files to the linker too
================
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();
----------------
Shankar Kalpathi Easwaran wrote:
> How is LinkerScript parsed now, doesnt this break existing functionality ?
The test suite does not have any coverage for linker scripts. But after talking to Michael, I fixed this to check the file extension. If .objtxt it assumes it is yaml, otherwise it assume it is a linker script.
================
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;
----------------
Shankar Kalpathi Easwaran wrote:
> You would need to add logInputFiles here to trace the archive file that was pulled in.
I added the logging.
================
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;
}
----------------
Shankar Kalpathi Easwaran wrote:
> Same here for logInputFiles.
done
================
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
#
----------------
Shankar Kalpathi Easwaran wrote:
> removed -stubs-pass ?
It is now running the darwin style linker, so the stub pass is implicit
http://llvm-reviews.chandlerc.com/D589
BRANCH
svn
ARCANIST PROJECT
lld
More information about the llvm-commits
mailing list