[PATCH] D55423: [LLD][ELF] - A fix for "linker script assignment loses relative nature of section" bug.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D55423#1324212 <https://reviews.llvm.org/D55423#1324212>, @ruiu wrote:

> This seems tricky to me. I wonder if you can just fix the problem by reordering symbol assignments in your linker scripts.


Reordering can help to set the correct symbols values. I.e. the following would work for PR:

  SECTIONS { .text 0x1000 : { __text = . ; *(.text) } }
  aliasto__text = __text;

But it does not look like a final solution. Without reordering we still produce the broken output which might be hard to diagnose.
And I think the initial pattern is used in the wild.

As an example: we have a `addr-zero.test` test case already (implemented in rL298083 <https://reviews.llvm.org/rL298083>) which is broken right now after recent rL332038 <https://reviews.llvm.org/rL332038>.
The problem has the same roots, just reordering the sections broke the correctness of symbols evaluation. Doing the one more pass would
solve this once and forever it seems.

> Also, by allowing forward reference in an assignment, you could theoretically write circular symbol assignments, e.g. foo=bar and bar=foo.

LLD already accepts this. For script containing `foo = boo; boo = foo;` we create 2 absolute zero symbols.
For just `foo = boo` we report a "symbol not found: boo" as expected.

It is not ideal, It happens because we declare the symbols early anyways as a Defined to let
they exist before starting LTO and versioning.

But I think that is just one more example that linker scripts should be used carefully.
I do not think we really should care about a fix for this, because gold has equal behavior it seems (though bfd - not).

> Maybe we should just keep the linearity of the symbol assignment, instead of supporting this corner case?

That sure would be the simplest and desired way, if we could drop the existent scripts I think and write LS logic from scratch.
But we have to support non-linearity behavior which is assumed to work I think. Or report an error.

I see one more argument that I find important for the LLD design. See our code in `maybeAddThunks`:
https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L1500

It was implemented to create thunks, but now it does not only that. It is called not only when `Target->NeedsThunks`,
but also when packed relocations are used. As a result, for some targets, we **always** call `assignAddresses` at least once again,
and for others - never do.

This makes the behavior for those platforms to be different from the common case (assuming it is x86-x64).
It is not a problem to call `assignAddresses` few times, it was designed for that.
And in this patch, I had to add `assignAddresses` to make other targets, that do not use `maybeAddThunks` to be happy.
I would prefer to have the consistent behavior of the linker script for all targets. I think we could rename `maybeAddThunks`
to something else, update its comment and make  `assignAddresses` be always called at least once for each target.
That would remove 2 of 4 code lines this patch has. It is very short for the problem it solves.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55423/new/

https://reviews.llvm.org/D55423