This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Define linkerscript symbols early.
AbandonedPublic

Authored by grimar on Sep 25 2017, 7:20 AM.

Details

Reviewers
ruiu
rafael
Summary

Currently symbols assigned or created by linkerscript are not processed early
enough. As a result it is not possible to version them or assign any other flags/properties.

Patch adds declareSymbols() method to script class. It allows to add
symbols early enough as undefined and that allows to version them properly.

Diff Detail

Event Timeline

grimar created this revision.Sep 25 2017, 7:20 AM
grimar updated this revision to Diff 116554.Sep 25 2017, 7:35 AM
grimar edited the summary of this revision. (Show Details)
  • Added testcase from D36579, showing PR34121 is also fixed with this patch.
grimar updated this revision to Diff 116557.Sep 25 2017, 7:48 AM
  • Updated comments.
ruiu edited edge metadata.Sep 25 2017, 11:20 AM

It doesn't seems that "prefetch" is a right word to describe what you are doing in this patch. Prefetch is, well, fetching something early, but what you are doing in this patch is to define them early. Defining symbols is not fetching symbols.

grimar updated this revision to Diff 116653.Sep 26 2017, 2:59 AM
grimar retitled this revision from [ELF] - Prefetch linkerscript symbols. to [ELF] - Define linkerscript symbols early..
grimar edited the summary of this revision. (Show Details)
  • Addressed review comment about naming things.
  • Updated implementation.
ruiu added inline comments.Sep 27 2017, 9:05 PM
ELF/LinkerScript.cpp
121–123

I do not understand this change.

131

Do not copy-n-paste the same comment all around.

grimar updated this revision to Diff 116975.Sep 28 2017, 6:42 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
ELF/LinkerScript.cpp
121–123

I reimplemented some parts. Does new version look better ?

grimar updated this revision to Diff 118607.Oct 11 2017, 6:49 AM
  • Rebased/reimplemented after recent changes, added comments.
ruiu added inline comments.Oct 11 2017, 11:24 AM
ELF/Driver.cpp
1071

Remove "enough"

ELF/LinkerScript.cpp
122–124

You shouldn't use Ctx in this function. Basically, you shouldn't use Ctx as long as you can avoid it. This is important because adding a use of Ctx = more dependencies you will have.

grimar updated this revision to Diff 118757.Oct 12 2017, 2:11 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
ELF/LinkerScript.cpp
122–124

Ok, I introduced DefineOnly argument instead.

ruiu added inline comments.Oct 12 2017, 11:07 PM
ELF/LinkerScript.cpp
125

I'm still not impressed by this change because it looks like this patch is making this function unnecessarily complicated.

If you add a boolean flag to an existing function and execute some pieces of code of the function only when the flag is true, you are effectively defining two separate functions sharing some amount of code. As a reader of this code, I need to think if the pieces of code that are not guarded by flag work correctly with and without that flag. It is hard to do.

grimar updated this revision to Diff 118902.Oct 13 2017, 6:52 AM
  • Addressed review comments.
smeenai added inline comments.Oct 13 2017, 12:52 PM
ELF/Driver.cpp
1073

Typo: proccessing -> processing

ruiu added inline comments.Oct 15 2017, 1:47 PM
ELF/LinkerScript.cpp
160–164

I don't think you need this.

grimar updated this revision to Diff 119137.Oct 16 2017, 5:10 AM
  • Addressed review comments.
ELF/Driver.cpp
1073

Fixed, thanks !

ELF/LinkerScript.cpp
160–164

Yes, you right. I tried to make logic handling PROVIDE nicer, so using early return
seemed both common and helpfull.
I found better approach for this diff it seems - to just drop the Provide flag after proccessing.

grimar planned changes to this revision.Oct 26 2017, 8:27 AM

I'll try to update this with better version.

grimar updated this revision to Diff 120832.Oct 30 2017, 8:56 AM
  • Reimplemented in much simpler and (hopefully) better way.
ruiu added inline comments.Oct 30 2017, 6:40 PM
ELF/LinkerScript.cpp
121–124

This seems like an optimization that you can remove.

ELF/Writer.cpp
777–782

It is not clear to me why you need to change this.

grimar added inline comments.Oct 31 2017, 2:31 AM
ELF/LinkerScript.cpp
121–124

I think I can't remove it. It is used for following case. If we have script like:
"PROVIDE_HIDDEN(newsym = __ehdr_start + 5);" (linkerscript/symbol-reserved.s)

When addSymbol is called first time to define newsym early, newsym is undefined, so
if (Cmd->Provide && (!B || B->isDefined())) check passes through and we define
dummy newsym.

Next time when addSymbol called to set proper symbol value, if condition will not pass
because symbol is defined. To let it pass I had to drop Provide to disable check.

ELF/Writer.cpp
777–782

Because otherwise I would have "duplicate symbol: _gp" error with script like
"SECTIONS { .text : { *(.text) } _gp = ABSOLUTE(.) + 0x100; .got : { *(.got) } }"
I add _gp early, so addAbsolute would trigger the error without this check.

