This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Linkerscript: improve handling of ASSERT and symbol assignment outside SECTIONS block
ClosedPublic

Authored by evgeny777 on Sep 12 2016, 4:47 AM.

Details

Summary

This patch does the following

  • ASSERT can be defined outside SECTIONS block
  • In case there is no SECTIONS block, section names can still be used in ASSERT and symbol assignment expressions
  • All command objects are stored and processed in a single place (createAssignments was removed)

Diff Detail

Event Timeline

evgeny777 updated this revision to Diff 70992.Sep 12 2016, 4:47 AM
evgeny777 retitled this revision from to [ELF] Linkerscript: improve ASSERT and symbol assignment outside SECTIONS block.
evgeny777 updated this object.
evgeny777 added reviewers: ruiu, rafael, atanasyan.
evgeny777 set the repository for this revision to rL LLVM.
evgeny777 added a project: lld.
evgeny777 retitled this revision from [ELF] Linkerscript: improve ASSERT and symbol assignment outside SECTIONS block to [ELF] Linkerscript: improve handling of ASSERT and symbol assignment outside SECTIONS block.
evgeny777 added subscribers: grimar, ikudrin, llvm-commits.
atanasyan edited edge metadata.Sep 12 2016, 5:16 AM

Is it possible to do s/HasContents/HasSections/ renaming as a separate commit to reduce diffs? It looks like this change is separable from this patch.

ELF/Writer.cpp
258

Is it important that we call processCommands in the beginning of the LinkerScript::createSections but after the Writer::createSections? If it does not matter, for my taste it is better to remove the processCommands call from the LinkerScript::createSections and do it here to accentuate that we always process commands from linker scripts.

Script<ELFT>::X->processCommands(Factory);
if (ScriptConfig->HasSections)
  Script<ELFT>::X->createSections(Factory);
else
  createSections();

I have feeling that by now HasContents has no meaning at all. Even without this patch there are occasions when HasContents is false, but linker script is not empty. Like when you have only symbol assignments.

ELF/Writer.cpp
258

Yes. You can now use section size and alignment in expressions, even though you may have no SECTIONS block at all.

ruiu edited edge metadata.Sep 12 2016, 1:09 PM

As Simon said, I'd appreciate if you separate the renaming patch from this one.

ruiu added inline comments.Sep 12 2016, 1:26 PM
ELF/LinkerScript.cpp
52–53

This needs comment.

264–265

What is this for? Please write a comment.

771–772

Sort them alphabetically.

evgeny777 updated this revision to Diff 71117.Sep 13 2016, 1:53 AM
evgeny777 edited edge metadata.
evgeny777 removed rL LLVM as the repository for this revision.

Addressed review comments

rafael edited edge metadata.Sep 13 2016, 11:13 AM

A patch renaming HasContent to HasSections LGTM. Please commit that and rebase.

evgeny777 updated this revision to Diff 71308.Sep 14 2016, 2:03 AM
evgeny777 edited edge metadata.

Rebased

atanasyan added inline comments.Sep 14 2016, 2:23 AM
ELF/Writer.cpp
258

Is it important that we call processCommands in the beginning of the LinkerScript::createSections but after the Writer::createSections?

Yes. You can now use section size and alignment in expressions, even though you may have no SECTIONS block at all.

OK. Is it possible to rewrite the code like that? The idea is to make clear that we always call the processCommands.

if (ScriptConfig->HasSections)
  Script<ELFT>::X->createSections(Factory);
else
  createSections();
Script<ELFT>::X->processCommands(Factory);
evgeny777 added inline comments.Sep 14 2016, 10:52 AM
ELF/Writer.cpp
258

No, createSections calls processCommands(). It is not possible to create sections in LinkerScript without walking over command list and at the same time processCommands() should be called after Writer<ELFT>::createSections(), because in this case it's possible to use SIZEOF, ALIGNOF in symbol assignments.

atanasyan accepted this revision.Sep 14 2016, 10:28 PM
atanasyan edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 14 2016, 10:28 PM
This revision was automatically updated to reflect the committed changes.