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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I think that this is close. I've got some concerns about the interaction of convergence limits, and it may be worth investigating if it is possible to make a separate symbol update pass that runs after addresses have stabilized.

I think it will be worth adding a test to make sure that the bail out does indeed occur. Something like:

  sym1 = sym2
  sym2 = sym3
  sym3 = sym4
  sym4 = sym5
  sym6 = sym7
  sym8 = .

I'm happy to work on a thunk creation test that takes quite a few iterations to converge.



================
Comment at: ELF/Writer.cpp:1571
   // For some targets, like x86, this loop iterates only once.
+  int tries = 5;
   for (;;) {
----------------
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. 


================
Comment at: ELF/Writer.cpp:1595
+      if (--tries == 0) {
+        errorOrWarn("assignment to symbol " + toString(*changedSym) +
+                    " does not converge");
----------------
To follow on from the previous comment about convergence errors in Thunks, if we choose to keep the convergence tests separate so we know which part isn't converging (if that is even possible) then it might be worth moving the tries and error message into script and assignAddresses() to keep this loop clean. 


================
Comment at: ELF/Writer.cpp:1602
 }
 
 static void finalizeSynthetic(SyntheticSection *sec) {
----------------
An argument could be made that testing for convergence of symbols could/should be done after the final addresses have stabilised. I don't think that there is a legal linker script that could be made where assigning to symbols in a linker script should affect the convergence of thunks, patches and partitions. Doing the symbol assignment with stable addresses would avoid the clashing convergence limit problem.

My initial thought was to see if just a symbol assignment update could be run here, without going through the complexity of assignAddresses() but that would definitely need some thought.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D66279