Page MenuHomePhabricator

[ELF] Allow `ASSERT` in output section descriptions
ClosedPublic

Authored by meadori on Nov 21 2016, 2:04 PM.

Details

Reviewers
ruiu
llvm-commits
Group Reviewers
lld
Summary

GNU LD allows ASSERT commands to be in output section descriptions.
Note that LD also mandates that ASSERT commands in this context must
end with a semicolon.

Diff Detail

Event Timeline

meadori updated this revision to Diff 78779.Nov 21 2016, 2:04 PM
meadori retitled this revision from to [ELF] Allow `ASSERT` in output section descriptions.
meadori updated this object.
meadori added a reviewer: lld.
meadori added a reviewer: llvm-commits.
meadori added a subscriber: rui314.
ruiu edited subscribers, added: ruiu; removed: rui314.Nov 21 2016, 2:18 PM
ruiu added inline comments.Nov 21 2016, 3:29 PM
ELF/LinkerScript.cpp
474

Don't you need to pass Dot instead of 0, do you?

1421–1428

Since you added {}, please add braces to all if's here.

1427–1428

Remove this comment as it is obvious.

meadori added inline comments.Nov 22 2016, 8:28 AM
ELF/LinkerScript.cpp
474

Yup, you are right. Will fix.

1421–1428

Will do.

1427–1428

Will do.

meadori updated this revision to Diff 78883.Nov 22 2016, 8:49 AM

Address review feedback.

ruiu accepted this revision.Nov 22 2016, 8:55 AM
ruiu added a reviewer: ruiu.

LGTM. Thanks!

test/ELF/linkerscript/assert.s
33–38

Can you write this in one line? Look at other tests in this file.

This revision is now accepted and ready to land.Nov 22 2016, 8:55 AM
meadori added inline comments.Nov 22 2016, 9:02 AM
test/ELF/linkerscript/assert.s
33–38

Sure. I noticed the recent changes to the tests in this file and changed the test following this one to be on one line. I left this one alone b/c it was a bit longer. Is it OK to exceed 80 columns?

ruiu added inline comments.Nov 22 2016, 9:11 AM
test/ELF/linkerscript/assert.s
33–38

I'd make it one line even if it exceeds 80 columns a bit. Because otherwise an error message for this test would be really hard to read. It becomes something like this as it contains lots of spaces.

error: foo1.o:1: SECTIONS { <lots of spaces> .foo { <lots of spaces> ASSERT(...) <lots of spaces> ...
error: foo1.o:1:                                   <lots of spaces> ^ error here

That's why I changed other tests.

meadori closed this revision.Nov 22 2016, 10:48 AM

Committed. Thanks for the review Rui.