[PATCH] D95199: [ELF] Write output sections in PT_LOAD segment order

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've had a look at a couple of the tests (not looked at the others) there look to be some ordering problems with some special cases like partition sections and .tbss. To find these I checked the order of writing sections with the new and old. As mentioned in the comment I don't know much about the partition mechanism, as such I'd recommend making sure that there were no explainable differences in those tests. Will try and find some more time to look at the others later in the week/weekend.



================
Comment at: lld/test/ELF/partition-synthetic-sections.s:195
 // FILL-NEXT: *
-// FILL-NEXT: 005000
+// FILL-NEXT: 006000
 
----------------
pattop wrote:
> This looks harmless to me as the output is shifted by exactly one page and can be explained by a slight reordering of sections in the output. Everything else is identical.
> 
> I haven't done a detailed analysis of why this moves. If there are concerns I can investigate further.
Looking at the ordering of sections the special part1 and .part.end  sections are not written in the same order. In particular .part.end comes after part1. I'm not familiar with the partition feature, but it is used by chrome, which is very aggressive at reverting any patch that breaks it. I think it will be worth understanding how partitions intersect with pt_load


================
Comment at: lld/test/ELF/tls-offset.s:58
 // CHECK2-NEXT: Address: 0x202010
-// CHECK2-NEXT: Offset: 0x2004
+// CHECK2-NEXT: Offset: 0x2008
 // CHECK-NEXT:  Size: 16
----------------
pattop wrote:
> I don't think the file offsets here are important. .tbss is a NOBITS section and has no file data associated with it.
> 
> Somewhat concerningly the TLS segment changes size from 4 bytes to 8 byes (not covered by this test) which should probably be investigated.
There is an ordering difference. See needsPTLoad the .tbss is deliberately not assigned to a program header so it is getting written in the non SHF_ALLOC portion. This may not be a problem in practice but it does result in a discontinuity in file offsets that is at the least confusing. Personally I'd prefer that we write the .tbss in its address order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95199