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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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));
----------------
jhenderson wrote:
> 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.
Whilst attempting to avoid this estimating by calling assignAddresses again, I found another issue, which is also theoretically present prior to my changes: the DT_MIPS_LOCAL_GOTNO depends on the number of local got entries, but these are dependent on the size calculated in updateAllocSize(). Updating this size, e.g. due to additional thunks being created, might render the the dynamic tag value invalid.


https://reviews.llvm.org/D38361