Page MenuHomePhabricator

[ELF, LinkerScript] Support ! operator in linker script.
ClosedPublic

Authored by abidh on Aug 8 2017, 12:50 AM.

Diff Detail

Event Timeline

abidh created this revision.Aug 8 2017, 12:50 AM
grimar added a subscriber: grimar.Aug 8 2017, 3:21 AM
grimar added inline comments.
test/ELF/linkerscript/assert.s
46 ↗(On Diff #110150)

You can reuse FAIL I think.

# RUN: FileCheck %s -check-prefix=FAIL < %t.log
53 ↗(On Diff #110150)

Excessive empty line.

test/ELF/linkerscript/symbol-assignexpr.s
18

All your new testcases using parentheses, is there some problem without them ?

meadori added a subscriber: meadori.Aug 8 2017, 7:51 AM

Outside the comments @grimar made, LGTM.

abidh updated this revision to Diff 110337.Aug 9 2017, 2:29 AM

Handle review comments.

abidh marked an inline comment as done.Aug 9 2017, 2:32 AM
abidh added inline comments.
test/ELF/linkerscript/assert.s
46 ↗(On Diff #110150)

I tried but it was not immediately obvious how to do it.

test/ELF/linkerscript/symbol-assignexpr.s
18

The linker script parsing have problem where it can not separate these operator. You will see that test of ~ operator also use '(' or space in between. I have also changed one test to use space but this needs to be fixed as a separate problem.

grimar added inline comments.
test/ELF/linkerscript/assert.s
46 ↗(On Diff #110150)

Following just works, no ?

# RUN: echo "SECTIONS { ASSERT(!(1), fail) }" > %t9.script
# RUN: not ld.lld -shared -o %t9 --script %t9.script %t1.o > %t.log 2>&1
# RUN: FileCheck %s -check-prefix=FAIL < %t.log
test/ELF/linkerscript/symbol-assignexpr.s
18

I fixed '~' here: D36508, I think you should be able to do the same for '!`

ruiu added inline comments.Aug 9 2017, 5:08 AM
test/ELF/linkerscript/assert.s
1 ↗(On Diff #110337)

I wouldn't add any tests to this file as the feature is tested by symbol-assignexpr.s.

48–52 ↗(On Diff #110337)

I wouldn't add this test -- the feature is tested enough with the above two tests.

test/ELF/linkerscript/symbol-assignexpr.s
18

Replace !symbol with !1 and add symbol14 = !0.

abidh updated this revision to Diff 110390.Aug 9 2017, 7:54 AM
abidh marked an inline comment as done.
ruiu added inline comments.Aug 9 2017, 11:47 AM
ELF/ScriptLexer.cpp
218–220

I don't think this is correct. For example, I don't think this can handle expression 0!=1. You want to add code to tokenizeExpr instead of this function.

abidh updated this revision to Diff 110532.Aug 10 2017, 1:59 AM

Fix case pointed out in review comments.

abidh added inline comments.Aug 10 2017, 2:02 AM
ELF/ScriptLexer.cpp
218–220

I think 0!=1 was failing even without my patch. I have fixed it now and added a test to check it.

ruiu accepted this revision.Aug 10 2017, 6:02 AM

LGTM

ELF/ScriptLexer.cpp
193–197

This is probably a bit more straightforward:

if (S.substr(E).startswith("!=")) {
  Ret.push_back(S.substr(E, 2));
  S = S.substr(E + 2);
} else {
  Ret.push_back(S.substr(E, 1));
  S = S.substr(E + 1);
}
test/ELF/linkerscript/symbol-assignexpr.s
20

Thank you for adding this test.

This revision is now accepted and ready to land.Aug 10 2017, 6:02 AM
abidh closed this revision.Aug 10 2017, 8:26 AM