This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Keep st_type for symbol assignment
ClosedPublic

Authored by MaskRay on Aug 19 2020, 10:05 PM.

Details

Summary

PR46970: for alias = aliasee, the alias can be used in relocation processing
and on ARM st_type does affect Thumb interworking. It is thus desirable for the
alias to get the same st_type.

Note that the st_size field should not be retained because some tools use
st_size=0 as a heuristic to detect aliases. Retaining st_size can thwart such
heuristics and cause aliases to be preferred over the original symbols.

Diff Detail

Event Timeline

MaskRay created this revision.Aug 19 2020, 10:05 PM
MaskRay requested review of this revision.Aug 19 2020, 10:05 PM
MaskRay updated this revision to Diff 286712.EditedAug 19 2020, 10:54 PM

Rebase after ac46bc35e98d922f1b05b451341f03dcaccd1527
Add an ARM test. Set st_type in LinkerScript::addSymbol (which runs before relocation processing) to make this affect relocation processing

MaskRay updated this revision to Diff 286717.Aug 19 2020, 11:23 PM

Add a doc

Not had a chance to look into the changes in detail yet, but on first glance it looks good. Given that the compiler uses STT_FUNC for the entry of a symbol I think it is reasonable to retain the type only for an alias.

lld/docs/ELF/linker_script.rst
37

May be worth using "The `st_size` field is set to 0."

grimar accepted this revision.Aug 20 2020, 3:00 AM

This LGTM with minor nit. Worth waiting for a final word from Peter too.

lld/ELF/LinkerScript.h
61

Deserves a comment I think. It is probably not that obvious that type is a symbol type.

Also, use STT_NOTYPE instead of 0?

This revision is now accepted and ready to land.Aug 20 2020, 3:00 AM

Had a chance to look over lunch. Thanks looks good to me too.

Thank you!

lld/test/ELF/linkerscript/symbol-assign-type.s
3

Typo: aliase -> alias

MaskRay updated this revision to Diff 286878.Aug 20 2020, 12:42 PM
MaskRay marked 2 inline comments as done.

Fix typo

Improve doc

Test _start in symbol-assign-type.s

MaskRay updated this revision to Diff 286879.Aug 20 2020, 12:43 PM

Add a comment

MaskRay marked an inline comment as done.Aug 20 2020, 12:44 PM
MaskRay updated this revision to Diff 286912.Aug 20 2020, 4:02 PM

Update comment

This revision was landed with ongoing or failed builds.Aug 20 2020, 4:05 PM
This revision was automatically updated to reflect the committed changes.
smeenai added a subscriber: hans.Aug 21 2020, 11:00 AM

@hans, would you consider this for 11.0? 11.0 LLD will no longer perform ARM-Thumb interworking for symbols that aren't marked STT_FUNC, so this fixes cases where interworking would incorrectly not occur because of a simple alias.

@hans, would you consider this for 11.0? 11.0 LLD will no longer perform ARM-Thumb interworking for symbols that aren't marked STT_FUNC, so this fixes cases where interworking would incorrectly not occur because of a simple alias.

This is not a case we intended to support before, so this is more of a feature request. The risk is tiny and can make some users happy. I am in favor of patching this into 11.0.

hans added a comment.Aug 24 2020, 11:14 AM

The risk is tiny and can make some users happy. I am in favor of patching this into 11.0.

Sounds like a good trade-off :-)

I tried applying it, but ELF/arm-thumb-interwork-ifunc.s doesn't pass with it on the branch. I guess the expectations need rebasing. Would you be willing to do that and push it to the branch?

The risk is tiny and can make some users happy. I am in favor of patching this into 11.0.

Sounds like a good trade-off :-)

I tried applying it, but ELF/arm-thumb-interwork-ifunc.s doesn't pass with it on the branch. I guess the expectations need rebasing. Would you be willing to do that and push it to the branch?

Done in d6d03d09e3f7498f60e2976b8cea235080f55fe7 (arm-thumb-interwork-ifunc.s needs a clean-up/fix before this patch) & c4e216711d001316df2313a85c8af73a85d804c0 (rewritten lld/test/ELF/linkerscript/symbol-assign-type.s to not depend on 'split-file' (which is not in release/11.x)).

hans added a comment.Aug 25 2020, 5:47 AM

The risk is tiny and can make some users happy. I am in favor of patching this into 11.0.

Sounds like a good trade-off :-)

I tried applying it, but ELF/arm-thumb-interwork-ifunc.s doesn't pass with it on the branch. I guess the expectations need rebasing. Would you be willing to do that and push it to the branch?

Done in d6d03d09e3f7498f60e2976b8cea235080f55fe7 (arm-thumb-interwork-ifunc.s needs a clean-up/fix before this patch) & c4e216711d001316df2313a85c8af73a85d804c0 (rewritten lld/test/ELF/linkerscript/symbol-assign-type.s to not depend on 'split-file' (which is not in release/11.x)).

Thanks!