[PATCH] D111137: [ELF] Propagate phdrs to orphan section with a smaller rank

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin updated this revision to Diff 378667.
ikudrin marked 2 inline comments as done.
ikudrin retitled this revision from "[ELF] Fix assigning segments to orphan sections" to "[ELF] Propagate phdrs to orphan section with a smaller rank".
ikudrin added a comment.

- Updated the title
- Updated the comment
- Removed `&& foundSec->hasInputSections`

In D111137#3053918 <https://reviews.llvm.org/D111137#3053918>, @MaskRay wrote:

>> Fix assigning segments to orphan sections
>
> I'd not use "Fix". It makes a case more user-friendly but does not necessarily mean we have a bug.
> I may use something like: Propagate `phdrs` to orphan section with a smaller rank.

Thanks for the suggestion!

> One practical problem is that the rule of thumb "the first output section of each segment should be specified" is not communicated among users.

A user may not be aware of all the nuances of the linker. For example, they might switch from a different linker that had different rules to place orphan sections and just continue using the same linker script that already worked for them.

> - the new logic is inconsistent when the `PHDRS` command is not used.

If no `PHDRS` is used, the new segment will start at the orphan section which is placed before the found section. From this perspective, the new logic works similarly.

> - it duplicates the `maybePropagatePhdrs` phdrs propagation logic in a different place

Right, and it also modifies its data, which is usually not expected in a `find` function.

Alternatively, we can place orphan sections after the closest-rank section, even if their sort rank is smaller. This may relieve the concerns, in the cost of slightly changing the resulting layout.


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

https://reviews.llvm.org/D111137

Files:
  lld/ELF/Writer.cpp
  lld/test/ELF/linkerscript/orphan-phdrs2.test


Index: lld/test/ELF/linkerscript/orphan-phdrs2.test
===================================================================
--- /dev/null
+++ lld/test/ELF/linkerscript/orphan-phdrs2.test
@@ -0,0 +1,33 @@
+# REQUIRES: x86
+
+# RUN: split-file %s %ts
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %ts/s -o %t.o
+# RUN: ld.lld -pie -o %t -T %ts/t %t.o
+# RUN: llvm-readelf -l %t | FileCheck %s
+
+## Check that an orphan section '.dynamic' is added to the same segment as
+## its closest-rank section '.data', even though it is placed before '.data'.
+## Adding 'dynamic' to the first segment would make the segment writable.
+# CHECK:     LOAD {{.*}} R E
+# CHECK:     LOAD {{.*}} RW
+
+# CHECK:     Segment Sections...
+# CHECK-NOT: 00{{.*}} .dynamic
+# CHECK:     01 .dynamic .data
+
+#--- s
+  .text
+  nop
+
+  .data
+  .quad 0
+
+#--- t
+PHDRS {
+  exec PT_LOAD;
+  rw   PT_LOAD;
+}
+SECTIONS {
+  .text : { *(.text) } : exec
+  .data : { *(.data) } : rw
+}
Index: lld/ELF/Writer.cpp
===================================================================
--- lld/ELF/Writer.cpp
+++ lld/ELF/Writer.cpp
@@ -1257,6 +1257,15 @@
   if (i == e)
     return e;
 
+  // If the orphan section is going to be placed before the found section,
+  // inherit explicitly specified segments from that section; otherwise, the
+  // orphan section may be placed in the previous segment. This may change the
+  // flags of that segment, for example, adding PF_W to a read-only segment.
+  auto foundSec = dyn_cast<OutputSection>(*i);
+  if (foundSec && sec->sortRank < foundSec->sortRank &&
+      script->hasPhdrsCommands())
+    sec->phdrs = foundSec->phdrs;
+
   // Consider all existing sections with the same proximity.
   int proximity = getRankProximity(sec, *i);
   for (; i != e; ++i) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D111137.378667.patch
Type: text/x-patch
Size: 1773 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211011/d25e74e5/attachment.bin>