This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Do not fail when set versions for linkerscript's symbol aliases
AbandonedPublic

Authored by grimar on Aug 10 2017, 7:00 AM.

Details

Summary

This is PR34121,

previously we would fail on following:

% cat sym.c
void f() {}

% cat sym.script
g = f;
VERSION { V1 { global: f; g; local: *; }; }

% clang -fPIC -c sym.c
% ld.lld -shared -o libsym.so sym.o sym.script --no-undefined-version
ld.lld: error: version script assignment of 'V1' to symbol 'g' failed: symbol not defined

That happened because at the point of proccessing symbol versions, symbol g was
not yet created. Patch changes logic to create undefined symbols referenced by script
earlier, so we are able to assing version correctly.

Diff Detail

Event Timeline

grimar created this revision.Aug 10 2017, 7:00 AM
ruiu edited edge metadata.Aug 10 2017, 11:09 AM

It doesn't seem correct. Essentially, the fundamental issue is that we add versions to symbols too early, so moving it upwards in the driver doesn't feel right.

In D36579#838308, @ruiu wrote:

It doesn't seem correct. Essentially, the fundamental issue is that we add versions to symbols too early, so moving it upwards in the driver doesn't feel right.

I am not sure I got your point right. My patch does not move code that adds versions upwards.

What it do is moves proccessing of symbols referenced by linkerscript script, so that at the moment of scaning version script, symbols
touched in linkerscript become available for versioning.

So it works like if I would scan over script commands and adds undefined symbol for each symbol assigned by
linkerscript's assignment expression. That allows to version that undefined symbol, which is replaced by
defined regular when later script is proccessed.

@grimar, I believe @ruiu's point was that the real issue is that LLD versions symbols too early, so moving other things upwards in the driver (in this case, the processing of symbols defined in a linker script) to work around the versioning being too early doesn't feel right.

Problem here is that we can not version symbols too late either, as we looks need to do that before
running LTO to allow internalization and versioning to work. In that sence versioning happens probably
at right place now. I'll update diff with a better version soon.

grimar updated this revision to Diff 113082.Aug 29 2017, 6:35 AM
  • Reimplemented in a different way.
ruiu added a comment.Sep 5 2017, 2:43 PM

It doesn't feel right to add a new member to Symbol just for this. Can you add a DefinedRegular symbol with some dummy value instead of an Undefined symbol, no?

grimar added a comment.Sep 6 2017, 4:12 AM
In D36579#861514, @ruiu wrote:

It doesn't feel right to add a new member to Symbol just for this. Can you add a DefinedRegular symbol with some dummy value instead of an Undefined symbol, no?

Yes, you right. In update (going to post in a minute) I used addAbsolute because it has less arguments than addRegular and used weak binding for these symbols to
avoid "duplicate symbol: _gp" error in ELF/mips-gp-ext.s. Linkersctipt's addRegular replaces binding with STB_GLOBAL for these symbols anyways.

grimar updated this revision to Diff 113984.Sep 6 2017, 4:13 AM
  • Addressed comments.
ruiu added inline comments.Sep 6 2017, 10:24 AM
ELF/Driver.cpp
950

Inline this function.

956

Add symbols defined in linker scripts early because otherwise version
scripts referring script-synthesized symbols wouldn't work due to undefined
symbol error. (Version scripts are processed earlier than linker scripts.)

grimar updated this revision to Diff 114141.Sep 7 2017, 3:41 AM
  • Addressed review comments.
ELF/Driver.cpp
950

Done.

956

Added, thanks for comment !

ruiu added inline comments.Sep 7 2017, 11:34 AM
ELF/Driver.cpp
988

Add symbols -> Add dummy symbols

994

What is !Cmd->Provide for?

995

Is there any reason to create symbols as STB_WEAK?

grimar updated this revision to Diff 114322.Sep 8 2017, 2:37 AM
  • Updated comment.
ELF/Driver.cpp
988

Done.

994

symbols.s has next testcase:
(testcase was innaccurate before today, I fixed it in rL312779).

# The symbol is not referenced. Don't provide it.
# RUN: echo "SECTIONS { PROVIDE(newsym = 1);}" > %t.script
# RUN: ld.lld -o %t1 --script %t.script %t
# RUN: llvm-objdump -t %t1 | FileCheck --check-prefix=DONTPROVIDE %s
# DONTPROVIDE-NOT: newsym

If we have no !Cmd->Provide condition here it will fail,
because we should not provide newsym always, but only when it is referenced.

995

Yes, please refer to my comment above:
https://reviews.llvm.org/D36579#861990

