This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Linkerscript: implemented BYTE/SHORT/LONG/QUAD commands.
ClosedPublic

Authored by grimar on Sep 22 2016, 8:36 AM.

Details

Summary

Previously our scripts handles these commands incorrectly. For example:

SECTIONS  {
  .foo : {
 *(.foo.1)
 BYTE(0x11)
...

We accepted the script above treating BYTE as input section description. These commands are used in the wild though, eg:
https://searchcode.com/codesearch/view/42878166/

The BYTE, SHORT, LONG, and QUAD commands store one, two, four, and eight bytes (respectively). After storing the bytes, the location counter is incremented by the number of bytes
stored.

Patch implements them.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 72178.Sep 22 2016, 8:36 AM
grimar retitled this revision from to [ELF] - Linkerscript: implemented BYTE/SHORT/QUAD commands..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, davide, evgeny777.
ruiu added inline comments.Sep 22 2016, 1:02 PM
ELF/LinkerScript.cpp
407 ↗(On Diff #72178)

This

417 ↗(On Diff #72178)

this and

424 ↗(On Diff #72178)

this needs comments.

657 ↗(On Diff #72178)

Don't we have a function to find an output section by name?

660–663 ↗(On Diff #72178)

This code seems to write the bytes in the host endianess instead of the target endianness.

1431–1436 ↗(On Diff #72178)

I don't think we need these enums. Can't this be

int Size = StringSwitch(Tok)
  .Case("BYTE", 1)
  .Case("SHORT", 2)
  ...

?

ELF/LinkerScript.h
47–51 ↗(On Diff #72178)

It's time to add comments for each command.

AssignmentKind, // . = expr or <sym> = expr
OutputSectionKind,
InputSectionKind,
AssertKind, // ASSERT(expr)
BytesDataKind, // BYTE(expr), SHORT(expr), LONG(expr) or QUAD(expr)
ELF/OutputSections.cpp
1008 ↗(On Diff #72178)

It needs a comment.

// Linker scripts may have BYTE()-family commands with which you
// can write arbitrary bytes to the output. Process them if any.
grimar updated this revision to Diff 72248.Sep 23 2016, 3:38 AM
grimar retitled this revision from [ELF] - Linkerscript: implemented BYTE/SHORT/QUAD commands. to [ELF] - Linkerscript: implemented BYTE/SHORT/LONG/QUAD commands..
grimar added inline comments.
ELF/LinkerScript.cpp
407 ↗(On Diff #72178)

Done.

417 ↗(On Diff #72178)

Done.

424 ↗(On Diff #72178)

Done.

657 ↗(On Diff #72178)

I do not think we have, I created new one, it can be reused in getFiller() in following patch.

660–663 ↗(On Diff #72178)

Ah, right. Fixed.

1431–1436 ↗(On Diff #72178)

Done.

ELF/LinkerScript.h
47–51 ↗(On Diff #72178)

Done. (btw comments looks a bit displaced because are clang-formatted)

ELF/OutputSections.cpp
1008 ↗(On Diff #72178)

Done.

rafael added inline comments.Sep 23 2016, 4:52 AM
ELF/LinkerScript.h
146 ↗(On Diff #72248)

This could just store a StringRef or ArrayRef, no?

We can compute the correct (target endian) representation upfront and simplify the remaining code.

grimar added inline comments.Sep 23 2016, 7:02 AM
ELF/LinkerScript.h
146 ↗(On Diff #72248)

Not sure about StringRef/ArrayRef, that could be std::vector<uint_8t>, computed in theory, but I think I can't do it.

Problem is that readBytesDataCommand() that reads command know nothing about target endianess because is not ELFT templated.

And I mean it is too early to know it as script can go before any other files, so I need to keep the value anyways, and in that case have a vector or something else in addition does not make much sence ?

rafael added inline comments.Sep 23 2016, 7:16 AM
ELF/LinkerScript.cpp
713 ↗(On Diff #72248)

You can probably use getSectionIndex.

grimar updated this revision to Diff 72277.Sep 23 2016, 7:39 AM
  • Addressed review comments.
ELF/LinkerScript.cpp
713 ↗(On Diff #72277)

Done.

rafael accepted this revision.Sep 23 2016, 10:35 AM
rafael edited edge metadata.

LGTM with nits, but wait so see if Rui is OK with it too.

test/ELF/linkerscript/data-commands.s
2 ↗(On Diff #72277)

Add a big endian test.

21 ↗(On Diff #72277)

Don't check the address.

This revision is now accepted and ready to land.Sep 23 2016, 10:35 AM
ruiu added inline comments.Sep 23 2016, 10:47 AM
ELF/LinkerScript.cpp
424 ↗(On Diff #72277)

Add a blank line before this.

Maybe just

// Handle BYTE(), SHORT(), LONG(), or QUAD().
717 ↗(On Diff #72277)

This looks odd. You can return early.

if (I == INT_MAX)
  return;
726 ↗(On Diff #72277)

I'd factor this switch out as an independent functions so that we can do something like this here.

writeInt<ELFT>(Buf + DataCmd->Offset, DataCmd->Data, DataCmd->Size);

where writeInt is defined as

writeInt(uint8_t *Buf, uint64_t Data, uint64_t Size);
1519 ↗(On Diff #72277)

Use int instead of unsigned so to remove the cast to unsigned below.

grimar updated this revision to Diff 72454.Sep 26 2016, 3:34 AM
grimar edited edge metadata.
grimar marked 2 inline comments as done.
  • Addressed review comments.
ELF/LinkerScript.cpp
424 ↗(On Diff #72277)

Done.

717 ↗(On Diff #72277)

Done.

1519 ↗(On Diff #72277)

Done.

test/ELF/linkerscript/data-commands.s
2 ↗(On Diff #72277)

Done.

21 ↗(On Diff #72277)

Done.

ruiu accepted this revision.Sep 26 2016, 10:43 AM
ruiu edited edge metadata.

LGTM with a nit.

ELF/LinkerScript.cpp
742–743 ↗(On Diff #72454)

This comment doesn't make sense to be written here. Probably I'd just remove.

This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Sep 26 2016, 12:45 PM
ELF/LinkerScript.cpp
742–743 ↗(On Diff #72454)

Yeah, initially I wrote it because we have comment for filler saying it works with big-endian,
but for this place there is no disagree between ld, gold and spec, so it was probably excessive, I removed it from commit.