This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Linkerscript: implemented += operator.
ClosedPublic

Authored by grimar on Jul 28 2016, 7:56 AM.

Details

Summary

Sometimes += is used to move the location counter.
Example from the wild is:

.dbg_excpt _DBG_EXCPT_ADDR (NOLOAD) :
{
  . += (DEFINED (_DEBUGGER) ? 0x8 : 0x0);

https://github.com/chipKIT32/pic32-Arduino-USB-Bootloader-original/blob/master/boot-linkerscript.ld

Patch implements it and opens way for others type of assignments (-= *= etc), though I think only += is
actual to support.

Diff Detail

Event Timeline

grimar updated this revision to Diff 65936.Jul 28 2016, 7:56 AM
grimar retitled this revision from to [ELF] - Linkerscript: implemented += operator..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, davide, evgeny777.
emaste added a subscriber: emaste.Jul 28 2016, 10:40 AM
ruiu added inline comments.Jul 28 2016, 11:33 AM
ELF/LinkerScript.cpp
16

It can be like this. You don't have to call combine() to handle operators that is known at compile time.

StringRef Op = next();
assert(Op == "=" || Op == "+=");
Expr E = readExpr();
if (Op == "+=")
  E = [=](uint64_t Dot) { return getSymbolValue(Name, Dot) + E(Dot); };
auto *Cmd = new SymbolAssignment(Name, E);
...
grimar updated this revision to Diff 65989.Jul 28 2016, 1:53 PM
  • Addressed review comments.
ELF/LinkerScript.cpp
16

Fixed.

ruiu accepted this revision.Jul 28 2016, 1:58 PM
ruiu edited edge metadata.

LGTM with a nit.

ELF/LinkerScript.cpp
922–923

I think you can remove this error check. If Tok is not a valid symbol name, getSymbolValue will find it.

This revision is now accepted and ready to land.Jul 28 2016, 1:58 PM
grimar added inline comments.Jul 28 2016, 2:14 PM
ELF/LinkerScript.cpp
922–923

Looks few tests begins to fail if I remove. Since this line is not added by this patch, I suggest to leave it for now, I'll check how to remove it and prepare a patch tomorrow.

This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Jul 29 2016, 3:54 AM
ELF/LinkerScript.cpp
922–923

After little investigation I am not sure will removing of check be good change or not now. Imagine we have the next script:

SECTIONS {
boom .temp : { *(.temp) } }
}

And imagine we have no .temp sections. While check is here, parser will catch the ".temp" and report mailformed number. Without that check since we never will create boom as there is no .temp sections, error will never shown.
But at the same time:

SECTIONS {
boom temp : { *(.temp) } }
}

Here temp is valid C identifier. So we will never see any error in that case. So that check helps only sometimes here.
But there are cases when this check really helps. Example:

SECTIONS {   . = ;  }

It finds that ";" is not a number and not a symbol and error outs. Without check it writes:
"; expected but got }" because of:

if (peek() == "=" || peek() == "+=") {
  readAssignment(Tok);
  expect(";");

Probably we should check somehow that expression that readExpr() returns is not empty.