This is an archive of the discontinued LLVM Phabricator instance.

[lld] Registry model for Readers and Reference Kind strings
ClosedPublic

Authored by kledzik on Dec 17 2013, 5:41 PM.

Details

Summary

Sorry about the big patch. I could not come up with a way to do this in stages.

The main changes are in:

include/lld/Core/Reference.h
include/lld/ReaderWriter/Reader.h

Everything else is details to support the main change.

  1. Registration based Readers

Previously, lld had a tangled interdependency with all the Readers. It would
have been impossible to make a streamlined linker (say for a JIT) which
just supported one file format and one architecture (no yaml, no archives, etc).
The old model also required a LinkingContext to read an object file, which
would have made .o inspection tools awkward.

The new model is that there is a global Registry object. You programmatically
register the Readers you want with the registry object. Whenever you need to
read/parse a file, you ask the registry to do it, and the registry tries each
registered reader.

For ease of use with the existing lld code base, there is one Registry
object inside the LinkingContext object.

  1. Changing kind value to be a tuple

Beside Readers, the registry also keeps track of the mapping for Reference
Kind values to and from strings. Along with that, this patch also fixes
an ambiguity with the previous Reference::Kind values. The problem was that
we wanted to reuse existing relocation type values as Reference::Kind values.
But then how can the YAML write know how to convert a value to a string? The
fix is to change the 32-bit Reference::Kind into a tuple with an 8-bit namespace
(e.g. ELF, COFFF, etc), an 8-bit architecture (e.g. x86_64, PowerPC, etc), and
a 16-bit value. This tuple system allows conversion to and from strings with
no ambiguities.

Diff Detail

Event Timeline

shankarke accepted this revision.Dec 17 2013, 9:39 PM

I like this change, very flexible.

include/lld/Core/LinkingContext.h
280

empty comment ?

281–282

Isnt having two functions returning the same in const/ non-const mode confusing ? We might want to settle on whether we want to use a const vs. non-const ?

include/lld/Core/Reference.h
42–43

What is all, testing ? is this for the core flavor ?

include/lld/ReaderWriter/FileArchive.h
15–20

we should delete this file, as its not providing any functionality.

include/lld/ReaderWriter/Reader.h
87

nitpick : adSupport -> addSupport.

lib/Driver/GnuLdDriver.cpp
339–340

Agree. Perfect place.

lib/ReaderWriter/ELF/Atoms.h
803

Hexeagon->Hexagon

lib/ReaderWriter/ELF/DefaultLayout.h
334

Thanks for fixing this.

joey added inline comments.Dec 18 2013, 2:23 AM
include/lld/Driver/DarwinInputGraph.h
56

Should this change be included?

include/lld/ReaderWriter/ELFLinkingContext.h
235

Randomly inserted whitespace!

include/lld/ReaderWriter/Reader.h
81

I'm wondering if we should use a typedef for Reference::KindValue?

include/lld/ReaderWriter/YamlContext.h
20

Indentation looks really weird here.

kledzik added inline comments.Dec 18 2013, 1:12 PM
include/lld/Core/LinkingContext.h
280

fixed

281–282

This is the standard way to allow non-const LinkingContexts to get a non-const registry and limit const LinkingContexts to only get a const registry.

include/lld/Core/Reference.h
42–43

"all" is used with values like "kindInGroup" which are applicable to all clients.
"testing" is for use with the test cases the use the CoreDriver.

include/lld/Driver/DarwinInputGraph.h
56

When mach-o starts handling archives, it will need this for use with the -all_load option.

include/lld/ReaderWriter/ELFLinkingContext.h
235

fixed

include/lld/ReaderWriter/FileArchive.h
15–20

I thought I had "svn rm"ed it, but apparently not. I've now removed it.

include/lld/ReaderWriter/Reader.h
81

Good Idea. I've added that typedef and changed lots of places to use it.

87

fixed

include/lld/ReaderWriter/YamlContext.h
20

The innermost namespace was not indented properly. Fixed.

