This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Support symbol assignment inside SECTIONS {} block in linker scripts
ClosedPublic

Authored by evgeny777 on Jul 7 2016, 5:45 AM.

Details

Reviewers
ruiu
ikudrin
Summary

This patch implements minimal support for symbol assignment. Only assignment within SECTIONS block is supported

Diff Detail

Event Timeline

evgeny777 updated this revision to Diff 63059.Jul 7 2016, 5:45 AM
evgeny777 retitled this revision from to [ELF] Support symbol assignment inside SECTIONS {} block in linker scripts.
evgeny777 updated this object.
evgeny777 added reviewers: ruiu, ikudrin.
evgeny777 set the repository for this revision to rL LLVM.
evgeny777 added a project: lld.
ruiu added inline comments.Jul 7 2016, 12:59 PM
ELF/LinkerScript.cpp
235

Why don't you directly call SymbolTable::addAbsolute here?

513–516

Hmm, so you are changing the grammar to LL(2). Do you really need to do that? I think you could do something like this to keep it LL(1).

if (Tok == ".") {
  readLocationCounterValue();
  continue;
}
next();
if (peek() == "=")
  readSymbolAssignment(Tok);
else
  readOutputSectionDescription(Tok);
ELF/LinkerScript.h
13–14

Instead of including these headers, please declare the classes you need in this header.

namespace lld {
namespace elf {
template <class ELFT> class SymbolTable;
...
}
}
84

The symbol table is globally visible as Symtab<ELFT>::X, so you don't need to pass it around.

evgeny777 added inline comments.Jul 7 2016, 1:20 PM
ELF/LinkerScript.cpp
235

I doubt this is possible.
Adding symbol will increase size of .symtab and (possibly) .dyntab which in its turn can affect virtual addresses of some sections.
Also symbols are copied from Symtab object to output sections in createSections() method, which is called before assignAddresses (and even before createPhdrs) - see Writer::run()

513–516

Makes sense. Although, strictly speaking, this doesn't make grammar LL(1) - you still have two lookahead tokens (one is saved in Tok)

evgeny777 updated this revision to Diff 63133.Jul 7 2016, 1:52 PM

Review updated. Symbols.h header file is still required in LinkerScript.h for DefinedRegular<ELFT>

ruiu added inline comments.Jul 7 2016, 2:45 PM
ELF/LinkerScript.cpp
235

Can you replace this with

Symtab<ELFT>::X->find(Cmd.Name)->Value = evalExpr(Cmd.Expr, Dot);

so that we can remove SymIndex and Symbols from this class? This code is slightly slower because it looks up the symbol table, but it should be negligible because the number of symbols appear in symbol scripts is small.

513–516

Well, I think technically it is LL(1) because it recognizes the language with one token look ahead. For semantic actions, we save a token, but it is different from recognizing a language, I think.

578

I'd make this function return a std::vector<StringRef> and use that as

Opt.Command = readSectionsCommandExpr();

It is slightly inefficient, but I believe it's easier to read.

ELF/LinkerScript.h
21

I think you can add

template <class ELFT> class DefinedRegular;

to remove #include "Symbols.h".

48

It is very confusing to reuse SectionName field for symbol name because symbol name is not a section name. Maybe we should rename this Name.

evgeny777 updated this revision to Diff 63188.Jul 8 2016, 1:55 AM

Review updated

ruiu added inline comments.Jul 8 2016, 2:33 PM
ELF/LinkerScript.cpp
237

Do you need this assert? If there's a bug, it would crash with a null pointer dereference.

315

You can remove {}

526

This can be

Opt.Commands.push_back({ExprKind, readSectionsCommandExpr(), ""});
574

This can be

std::vector<StringRef> Exprs = readSectionsCommandExpr();
if (Exprs.empty())
  error(...);
Opt.Command.push_back({SymbolAssignmentKind, Exprs, Name});
ELF/Writer.cpp
788

Add a blank line before this comment.

test/ELF/linkerscript-symbols.s
12

Remove this blank line at end of file.

evgeny777 added inline comments.Jul 11 2016, 2:32 AM
ELF/LinkerScript.cpp
526

This will omit error message and break unit test. Also, I suggest to use move semantics here for performance reasons

574

I would use move semantics for initializer list for performance reasons.

Opt.Command.push_back({SymbolAssignmentKind, std::move(Exprs), Name})
evgeny777 updated this revision to Diff 63479.Jul 11 2016, 2:34 AM

Review updated

ruiu accepted this revision.Jul 11 2016, 2:28 PM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 11 2016, 2:28 PM
evgeny777 closed this revision.Jul 12 2016, 12:32 AM