Page MenuHomePhabricator

[LLD][ELF] - Fix the different behavior of the linker script symbols on different platforms.
Needs ReviewPublic

Authored by grimar on Dec 11 2018, 2:16 AM.

Details

Summary

This should help make D55423 be even more simple and also
fixes the broken behavior shown in one of our test cases
for some targets, like x86-64.

The issue occurs when the forward declarations are used in the script.
One of the samples is:

SECTIONS {
  foo = ADDR(.text) - ABSOLUTE(ADDR(.text));
};

In that case, we have a broken output when output target does
not use thunks. That happens because thunks creating code
calls Script->assignAddresses() at least once one more time,
what fixups the values.

In this patch, I generalize and rename maybeAddThunks to be used
by all targets.

Diff Detail

Event Timeline

grimar created this revision.Dec 11 2018, 2:16 AM

Overall I'm happy with this change as I think it is simpler than adding another call to Writer<ELFT>::run(). The one remaining thought is whether Writer<ELFT>::run() can be moved into finalizeAddressDependentContent() as there shouldn't be anything after that function that changes address. Could we move the remaining Writer<ELFT>::run() to the end of finalizeAddressDependentContent() ?

Overall I'm happy with this change as I think it is simpler than adding another call to Writer<ELFT>::run(). The one remaining thought is whether Writer<ELFT>::run() can be moved into finalizeAddressDependentContent() as there shouldn't be anything after that function that changes address. Could we move the remaining Writer<ELFT>::run() to the end of finalizeAddressDependentContent() ?

I am not sure honestly. I like that we do not spread and have all the finalizeSynthetic in one place (in one method) and I
like that finalizeAddressDependentContent is an isolated piece where the iteration magic happens.
I would probably try to avoid adding the additional code into it. This patch only removes the code from there
and I think it is good.

But If we decide to do something like this, I would suggest doing it in a different patch.

I think this should be landed because at least it simplifies the code. Ping.