[PATCH] D38361: [ELF] Stop setting output section size early

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: peter.smith.
jhenderson added a comment.

In https://reviews.llvm.org/D38361#890146, @ruiu wrote:

> This is much more complicated than https://reviews.llvm.org/D38321, but I wonder if it has to be. I feel like this part of code is becoming out of control.


Are you referring to the MipsGot stuff, or the addSection code? I feel like this change makes the addSection code more correct, if anything, as it stops setting a "false" value for OutputSection::Size.



================
Comment at: ELF/SyntheticSections.cpp:844-848
+    // The OutputSection Size is not set until after assignAddresses has been
+    // called, so we cannot use it. Instead we make a rough estimate of the
+    // section size, which should be sufficient as long as users don't start
+    // modifying the section size via the linker script.
+    PageEntriesNum += getMipsPageCount(estimateOutputSectionSize(P.first));
----------------
ruiu wrote:
> What happens when a user starts modifying the output using the linker script?
The output section sizes will be different to what is estimated here, meaning that the number of GOT entries may not be entirely correct. I think that the only way around this is if we properly loop over assignAddresses until the sizes are all stable. This is an existing issue with the current behaviour anyway, and I see that @peter.smith already mentioned this in D29983#681632:
> In the general case we will need to assign addresses multiple times as it is possible to write linker scripts where adding thunks causes address expressions in the script to change in ways that breaks the address calculations. 
The size calculation for the GOT relies on the size calculated by assignAddresses, but that is not final until after thunk creation. As things stand, I think the code is slightly broken anyway, but I think it might be fixable by calling assignAddresses again immediately before the updateAllocSize call. I will look a that shortly.


https://reviews.llvm.org/D38361