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

[Patrick Oppenlander via Phabricator via llvm-commits]

pattop added inline comments.


================
Comment at: lld/test/ELF/partition-synthetic-sections.s:195
 // FILL-NEXT: *
-// FILL-NEXT: 005000
+// FILL-NEXT: 006000
 
----------------
peter.smith wrote:
> 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
You're right, the ordering is different, but the extracted partitions still pass all of the tests which is why I thought this probably wasn't significant.

I'm struggling to understand what this 'od' test is actually validating. I read through D62350 which is where the test originated but didn't find any explanation.

Unfortunately I'm not in a position where I can try a chrome build to see if anything breaks.


================
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
----------------
peter.smith wrote:
> 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.
Explainable file offsets is one reason why I think setting a file offset on sections without file data isn't helpful. IMHO it would be easier to expalin that the file offset on a section with no file data is always 0.

Writing .tbss in address order should be doable, I'll take a look.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95199