[PATCH] D38687: [ELF] Make section order rely on explicit member

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D38687#912500, @ruiu wrote:

> Is my understanding correct that if you check in https://reviews.llvm.org/D38687, you don't need this and https://reviews.llvm.org/D38361?
>
> I thought about all these patches carefully, and I'm leaning towards https://reviews.llvm.org/D38687 because it is by far simple, doesn't need a new member in any class, and it is becoming more in line with other code that uses section offsets. Section offsets used to be computed only once, and once they are set, they would never change. The situation quickly changed because of linker scripts and range extension thunks. In particular, in order to implement the range extension, we need to reassign offsets until they converge. So, section offsets are no longer computed-once value in any sense. So, it started feeling more natural to use section offsets than before.


So there are currently 3 related patches up for review, https://reviews.llvm.org/D38321, https://reviews.llvm.org/D38361, and https://reviews.llvm.org/D38687 (this one - I think you may have linked to the wrong review in the above comment, based on what you said). Either https://reviews.llvm.org/D38321 needs committing to fix the issue, or https://reviews.llvm.org/D38361, or https://reviews.llvm.org/D38687 and a stripped down version of https://reviews.llvm.org/D38361.

I've looked again at https://reviews.llvm.org/D38321, and although it is sufficient to fix the problem, it still feels like a sticky plaster covering up the fact that we compute a potentially incorrect section Size and Offset too early, which then causes problems further down the line (note that there is even a comment in the code explaining that this is a crude approximation). https://reviews.llvm.org/D38361 is a more comprehensive fix to this, as it means we have no Size or Offset set until assignAddresses is called (which is correct, because only assignAddresses has all the necessary information needed to calculate these). To me, the converging loop point is a different situation, in my opinion, because the Size and Offset values are always correct immediately after assignAddresses for the ELF as it stands at that point, although sections might later need adding/expanding (e.g. due to thunks), hence the need to rerun assignAddresses.

It's also worth pointing out that the changes for the MIPS GOT dynamic tag in https://reviews.llvm.org/D38361 are needed regardless, since this needs to be set post-thunk creation, and take into account linker script section layout. I should probably spin off a separate patch for that, thinking about it, since I'm pretty sure it's actually a bug even without these other changes - I'll see if I can come up with a test case to illustrate.

I'm concerned that by leaving the potentially incorrect Size and Offset as is, we may cause further confusion and have more code in the future that tries to rely on these values before they are finalized, so my preference is still to go with https://reviews.llvm.org/D38361 (and maybe this one, https://reviews.llvm.org/D38687).


https://reviews.llvm.org/D38687