ruiu added inline comments.Sep 8 2017, 12:42 PM
ELF/Driver.cpp
995

I don't think adding symbols as weak symbols is a right thing to do, if the problem is just for _gp. It implies that _gp's handling is not right.

grimar updated this revision to Diff 114771.Sep 12 2017, 12:46 AM
grimar added a reviewer: atanasyan.
  • Addressed review comments.
ELF/Driver.cpp
995

I changed how _gp and other special symbols affected are created, added testcase.

atanasyan edited edge metadata.Sep 12 2017, 8:48 AM

MIPS part is LGTM.

It's interesting that _gp_disp and __gnu_local_gp have incorrect binding and visibility for so long time. I will fix that later.

ruiu added inline comments.Sep 12 2017, 10:07 AM
ELF/Writer.cpp
800 ↗(On Diff #114771)

GP -> Gp

(You can see below that MipsGp is named MipsGp and not MipsGP.)

801–804 ↗(On Diff #114771)

I don't get the meaning of this part. Gp doesn't seem to be guaranteed to be a DefinedRegular.

808 ↗(On Diff #114771)

GPD -> GpDisp

819 ↗(On Diff #114771)

LGP -> LocalGp

MIPS part is LGTM.

It's interesting that _gp_disp and __gnu_local_gp have incorrect binding and visibility for so long time. I will fix that later.

Thanks for looking. It only happens for _gp_disp and __gnu_local_gp if them are assigned from script to something, I guess that is just a rare case.
Also I think that other special symbols are also affected. I did not yet check, but I think that for example __ehdr_start, __executable_start, __dso_handle
will remain hidden even if be assigned from script.

grimar updated this revision to Diff 115003.Sep 13 2017, 2:18 AM
  • Addressed review comments.
ELF/Writer.cpp
801–804 ↗(On Diff #114771)

!GP->isInCurrentDSO() condition handled all other possible cases I believe except DefinedCommon, which looks
possible and I missed it.
Fixed.

MIPS part is LGTM.

It's interesting that _gp_disp and __gnu_local_gp have incorrect binding and visibility for so long time. I will fix that later.

Thanks for looking. It only happens for _gp_disp and __gnu_local_gp if them are assigned from script to something, I guess that is just a rare case.
Also I think that other special symbols are also affected. I did not yet check, but I think that for example __ehdr_start, __executable_start, __dso_handle
will remain hidden even if be assigned from script.

It looks like we do not have a strict requirement to put _gp_disp and __gnu_local_gp into the linked file at all. Both these symbols are "magic" symbols. The _gp_disp designates offset between start of a function and '_gp' value. I.e. its value used for relocations calculation differs from function to function. The __gnu_local_gp can be considered as just a synonym for the _gp. GNU ld/gold write both symbols into an output and LLD mimics this behavior, but as far as I know no tool uses these symbols in linked binaries.

Both _gp_disp and __gnu_local_gp are undefined symbols in input object files. So if we do nothing, we get the "undefined symbol" error. Do you know any method to suppress this error for particular symbols? We would recognize these special symbols when calculate relocations, but do not define them and do not write them into the output symbol table.

MIPS part is LGTM.

It's interesting that _gp_disp and __gnu_local_gp have incorrect binding and visibility for so long time. I will fix that later.

Thanks for looking. It only happens for _gp_disp and __gnu_local_gp if them are assigned from script to something, I guess that is just a rare case.
Also I think that other special symbols are also affected. I did not yet check, but I think that for example __ehdr_start, __executable_start, __dso_handle
will remain hidden even if be assigned from script.

It looks like we do not have a strict requirement to put _gp_disp and __gnu_local_gp into the linked file at all. Both these symbols are "magic" symbols. The _gp_disp designates offset between start of a function and '_gp' value. I.e. its value used for relocations calculation differs from function to function. The __gnu_local_gp can be considered as just a synonym for the _gp. GNU ld/gold write both symbols into an output and LLD mimics this behavior, but as far as I know no tool uses these symbols in linked binaries.

Both _gp_disp and __gnu_local_gp are undefined symbols in input object files. So if we do nothing, we get the "undefined symbol" error. Do you know any method to suppress this error for particular symbols? We would recognize these special symbols when calculate relocations, but do not define them and do not write them into the output symbol table.

Probably it should be possible to set IsUsedInRegularObj = false for them and define as weak. That way they should not be in symtab and should not be reported as undefined.
I am not sure that is a good solution though.

grimar abandoned this revision.Oct 10 2017, 1:06 AM

In favor of D38239.