lib/ReaderWriter/ELF/Atoms.h
803

Fixed

ruiu added a comment.Dec 18 2013, 11:17 PM

LGTM with a few comments.

There are format errors in this patch, but I don't think pointing every one of them does not make sense as this patch is huge. :) So please run clang-format on this patch before submitting.

include/lld/ReaderWriter/CoreLinkingContext.h
32

Are these fields for testing? If so, is there any way to move this to a unit test file and remove them from this file?

include/lld/ReaderWriter/YamlContext.h
40

Why mach-o file only? If we need it, it looks weird that we don't have files for other formats.

lib/Driver/GnuLdDriver.cpp
339

nit: s/is is/is/

lib/Driver/WinLinkDriver.cpp
654

This code looks pretty clean. Nice!

lib/Passes/LayoutPass.cpp
73

Does it make sense to start a debug output line with ":"?

lib/Passes/RoundTripNativePass.cpp
47

s/yaml/native/

lib/ReaderWriter/ELF/File.h
17

Remove this empty line.

lib/ReaderWriter/ELF/Reader.cpp
119

This is not new code, but it does not seem to be the right way to calculate an alignment from the buffer start address. If the start address happens to be aligned on a large alignment, maxAlignment will become a large number for no reason. We should probably revisit this after submitting this patch.

126

Can we do this as push_back(std::unique_ptr<File>(f->release)), or just push_back(std::move(f))?

lib/ReaderWriter/PECOFF/ReaderImportHeader.cpp
309

I think you can remove this file magic check as it's already been checked.

lib/ReaderWriter/Reader.cpp
33

You can use extension(StringRef) declared in llvm/include/llvm/Support/Path.h.

46

These brackets are not needed.

Bigcheese requested changes to this revision.Dec 19 2013, 7:16 AM

Looks good here other than the remaining comments.

lib/ReaderWriter/ELF/Reader.cpp
119

maxAlignment is the maximum assumable alignment. createELF handles it correctly.

126

Yes, the previous code should still work.

kledzik added inline comments.Dec 19 2013, 11:58 AM
include/lld/ReaderWriter/CoreLinkingContext.h
32

The CoreLinkingContext is used only with "lld -core" which is only used in the test suite.

Currently, these are only used with the pass-got-basic.objtxt and pass-stubs-basic.objtxt tests. When I wrote those passes, the intent was that there was a base generic pass, tested by these tests. And each writer would subclass the generic pass as mach-o does. But, ELF did not do that, instead it has its own private RelocationPass. We need to decide it if is worth have base passes and format specific sub-classes (or delegate objects). If so, we need some sort of generic tests and reference kinds like this. If not, I can remove all this and push the base classes into mach-o.

include/lld/ReaderWriter/YamlContext.h
40

We can add ivars for other formats as needed. This is a step to allowing heterogenous yaml with some files that are the traditional atoms-in-yaml and some that are mach-o-in-yaml.

lib/Driver/GnuLdDriver.cpp
339

fixed

lib/Passes/LayoutPass.cpp
73

fixed

lib/Passes/RoundTripNativePass.cpp
47

Actually, the dyn_cast and assert lines need to be removed. Fixed.

lib/ReaderWriter/ELF/Reader.cpp
126

I reverted to the original. This was cruft left over from a previous experiment.

lib/ReaderWriter/PECOFF/ReaderImportHeader.cpp
309

Ok. I moved the size check into canParse() method and got rid of the check here.

lib/ReaderWriter/Reader.cpp
33

Cool. Switched to use that.

46

removed

kledzik accepted this revision.Dec 19 2013, 2:04 PM

committed in 197727

ruiu added a comment.Dec 19 2013, 11:49 PM

The patch contains many indentation errors and trailing white spaces. I'll
run clang-format on the committed change.

Bigcheese resigned from this revision.Nov 14 2019, 1:11 PM
This revision is now accepted and ready to land.Nov 14 2019, 1:11 PM
MaskRay closed this revision.Nov 14 2019, 1:22 PM