[PATCH] D66279: [ELF] Make LinkerScript::assignAddresses iterative

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: ELF/Writer.cpp:1571
   // For some targets, like x86, this loop iterates only once.
+  int tries = 5;
   for (;;) {
----------------
peter.smith wrote:
> The thunks code already has a converge limit of 10, I think it would be possible to create a test case that had a single assignment say sym = . that was after a code section that would vary in size due to Thunk creation. If the code section converged within 10 but not within 5 then we'd get an error. I think if the symbol convergence test is within this loop then we should probably extract the convergence test out of ThunkCreation, or at least have some way of synchronizing all the convergence limits.
> 
> Writing a thunk test that doesn't converge isn't difficult, writing one that takes many iterations to convergence is somewhat tedious. I can help write one as I've done a few before. 
This is a bit different. The return value of `script->assignAddresses();` does not affect `changed`, i.e. before thunks converge (`changed` becomes false) `tries` should not decrease.

`script->assignAddresses` is executed at least twice, without or with this patch. This is a bit awkward, but if I change the logic here to:

  for (;;)
    assignAddresses();
    createThunks();
    if (!changed) break;

test/ELF/linkerscript/addr-zero.test added by @grimar's D55550 will break.

> foo = ADDR(.text) - ABSOLUTE(ADDR(.text));

Ideally we don't have to call `assignAddresses` twice.

> Writing a thunk test that doesn't converge isn't difficult, writing one that takes many iterations to convergence is somewhat tedious.

Yes, a test involving both non-trivial symbol assignment and non-trivial thunk will be very useful... Thanks in advance.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D66279