This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Implemented --defsym option
ClosedPublic

Authored by grimar on Apr 18 2017, 9:19 AM.

Details

Summary

gnu ld description of option is:

--defsym=symbol=expression
Create a global symbol in the output file, containing the absolute address given by expression. You may use this option as many times as necessary to define multiple symbols in the command line. A limited form of arithmetic is supported for the expression in this context: you may give a hexadecimal constant or the name of an existing symbol, or use "+" and "-" to add or subtract hexadecimal constants or symbols. If you need more elaborate expressions, consider using the linker command language from a script. Note: there should be no white space between symbol, the equals sign ("="), and expression.

In compare with D32082, this patch does not support math expressions and absolute symbols. It implemented via code similar to --wrap.
That covers 1 of 3 possible --defsym cases.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Apr 18 2017, 9:19 AM
davide requested changes to this revision.Apr 18 2017, 9:21 AM
davide added a subscriber: davide.
davide added inline comments.
ELF/SymbolTable.cpp
174–178 ↗(On Diff #95586)

this bit is untested.

This revision now requires changes to proceed.Apr 18 2017, 9:21 AM
grimar updated this revision to Diff 95591.Apr 18 2017, 9:31 AM
grimar edited edge metadata.
  • Addressed review comment.
ELF/SymbolTable.cpp
174–178 ↗(On Diff #95586)

Fixed.

grimar updated this revision to Diff 95693.Apr 19 2017, 12:43 AM
  • Updated testcase to show we can use aliasing.
test/ELF/defsym.s
16 ↗(On Diff #95591)

Forgot to mention this moment. We have 2 symbols 'foo1' in output here. That is because I don't
change name of alias symbol body in alias().
To change the name in symtab I think I need to make SymbolBody::Name public
and remove getter, that allows to set new name, but also involves changes in many places. Do we want it ?
Aliasing itself works without that I believe.

ruiu added inline comments.Apr 19 2017, 4:16 AM
ELF/Driver.cpp
884 ↗(On Diff #95693)

I'd support just 1.

887 ↗(On Diff #95693)

Make this a function that returns a vector of pair<StringRef, StringRef> and use alias function in link().

grimar updated this revision to Diff 95926.Apr 20 2017, 3:55 AM
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
ELF/Driver.cpp
884 ↗(On Diff #95693)

Done.

887 ↗(On Diff #95693)

Done.

ruiu edited edge metadata.Apr 24 2017, 9:53 PM

Generally looking good, a few nits.

Please remove "#2" from the patch title. It's not relevant for people who will be reading this patch after you submit this, as they don't know and don't care about how many you have tried before.

ELF/Driver.cpp
886 ↗(On Diff #95926)

Expr is not an expr, but a symbol name, so Expr is not a good name. I'd name them From and To.

888 ↗(On Diff #95926)

Remove =.

Since we expect a symbol name, the error should be something like error("-defsym: symbol name expected, but got " + To);

ELF/Options.td
28 ↗(On Diff #95926)

"Define a symbol alias"

ELF/SymbolTable.cpp
176 ↗(On Diff #95926)

= is not part of the option name but a separator.

Also, we are not handling an expression, so don't use the word "expression".

Error message should be error("-defsym: undefined symbol: " + Name);

This revision now requires changes to proceed.Apr 24 2017, 9:53 PM
grimar retitled this revision from [ELF] - Implemented --defsym option #2 to [ELF] - Implemented --defsym option.
grimar updated this revision to Diff 96528.Apr 25 2017, 4:06 AM
  • Addressed review comments.
ELF/Driver.cpp
886 ↗(On Diff #95926)

Done.

888 ↗(On Diff #95926)

Done.

ELF/Options.td
28 ↗(On Diff #95926)

Done.

ELF/SymbolTable.cpp
176 ↗(On Diff #95926)

Done.

ruiu accepted this revision.Apr 25 2017, 10:02 AM

LGTM

This revision was automatically updated to reflect the committed changes.