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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D66279#1634954 <https://reviews.llvm.org/D66279#1634954>, @peter.smith wrote:

> Thanks for the update. Have been thinking in the back of my mind if there were any other way of solving the problem without needing to iterate. The only thing I can think of right now is some kind of analysis that, for want of a better word, topologically sorts expressions so that there are no forward references, with an error message if this isn't possible. This could get complicated as there is limited scope to move or reorder assignments in OutputSections. Other than that I've not got any more comments at the moment.


Unfortunately linker scripts are not in a single static assignment form... `=` and `+=` can appear multiple times. Such analysis will be difficult. According to my experiments (see prior comments), ld.bfd's evaluation strategy is quite similar to: run assignment statements one by one, but repeat a few times, where the repetition count depends on layout convergence. It doesn't wait for symbol assignments to converge, but it evaluates the assignment statements more than we current do. Thus it can correctly evaluate some simple forward declaration forms. I think the approach we use in this patch should capture its strategy quite well. This is probably sufficient for most forward declarations we may encounter in practice.



================
Comment at: ELF/LinkerScript.cpp:1065
 
+  DenseMap<const Defined *, std::pair<SectionBase *, uint64_t>> old;
+  for (BaseCommand *base : sectionCommands) {
----------------
peter.smith wrote:
> It might be worth extracting this and the check below into functions. Something like:
> ```
> oldValues = getSymbolAssignmentValues(sectionCommands);
> ...
> return findChangedSymbol(oldValues);
> ```
> Not a strong opinion though.
> 
I'll do:

```
oldValues = getSymbolAssignmentValues(sectionCommands);
...
for (auto &it : oldValues) { /// this part is not too complex now, so i'll inline it
  ...
}
return changed;
```


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D66279