[PATCH] D111137: [ELF] Fix assigning segments to orphan sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

> 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.

I have a mixed feeling on this patch. It does make it a bit more user-friendly for the added case by not making the non-PF_W segment PF_W, but:

- the logic is inconsistent when the `PHDRS` command is not used.
- it makes the `maybePropagatePhdrs` phdrs propagation logic duplicate in a different place

In addition, the original rationale is not sufficient justifying.
>From https://maskray.me/blog/2021-07-04-sections-and-overwrite-sections, when linking a regular program, if one really wants to use a linker script, the linker script should at least specify the first output section of each segment, specifically including the first output section of PT_GNU_RELRO. A minimal example:

  text
  SECTIONS
  {
    PROVIDE (__executable_start = SEGMENT_START("text-segment", 0x400000)); . = SEGMENT_START("text-segment", 0x400000) + SIZEOF_HEADERS;
  
    . = DATA_SEGMENT_ALIGN (CONSTANT (MAXPAGESIZE), CONSTANT (COMMONPAGESIZE));
    .init_array    :
    {
      PROVIDE_HIDDEN (__init_array_start = .);
      KEEP (*(SORT_BY_INIT_PRIORITY(.init_array.*)))
      KEEP (*(.init_array))
      PROVIDE_HIDDEN (__init_array_end = .);
    }
    .fini_array    :
    {
      PROVIDE_HIDDEN (__fini_array_start = .);
      KEEP (*(SORT_BY_INIT_PRIORITY(.fini_array.*)))
      KEEP (*(.fini_array))
      PROVIDE_HIDDEN (__fini_array_end = .);
    }
    . = DATA_SEGMENT_RELRO_END (0, .);
    .data           : { *(.data .data.*) }
    . = .;
    .bss            : { *(.bss .bss.*) *(COMMON) }
    . = DATA_SEGMENT_END (.);
  }

I haven't formed my opinion completely, and want to hear from Peter, but do have some concern on this direction.



================
Comment at: lld/ELF/Writer.cpp:1263
+  // the orphan section may be placed in the previous segment, which may
+  // corrupt its flags.
+  auto foundSec = dyn_cast<OutputSection>(*i);
----------------
"corrupt" is inaccurate. A non-PF_W segment may now become PF_W, but it is not corrupted.


================
Comment at: lld/ELF/Writer.cpp:1265
+  auto foundSec = dyn_cast<OutputSection>(*i);
+  if (script->hasPhdrsCommands() && foundSec && foundSec->hasInputSections &&
+      sec->sortRank < foundSec->sortRank)
----------------
`&& foundSec->hasInputSections` can be deleted


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111137