ruiu added inline comments.Oct 31 2017, 8:15 PM
ELF/LinkerScript.cpp
121–124

That's too complicated. It seems you are making this function do too many different things, so you need to dispatch inside the function. Can you redesign?

ELF/Writer.cpp
777–782

But if that's the case, that is applicable to all symbols. What if you linker scripts defines _gp_disp, for example?

grimar updated this revision to Diff 121483.Nov 3 2017, 8:17 AM
  • Addressed review comments.
ELF/LinkerScript.cpp
121–124

Done. What do you think about new version ?

ELF/Writer.cpp
777–782

Yeah, but them do not break any tests, so change for them can be prepared+committed separatelly.
(I did similar change in D36579 and it even has testcase, going to reuse it, but don't want to do that in this patch).

ruiu added inline comments.Nov 14 2017, 1:08 AM
ELF/LinkerScript.cpp
149

Please always write a function comment and choose an appropriate function name if a function is not trivial. Usually if there are more than one way to "handle" something, you should avoid naming a function "handle", as it is ambiguous. I expected that this function would handle assignments (because the function name says so), but at the beginning it returns if it is an assignment to ".", which is obviously odd.

178

Please avoid using some rarely used magical RAII class.

grimar added inline comments.Nov 14 2017, 1:11 AM
ELF/LinkerScript.cpp
178

Ok. Is this approach to swap/restore the expression looks fine for you in general ?
I feel it is a bit tricky though helps to simplify things.

ruiu added a comment.Nov 14 2017, 1:14 AM

Sorry, I didn't even carefully read the code to understand what it is doing, as I don't really see the code of the RAII class. Please rewrite and upload again.

In D38239#924325, @ruiu wrote:

Sorry, I didn't even carefully read the code to understand what it is doing, as I don't really see the code of the RAII class. Please rewrite and upload again.

OK :)

grimar abandoned this revision.Nov 15 2017, 4:55 AM

Its now intersects with how we handle -defsym. Abandoning it for now, will try to revisit later.

grimar updated this revision to Diff 127319.Dec 18 2017, 3:16 AM
grimar marked an inline comment as done.
  • Rebased.
  • Addressed review comments.
smeenai added inline comments.Dec 18 2017, 9:17 AM
ELF/LinkerScript.cpp
177

Why are defsym's handled differently than linker script assignments?

grimar added inline comments.Dec 18 2017, 9:56 AM
ELF/LinkerScript.cpp
177

Interesting. Initially I did that because investigated defsym.ll failture (error : duplicate symbol: bar2)
with this patch. I found different behavior of gold (and even observed its crash) and bfd, but now I see a error in my testcase.
I retested and both of them have equal behavior and assign versions to defsym symbols.
So I am going to update this patch tomorrow (will have to reinvestigate defsym.ll failture though)

Interesting point that when following code and script are used:

.global foo
foo:
VER1 { global: foo ; local: * ; } ;

Then gold allows to redefine foo:
ld.gold --defsym=foo=2 --version-script version.txt -shared test.o -o test.so

but bfd does not:

++ ld.bfd --defsym=foo=2 --version-script version.txt -shared test.o -o test.so
test.o: In function `foo':
(.text+0x0): multiple definition of `foo'

LLD 6.0.0 (trunk 319747) also links it fine without errors.
It looks like an issue of both gold and LLD for me.

grimar added a comment.EditedDec 19 2017, 4:16 AM

I investigated it and problem is deeper than I expected.

With this patch (without check for Cmd->IsDefsym),
defsym.ll testcase fails with

ld.lld.EXE : error : duplicate symbol: bar2
>>> defined in <internal>
>>> defined in lto.tmp

It happens because of -defsym:
ld.lld %t.o %t1.o -shared -o %t.so -defsym=bar2=bar3

In this patch I define script symbols early and so define bar2, and
after LTO->compile()
it fails with duplicate error in SymbolTable::addRegular(), because
LTO returns defined symbol bar2 again.

I tried playing with Prevailing flag and undefining them before LTO,
but no luck for now.
May be somebody knows how this can/should be solved ?

So we could probably use replaceSymbol<Defined>, like we do in linkerscript to avoid the error,
for script symbols coming from LTO, but does not feel right probably.

Will try to do something else probably with it tomorrow anyways.

grimar planned changes to this revision.Dec 20 2017, 4:21 AM
grimar updated this revision to Diff 128019.Dec 22 2017, 5:39 AM
grimar edited the summary of this revision. (Show Details)
  • Reimplemented.

I think correct solution is to add linker script symbols as undefined and
set flag for them, showing we know they be defined/adjusted later by script.
That looks cleaner and allows to avod multiple definition error I mentioned earlier.

Patch do this.

grimar updated this revision to Diff 128074.Dec 23 2017, 2:05 AM
  • Move declareSymbols() below fetchIfLazy calls.
grimar abandoned this revision.Jan 12 2018, 5:02 AM

Abandoning because alternative version was posted (D41987).